| Message ID | 9b09c7fe-cf5c-fe48-2bdf-d7ac692da2e1@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 15 Jun 2017 11:54:29 +0200 Peter Rosin <peda@axentia.se> wrote: > On 2017-06-13 17:30, Boris Brezillon wrote: > > Hi Peter, > > > > On Tue, 13 Jun 2017 16:34:25 +0200 > > Peter Rosin <peda@axentia.se> wrote: > > > >> Hi! > >> > >> I need color lookup support for the atmel-hlcdc driver, and had a peek > >> at the code. I also looked at the drivers/gpu/drm/stm driver and came > >> up with the below diff. It compiles, but I have not booted it for the > >> simple reason that I can't imagine it will work. > >> > >> Sure, the code fills the clut registers in the .load_lut helper function > >> and I think it might even do the right thing when setting up the mode. > >> I'm less sure DMA will work correctly? It might, but I haven't looked at > >> it for many seconds... > >> > >> The big question is how the color info gets into the new clut array > >> I created in atmel_hlcdc_crtc? When I look at the stm driver, which does > >> it just like this AFAICT I just don't see it. So, what I'm I missing? > > > > You should look at drm_atomic_helper_legacy_gamma_set() and its users. > > Ok, thanks. I had a long look and could not get it to work at all. Probably because you're trying to set this up through the emulated fbdev interface. The solution I proposed is supposed to be accessed through DRM ioctls. > So, > I did as below instead. However, there are a few glaring problems... Well, I doubt this will be accepted. The fbdev emulation layer is supposed to be rather dumb, partly because DRM people want developers to switch to the DRM interface. Also note that I won't accept a solution that supports setting the LUT table only through the fbdev interface, so please try to make it work the DRM way before you even consider modifying the fbdev emulation layer. > > Something, somewhere, sets up default entries for the 16 first entries > of the palette and seem to expect them to stay as some kind of safe > basic palette, and my applications don't handle it too well. When they > want to draw black, they get a poisonous cyan/green instead etc. But > it's pretty close. Can anyone provide some clue as to how that is > supposed to be handled? > > Also, I had to change the second argument of the drm_fbdev_cma_init... > call at the end of atmel_hlcdc_dc.c:atmel_hlcdc_dc_load() from 24 to 8 > to make it possible to set 8-modes. However, doing so fucks up 24-bit > modes. Which made me suspect that no non-24-bit mode work w/o my patch. > And I could indeed only get 24-bit modes to work, unless I changed this > value to 16. Then RGB565 works like a charm, but RGB888 don't. What's > up with that? Seems like a rather silly limitation, but it's perhaps > just a bug? I'm pretty sure this is not a bug. AFAIR, prefered_bpp is used when you don't explicitly specify the video mode you want to use on the cmdline [1]. [1]http://elixir.free-electrons.com/linux/v3.11/source/Documentation/fb/modedb.txt
On 2017-06-15 12:15, Boris Brezillon wrote: > On Thu, 15 Jun 2017 11:54:29 +0200 > Peter Rosin <peda@axentia.se> wrote: > >> On 2017-06-13 17:30, Boris Brezillon wrote: >>> Hi Peter, >>> >>> On Tue, 13 Jun 2017 16:34:25 +0200 >>> Peter Rosin <peda@axentia.se> wrote: >>> >>>> Hi! >>>> >>>> I need color lookup support for the atmel-hlcdc driver, and had a peek >>>> at the code. I also looked at the drivers/gpu/drm/stm driver and came >>>> up with the below diff. It compiles, but I have not booted it for the >>>> simple reason that I can't imagine it will work. >>>> >>>> Sure, the code fills the clut registers in the .load_lut helper function >>>> and I think it might even do the right thing when setting up the mode. >>>> I'm less sure DMA will work correctly? It might, but I haven't looked at >>>> it for many seconds... >>>> >>>> The big question is how the color info gets into the new clut array >>>> I created in atmel_hlcdc_crtc? When I look at the stm driver, which does >>>> it just like this AFAICT I just don't see it. So, what I'm I missing? >>> >>> You should look at drm_atomic_helper_legacy_gamma_set() and its users. >> >> Ok, thanks. I had a long look and could not get it to work at all. > > Probably because you're trying to set this up through the emulated > fbdev interface. The solution I proposed is supposed to be accessed > through DRM ioctls. Yes, that seems like the root cause of most of my troubles. But, the apps I have seem to use the fbdev interface, and if the emulation is poorly done, they won't work right... >> So, >> I did as below instead. However, there are a few glaring problems... > > Well, I doubt this will be accepted. The fbdev emulation layer is > supposed to be rather dumb, partly because DRM people want developers > to switch to the DRM interface. Sigh. > Also note that I won't accept a solution that supports setting the LUT > table only through the fbdev interface, so please try to make it work > the DRM way before you even consider modifying the fbdev emulation > layer. Ok, I'll have another look. But to me, it's a maze... >> >> Something, somewhere, sets up default entries for the 16 first entries >> of the palette and seem to expect them to stay as some kind of safe >> basic palette, and my applications don't handle it too well. When they >> want to draw black, they get a poisonous cyan/green instead etc. But >> it's pretty close. Can anyone provide some clue as to how that is >> supposed to be handled? >> >> Also, I had to change the second argument of the drm_fbdev_cma_init... >> call at the end of atmel_hlcdc_dc.c:atmel_hlcdc_dc_load() from 24 to 8 >> to make it possible to set 8-modes. However, doing so fucks up 24-bit >> modes. Which made me suspect that no non-24-bit mode work w/o my patch. >> And I could indeed only get 24-bit modes to work, unless I changed this >> value to 16. Then RGB565 works like a charm, but RGB888 don't. What's >> up with that? Seems like a rather silly limitation, but it's perhaps >> just a bug? > > I'm pretty sure this is not a bug. AFAIR, prefered_bpp is used when you > don't explicitly specify the video mode you want to use on the cmdline > [1]. > > [1]http://elixir.free-electrons.com/linux/v3.11/source/Documentation/fb/modedb.txt > (Why refer to v3.11 docs?) Let's start with some basics, I tried to add video=atmel-hclcd-dc:1024x768-16 and video=atmel_hclcd_dc:1024x768-16 to the kernel cmdline of an unpatched linux-next kernel. No dice, I can still only access RBG888. I'm obviously in need of some hand-holding... How am I supposed to get a 16-bit mode? Cheers, peda
On Thu, 15 Jun 2017 13:47:35 +0200 Peter Rosin <peda@axentia.se> wrote: > On 2017-06-15 12:15, Boris Brezillon wrote: > > On Thu, 15 Jun 2017 11:54:29 +0200 > > Peter Rosin <peda@axentia.se> wrote: > > > >> On 2017-06-13 17:30, Boris Brezillon wrote: > >>> Hi Peter, > >>> > >>> On Tue, 13 Jun 2017 16:34:25 +0200 > >>> Peter Rosin <peda@axentia.se> wrote: > >>> > >>>> Hi! > >>>> > >>>> I need color lookup support for the atmel-hlcdc driver, and had a peek > >>>> at the code. I also looked at the drivers/gpu/drm/stm driver and came > >>>> up with the below diff. It compiles, but I have not booted it for the > >>>> simple reason that I can't imagine it will work. > >>>> > >>>> Sure, the code fills the clut registers in the .load_lut helper function > >>>> and I think it might even do the right thing when setting up the mode. > >>>> I'm less sure DMA will work correctly? It might, but I haven't looked at > >>>> it for many seconds... > >>>> > >>>> The big question is how the color info gets into the new clut array > >>>> I created in atmel_hlcdc_crtc? When I look at the stm driver, which does > >>>> it just like this AFAICT I just don't see it. So, what I'm I missing? > >>> > >>> You should look at drm_atomic_helper_legacy_gamma_set() and its users. > >> > >> Ok, thanks. I had a long look and could not get it to work at all. > > > > Probably because you're trying to set this up through the emulated > > fbdev interface. The solution I proposed is supposed to be accessed > > through DRM ioctls. > > Yes, that seems like the root cause of most of my troubles. But, the apps > I have seem to use the fbdev interface, and if the emulation is poorly > done, they won't work right... > > >> So, > >> I did as below instead. However, there are a few glaring problems... > > > > Well, I doubt this will be accepted. The fbdev emulation layer is > > supposed to be rather dumb, partly because DRM people want developers > > to switch to the DRM interface. > > Sigh. > > > Also note that I won't accept a solution that supports setting the LUT > > table only through the fbdev interface, so please try to make it work > > the DRM way before you even consider modifying the fbdev emulation > > layer. > > Ok, I'll have another look. But to me, it's a maze... > > >> > >> Something, somewhere, sets up default entries for the 16 first entries > >> of the palette and seem to expect them to stay as some kind of safe > >> basic palette, and my applications don't handle it too well. When they > >> want to draw black, they get a poisonous cyan/green instead etc. But > >> it's pretty close. Can anyone provide some clue as to how that is > >> supposed to be handled? > >> > >> Also, I had to change the second argument of the drm_fbdev_cma_init... > >> call at the end of atmel_hlcdc_dc.c:atmel_hlcdc_dc_load() from 24 to 8 > >> to make it possible to set 8-modes. However, doing so fucks up 24-bit > >> modes. Which made me suspect that no non-24-bit mode work w/o my patch. > >> And I could indeed only get 24-bit modes to work, unless I changed this > >> value to 16. Then RGB565 works like a charm, but RGB888 don't. What's > >> up with that? Seems like a rather silly limitation, but it's perhaps > >> just a bug? > > > > I'm pretty sure this is not a bug. AFAIR, prefered_bpp is used when you > > don't explicitly specify the video mode you want to use on the cmdline > > [1]. > > > > [1]http://elixir.free-electrons.com/linux/v3.11/source/Documentation/fb/modedb.txt > > > > (Why refer to v3.11 docs?) > > Let's start with some basics, I tried to add > > video=atmel-hclcd-dc:1024x768-16 > > and > > video=atmel_hclcd_dc:1024x768-16 > > to the kernel cmdline of an unpatched linux-next kernel. No dice, I can > still only access RBG888. I'm obviously in need of some hand-holding... > > How am I supposed to get a 16-bit mode? You should have a look at [1]. [1]https://www.at91.com/linux4sam/bin/view/Linux4SAM/UsingAtmelDRMDriver#Choose_supported_modes
On Thu, Jun 15, 2017 at 12:15:58PM +0200, Boris Brezillon wrote: > On Thu, 15 Jun 2017 11:54:29 +0200 > Peter Rosin <peda@axentia.se> wrote: > > > On 2017-06-13 17:30, Boris Brezillon wrote: > > > Hi Peter, > > > > > > On Tue, 13 Jun 2017 16:34:25 +0200 > > > Peter Rosin <peda@axentia.se> wrote: > > > > > >> Hi! > > >> > > >> I need color lookup support for the atmel-hlcdc driver, and had a peek > > >> at the code. I also looked at the drivers/gpu/drm/stm driver and came > > >> up with the below diff. It compiles, but I have not booted it for the > > >> simple reason that I can't imagine it will work. > > >> > > >> Sure, the code fills the clut registers in the .load_lut helper function > > >> and I think it might even do the right thing when setting up the mode. > > >> I'm less sure DMA will work correctly? It might, but I haven't looked at > > >> it for many seconds... > > >> > > >> The big question is how the color info gets into the new clut array > > >> I created in atmel_hlcdc_crtc? When I look at the stm driver, which does > > >> it just like this AFAICT I just don't see it. So, what I'm I missing? > > > > > > You should look at drm_atomic_helper_legacy_gamma_set() and its users. > > > > Ok, thanks. I had a long look and could not get it to work at all. > > Probably because you're trying to set this up through the emulated > fbdev interface. The solution I proposed is supposed to be accessed > through DRM ioctls. Yup, no one typed the fbdev emulation support to use the new&fancy color manager stuff if available. That needs to be done if you want that. Also note that even the existing legacy lut support for fbdev is in imo rather bad shape, and I really don't want to see it's usage spread (it has its own special functions which use neither the old nor new kms interfaces, forcing drivers to duplicate stuff for no gain at all). > > So, > > I did as below instead. However, there are a few glaring problems... > > Well, I doubt this will be accepted. The fbdev emulation layer is > supposed to be rather dumb, partly because DRM people want developers > to switch to the DRM interface. > > Also note that I won't accept a solution that supports setting the LUT > table only through the fbdev interface, so please try to make it work > the DRM way before you even consider modifying the fbdev emulation > layer. Correct :-) Bunch more details: fbdev emulation in drm_fb_helper.c uses ->gamma_set and ->load_lut, that needs to be replaced by the proper kms interfaces (and using the color manager properties for atomic drivers directly, if available). Cheers, Daniel
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 53489859997b..62ef30676cc3 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc { struct atmel_hlcdc_dc *dc; struct drm_pending_vblank_event *event; int id; + u32 clut[256]; }; static inline struct atmel_hlcdc_crtc * @@ -140,6 +141,23 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) cfg); } +static void +atmel_hlcdc_crtc_load_lut(struct drm_crtc *c) +{ + struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c); + struct atmel_hlcdc_dc *dc = crtc->dc; + unsigned int color; + int layer; + + for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) { + if (!dc->layers[layer]) + continue; + for (color = 0; color < 256; color++) + atmel_hlcdc_layer_write_clut(dc->layers[layer], + color, crtc->clut[color]); + } +} + static enum drm_mode_status atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c, const struct drm_display_mode *mode) @@ -319,6 +337,7 @@ static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = { .mode_set = drm_helper_crtc_mode_set, .mode_set_nofb = atmel_hlcdc_crtc_mode_set_nofb, .mode_set_base = drm_helper_crtc_mode_set_base, + .load_lut = atmel_hlcdc_crtc_load_lut, .disable = atmel_hlcdc_crtc_disable, .enable = atmel_hlcdc_crtc_enable, .atomic_check = atmel_hlcdc_crtc_atomic_check, diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 30dbffdb45a3..4723cadb67c4 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -37,6 +37,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = { .id = 0, .type = ATMEL_HLCDC_BASE_LAYER, .cfgs_offset = 0x2c, + .clut_offset = 0x400, .layout = { .xstride = { 2 }, .default_color = 3, @@ -66,6 +67,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = { .id = 0, .type = ATMEL_HLCDC_BASE_LAYER, .cfgs_offset = 0x2c, + .clut_offset = 0x400, .layout = { .xstride = { 2 }, .default_color = 3, @@ -81,6 +83,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = { .id = 1, .type = ATMEL_HLCDC_OVERLAY_LAYER, .cfgs_offset = 0x2c, + .clut_offset = 0x800, .layout = { .pos = 2, .size = 3, @@ -99,6 +102,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = { .id = 2, .type = ATMEL_HLCDC_OVERLAY_LAYER, .cfgs_offset = 0x4c, + .clut_offset = 0x1000, .layout = { .pos = 2, .size = 3, @@ -122,6 +126,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = { .max_width = 128, .max_height = 128, .cfgs_offset = 0x2c, + .clut_offset = 0x1400, .layout = { .pos = 2, .size = 3, @@ -155,6 +160,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = { .id = 0, .type = ATMEL_HLCDC_BASE_LAYER, .cfgs_offset = 0x2c, + .clut_offset = 0x600, .layout = { .xstride = { 2 }, .default_color = 3, @@ -170,6 +176,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = { .id = 1, .type = ATMEL_HLCDC_OVERLAY_LAYER, .cfgs_offset = 0x2c, + .clut_offset = 0xa00, .layout = { .pos = 2, .size = 3, @@ -188,6 +195,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = { .id = 2, .type = ATMEL_HLCDC_OVERLAY_LAYER, .cfgs_offset = 0x2c, + .clut_offset = 0xe00, .layout = { .pos = 2, .size = 3, @@ -206,6 +214,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = { .id = 3, .type = ATMEL_HLCDC_OVERLAY_LAYER, .cfgs_offset = 0x4c, + .clut_offset = 0x1200, .layout = { .pos = 2, .size = 3, @@ -233,6 +242,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = { .max_width = 128, .max_height = 128, .cfgs_offset = 0x2c, + .clut_offset = 0x1600, .layout = { .pos = 2, .size = 3, diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h index b0596a84c1b8..a2cc2ab923fa 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h @@ -88,6 +88,11 @@ #define ATMEL_HLCDC_YUV422SWP BIT(17) #define ATMEL_HLCDC_DSCALEOPT BIT(20) +#define ATMEL_HLCDC_C1_MODE ATMEL_HLCDC_CLUT_MODE(0) +#define ATMEL_HLCDC_C2_MODE ATMEL_HLCDC_CLUT_MODE(1) +#define ATMEL_HLCDC_C4_MODE ATMEL_HLCDC_CLUT_MODE(2) +#define ATMEL_HLCDC_C8_MODE ATMEL_HLCDC_CLUT_MODE(3) + #define ATMEL_HLCDC_XRGB4444_MODE ATMEL_HLCDC_RGB_MODE(0) #define ATMEL_HLCDC_ARGB4444_MODE ATMEL_HLCDC_RGB_MODE(1) #define ATMEL_HLCDC_RGBA4444_MODE ATMEL_HLCDC_RGB_MODE(2) @@ -259,6 +264,7 @@ struct atmel_hlcdc_layer_desc { int id; int regs_offset; int cfgs_offset; + int clut_offset; struct atmel_hlcdc_formats *formats; struct atmel_hlcdc_layer_cfg_layout layout; int max_width; @@ -414,6 +420,14 @@ static inline u32 atmel_hlcdc_layer_read_cfg(struct atmel_hlcdc_layer *layer, (cfgid * sizeof(u32))); } +static inline void atmel_hlcdc_layer_write_clut(struct atmel_hlcdc_layer *layer, + unsigned int c, u32 val) +{ + atmel_hlcdc_layer_write_reg(layer, + layer->desc->clut_offset + c * sizeof(u32), + val); +} + static inline void atmel_hlcdc_layer_init(struct atmel_hlcdc_layer *layer, const struct atmel_hlcdc_layer_desc *desc, struct regmap *regmap) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index 1124200bb280..5537843e48fe 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -83,6 +83,7 @@ drm_plane_state_to_atmel_hlcdc_plane_state(struct drm_plane_state *s) #define SUBPIXEL_MASK 0xffff static uint32_t rgb_formats[] = { + DRM_FORMAT_C8, DRM_FORMAT_XRGB4444, DRM_FORMAT_ARGB4444, DRM_FORMAT_RGBA4444, @@ -100,6 +101,7 @@ struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_formats = { }; static uint32_t rgb_and_yuv_formats[] = { + DRM_FORMAT_C8, DRM_FORMAT_XRGB4444, DRM_FORMAT_ARGB4444, DRM_FORMAT_RGBA4444, @@ -128,6 +130,9 @@ struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_and_yuv_formats = { static int atmel_hlcdc_format_to_plane_mode(u32 format, u32 *mode) { switch (format) { + case DRM_FORMAT_C8: + *mode = ATMEL_HLCDC_C8_MODE; + break; case DRM_FORMAT_XRGB4444: *mode = ATMEL_HLCDC_XRGB4444_MODE; break;