| Message ID | 20190726203626.GA31474@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | My penguin has blue feets (aka: RGB versus BGR troubles) | expand |
On Fri, Jul 26, 2019 at 4:36 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi all. > > The Atmel at91sam9263 has a nice LCDC IP core that supports several > formats: > DRM_FORMAT_XBGR8888, DRM_FORMAT_BGR888, DRM_FORMAT_BGR565 > > (It also supports a palletized C8 format, but thats for later). > > The formats are all BGR formats - and some boards actually reverse Blue > and Red when wiring up the display. I have added a DT property to > identify boards with this difference. > > The penguin shown during boot of the kernel had blue feet which is a > clear sign that RED and GREEN was reversed. > > So to fix this I (got help from imirkin on irc) I implmented a quirk. > See patch below. > > With the quirk enabled I see: > - penguin shown during kernel boot has yellow feets (OK) > - modetest tell the driver reports: XB24 BG24 BG16 (as expected) > - modetest -s fails with: > # modetest -M atmel-lcdc -s 34:800x480-60Hz > setting mode 800x480-60Hz@XR24 on connectors 34, crtc 32 > failed to set mode: Function not implemented > > Snip from dmesg: > > drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_ADDFB2 > [drm:drm_mode_addfb2] [FB:37] > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETCRTC > [drm:drm_mode_setcrtc] [CRTC:32:crtc-0] > [drm:drm_mode_setcrtc] Invalid pixel format XR24 little-endian (0x34325258), modifier 0x0 > ^^^^ > [drm:drm_mode_object_put] OBJ ID: 37 (2) > [drm:drm_ioctl] pid=208, ret = -22 > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_DIRTYFB > [drm:drm_mode_object_put] OBJ ID: 37 (2) > [drm:drm_ioctl] pid=208, ret = -38 > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_RMFB > [drm:drm_mode_object_put] OBJ ID: 37 (2) > [drm:drm_mode_object_put] OBJ ID: 37 (1) > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_DESTROY_DUMB > [drm:drm_release] open_count = 1 > [drm:drm_file_free] pid = 208, device = 0xe201, open_count = 1 > [drm:drm_lastclose] > [drm:drm_lastclose] driver lastclose completed > > Notice that somehow we have a framebuffer in XR24 format, which is not > supported by the driver. > > > I have tried to tell that my driver supports DRM_FORMAT_XRGB8888, > DRM_FORMAT_RGB888, DRM_FORMAT_RGB565 and then modetest works. > But in the output of modetest -s the blue and red colors are reversed. > > *Any* hints why modetest fails when my driver reports the correct formats? Every driver to date supports XR24. So modetest uses that by default. But this fails with your driver since it's an unsupported format. Something like: modetest -M atmel-lcdc -s 34:800x480-60Hz@XB24 should do the trick. The quirk covers drivers that use AddFB(). However modetest is fancy and uses AddFB2, which takes an explicit format. -ilia
On Fri, Jul 26, 2019 at 11:35 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote: > On Fri, Jul 26, 2019 at 4:36 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > Hi all. > > > > The Atmel at91sam9263 has a nice LCDC IP core that supports several > > formats: > > DRM_FORMAT_XBGR8888, DRM_FORMAT_BGR888, DRM_FORMAT_BGR565 > > > > (It also supports a palletized C8 format, but thats for later). > > > > The formats are all BGR formats - and some boards actually reverse Blue > > and Red when wiring up the display. I have added a DT property to > > identify boards with this difference. > > > > The penguin shown during boot of the kernel had blue feet which is a > > clear sign that RED and GREEN was reversed. > > > > So to fix this I (got help from imirkin on irc) I implmented a quirk. > > See patch below. > > > > With the quirk enabled I see: > > - penguin shown during kernel boot has yellow feets (OK) > > - modetest tell the driver reports: XB24 BG24 BG16 (as expected) > > - modetest -s fails with: > > # modetest -M atmel-lcdc -s 34:800x480-60Hz > > setting mode 800x480-60Hz@XR24 on connectors 34, crtc 32 > > failed to set mode: Function not implemented > > > > Snip from dmesg: > > > > drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_ADDFB2 > > [drm:drm_mode_addfb2] [FB:37] > > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETCRTC > > [drm:drm_mode_setcrtc] [CRTC:32:crtc-0] > > [drm:drm_mode_setcrtc] Invalid pixel format XR24 little-endian (0x34325258), modifier 0x0 > > ^^^^ > > [drm:drm_mode_object_put] OBJ ID: 37 (2) > > [drm:drm_ioctl] pid=208, ret = -22 > > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_DIRTYFB > > [drm:drm_mode_object_put] OBJ ID: 37 (2) > > [drm:drm_ioctl] pid=208, ret = -38 > > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_RMFB > > [drm:drm_mode_object_put] OBJ ID: 37 (2) > > [drm:drm_mode_object_put] OBJ ID: 37 (1) > > [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_DESTROY_DUMB > > [drm:drm_release] open_count = 1 > > [drm:drm_file_free] pid = 208, device = 0xe201, open_count = 1 > > [drm:drm_lastclose] > > [drm:drm_lastclose] driver lastclose completed > > > > Notice that somehow we have a framebuffer in XR24 format, which is not > > supported by the driver. > > > > > > I have tried to tell that my driver supports DRM_FORMAT_XRGB8888, > > DRM_FORMAT_RGB888, DRM_FORMAT_RGB565 and then modetest works. > > But in the output of modetest -s the blue and red colors are reversed. > > > > *Any* hints why modetest fails when my driver reports the correct formats? > > Every driver to date supports XR24. So modetest uses that by default. > But this fails with your driver since it's an unsupported format. > Something like: > > modetest -M atmel-lcdc -s 34:800x480-60Hz@XB24 > > should do the trick. The quirk covers drivers that use AddFB(). > However modetest is fancy and uses AddFB2, which takes an explicit > format. Yeah I thinki for fbdev the correct fix is to look whether the driver enabled atomic and if so, consult the fourcc format list of the primary plane of the first crtc to figure out what you might actually have set. Without atomic you can't realy on the format list being correct since for drivers who get the default primary plane that drm_crtc_init sets up the format list is garbage. Reworking the entire fbdev emulation to use fourcc codes would be even more awesome I guess. -Daniel
Hi all. Thanks for the responses. It turns out this was a PICNIC error (Problem In Chair, Not In Computer) On Fri, Jul 26, 2019 at 10:36:26PM +0200, Sam Ravnborg wrote: > Hi all. > > The Atmel at91sam9263 has a nice LCDC IP core that supports several > formats: > DRM_FORMAT_XBGR8888, DRM_FORMAT_BGR888, DRM_FORMAT_BGR565 > > (It also supports a palletized C8 format, but thats for later). > > The formats are all BGR formats - and some boards actually reverse Blue > and Red when wiring up the display. I have added a DT property to > identify boards with this difference. > > The penguin shown during boot of the kernel had blue feet which is a > clear sign that RED and GREEN was reversed. > > So to fix this I (got help from imirkin on irc) I implmented a quirk. > See patch below. > > With the quirk enabled I see: > - penguin shown during kernel boot has yellow feets (OK) > - modetest tell the driver reports: XB24 BG24 BG16 (as expected) > - modetest -s fails with: > # modetest -M atmel-lcdc -s 34:800x480-60Hz modetest do not like the "Hz" prefix when specifying the refresh rate. When Hz is added it reverts back to XR24. So when I use: # modetest -M atmel-lcdc -s 34:800x480@BG24 Then I see the expected output with the right colors. I also had got the false impression that if no format was specified modetest would use the first format reported from the driver. This was obviously also wrong. So all-in-all - I am quite happy with how things works now. Needs to do more testing, polishing a few patches and then I can send out a v3 of this driver. Sam
Hi Daniel. > > > > > > *Any* hints why modetest fails when my driver reports the correct formats? > > > > Every driver to date supports XR24. So modetest uses that by default. > > But this fails with your driver since it's an unsupported format. > > Something like: > > > > modetest -M atmel-lcdc -s 34:800x480-60Hz@XB24 > > > > should do the trick. The quirk covers drivers that use AddFB(). > > However modetest is fancy and uses AddFB2, which takes an explicit > > format. > > Yeah I thinki for fbdev the correct fix is to look whether the driver > enabled atomic and if so, consult the fourcc format list of the > primary plane of the first crtc to figure out what you might actually > have set. Without atomic you can't realy on the format list being > correct since for drivers who get the default primary plane that > drm_crtc_init sets up the format list is garbage. I do not follow you here. # modetest -M atmel-lcdc -p Planes: id crtc fb CRTC x,y x,y gamma size possible crtcs 31 32 35 0,0 0,0 0 0x00000001 formats: XB24 BG24 BG16 # modetest -M atmel-lcdc -a -p Planes: id crtc fb CRTC x,y x,y gamma size possible crtcs 31 32 35 0,0 0,0 0 0x00000001 formats: XB24 BG24 BG16 So with or without atomic (-a option) the driver reports the same formats. # modetest -M atmel-lcdc -a -s ... does not work btw. It just exists without anything shown on the panel. > Reworking the entire > fbdev emulation to use fourcc codes would be even more awesome I > guess. To take the bait I would need a little bit more guidiance on this. Is it something like: - New variant af drm_fbdev_generic_setup() that takes a fourcc code, not a preferred_bpp - drm_fb_helper_initial_config likewise - drm_fb_helper_fbdev_setup likewise - drm_fb_helper_single_fb_probe likewise And then work through all the code until we no longer rely on preferred_bpp?? Sam
Hi > # modetest -M atmel-lcdc -a -s ... > > does not work btw. It just exists without anything shown on the panel. Looking at the modetest src -a requires both a primary plane + an overlay. If either is missing modetest just exists with no errors printed. Another opportunity to improve error handling. Sam
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index a7ba5b4902d6..adc1b1fcb153 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1236,9 +1236,9 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1, } static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, - u8 depth) + struct drm_framebuffer *fb) { - switch (depth) { + switch (fb->format->depth) { case 8: var->red.offset = 0; var->green.offset = 0; @@ -1260,18 +1260,30 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, var->transp.length = 1; break; case 16: - var->red.offset = 11; - var->green.offset = 5; - var->blue.offset = 0; + if (!fb->dev->mode_config.quirk_addfb_rgb_to_bgr) { + var->red.offset = 11; + var->green.offset = 5; + var->blue.offset = 0; + } else { + var->red.offset = 0; + var->green.offset = 5; + var->blue.offset = 11; + } var->red.length = 5; var->green.length = 6; var->blue.length = 5; var->transp.offset = 0; break; case 24: - var->red.offset = 16; - var->green.offset = 8; - var->blue.offset = 0; + if (!fb->dev->mode_config.quirk_addfb_rgb_to_bgr) { + var->red.offset = 16; + var->green.offset = 8; + var->blue.offset = 0; + } else { + var->red.offset = 0; + var->green.offset = 8; + var->blue.offset = 16; + } var->red.length = 8; var->green.length = 8; var->blue.length = 8; @@ -1279,9 +1291,15 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, var->transp.length = 0; break; case 32: - var->red.offset = 16; - var->green.offset = 8; - var->blue.offset = 0; + if (!fb->dev->mode_config.quirk_addfb_rgb_to_bgr) { + var->red.offset = 16; + var->green.offset = 8; + var->blue.offset = 0; + } else { + var->red.offset = 0; + var->green.offset = 8; + var->blue.offset = 16; + } var->red.length = 8; var->green.length = 8; var->blue.length = 8; @@ -1342,7 +1360,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, !var->blue.length && !var->transp.length && !var->red.msb_right && !var->green.msb_right && !var->blue.msb_right && !var->transp.msb_right) { - drm_fb_helper_fill_pixel_fmt(var, fb->format->depth); + drm_fb_helper_fill_pixel_fmt(var, fb); } /* @@ -1684,7 +1702,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info, info->var.yoffset = 0; info->var.activate = FB_ACTIVATE_NOW; - drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth); + drm_fb_helper_fill_pixel_fmt(&info->var, fb); info->var.xres = fb_width; info->var.yres = fb_height; @@ -2205,7 +2223,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, sizes->surface_width, sizes->surface_height, sizes->surface_bpp); - format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); + format = drm_driver_legacy_fb_format(fb_helper->dev, sizes->surface_bpp, sizes->surface_depth); buffer = drm_client_framebuffer_create(client, sizes->surface_width, sizes->surface_height, format); if (IS_ERR(buffer)) diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index c630064ccf41..972ea746c06d 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -126,6 +126,20 @@ uint32_t drm_driver_legacy_fb_format(struct drm_device *dev, fmt == DRM_FORMAT_XRGB2101010) fmt = DRM_FORMAT_XBGR2101010; + if (dev->mode_config.quirk_addfb_rgb_to_bgr) { + switch (fmt) { + case DRM_FORMAT_RGB565: + fmt = DRM_FORMAT_BGR565; + break; + case DRM_FORMAT_RGB888: + fmt = DRM_FORMAT_BGR888; + break; + case DRM_FORMAT_XRGB8888: + fmt = DRM_FORMAT_XBGR8888; + break; + } + } + return fmt; } EXPORT_SYMBOL(drm_driver_legacy_fb_format); diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index f57eea0481e0..58aa8c02232e 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -881,6 +881,13 @@ struct drm_mode_config { */ bool quirk_addfb_prefer_host_byte_order; + /** + * @quirk_addfb_rgb_to_bgr: + * + * TODO + */ + bool quirk_addfb_rgb_to_bgr; + /** * @async_page_flip: Does this device support async flips on the primary * plane?
Hi all. The Atmel at91sam9263 has a nice LCDC IP core that supports several formats: DRM_FORMAT_XBGR8888, DRM_FORMAT_BGR888, DRM_FORMAT_BGR565 (It also supports a palletized C8 format, but thats for later). The formats are all BGR formats - and some boards actually reverse Blue and Red when wiring up the display. I have added a DT property to identify boards with this difference. The penguin shown during boot of the kernel had blue feet which is a clear sign that RED and GREEN was reversed. So to fix this I (got help from imirkin on irc) I implmented a quirk. See patch below. With the quirk enabled I see: - penguin shown during kernel boot has yellow feets (OK) - modetest tell the driver reports: XB24 BG24 BG16 (as expected) - modetest -s fails with: # modetest -M atmel-lcdc -s 34:800x480-60Hz setting mode 800x480-60Hz@XR24 on connectors 34, crtc 32 failed to set mode: Function not implemented Snip from dmesg: drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_ADDFB2 [drm:drm_mode_addfb2] [FB:37] [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_SETCRTC [drm:drm_mode_setcrtc] [CRTC:32:crtc-0] [drm:drm_mode_setcrtc] Invalid pixel format XR24 little-endian (0x34325258), modifier 0x0 ^^^^ [drm:drm_mode_object_put] OBJ ID: 37 (2) [drm:drm_ioctl] pid=208, ret = -22 [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_DIRTYFB [drm:drm_mode_object_put] OBJ ID: 37 (2) [drm:drm_ioctl] pid=208, ret = -38 [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_RMFB [drm:drm_mode_object_put] OBJ ID: 37 (2) [drm:drm_mode_object_put] OBJ ID: 37 (1) [drm:drm_ioctl] pid=208, dev=0xe201, auth=1, DRM_IOCTL_MODE_DESTROY_DUMB [drm:drm_release] open_count = 1 [drm:drm_file_free] pid = 208, device = 0xe201, open_count = 1 [drm:drm_lastclose] [drm:drm_lastclose] driver lastclose completed Notice that somehow we have a framebuffer in XR24 format, which is not supported by the driver. I have tried to tell that my driver supports DRM_FORMAT_XRGB8888, DRM_FORMAT_RGB888, DRM_FORMAT_RGB565 and then modetest works. But in the output of modetest -s the blue and red colors are reversed. *Any* hints why modetest fails when my driver reports the correct formats? For reference I will post source for the driver in a follow-up mail. Sam The quirk I implmented looks like this (preliminary patch): From ab0f213e50fbe1b053d624745306252b28e6ecdc Mon Sep 17 00:00:00 2001 From: Sam Ravnborg <sam@ravnborg.org> Date: Sun, 21 Jul 2019 08:59:49 +0200 Subject: [PATCH 21/22] drm: add quirk_addfb_rgb_to_bgr Used for drivers where the format supported is default BGR. Then use this quirk to force drm_fb_helper to convert the framebuffer from RGB => BGR. Signed-off-by: Sam Ravnborg <sam@ravnborg.org> --- drivers/gpu/drm/drm_fb_helper.c | 46 +++++++++++++++++++++++---------- drivers/gpu/drm/drm_fourcc.c | 14 ++++++++++ include/drm/drm_mode_config.h | 7 +++++ 3 files changed, 53 insertions(+), 14 deletions(-)