Hi Am 15.05.23 um 10:16 schrieb Daniel Vetter:
On Fri, May 12, 2023 at 04:11:48PM +0200, Thomas Zimmermann wrote:Hi Am 12.05.23 um 15:20 schrieb Linus Walleij:Sorry for late regression detection but this patch regresses the Integrator AB IMPD-1 graphics, I bisected down to this patch.[...]This is the driver: drivers/gpu/drm/pl111/pl111_versatile.c with the pl110_impd1 variant, so these are the supported modes: /* PL110 pixel formats for Integrator, vanilla PL110 */ static const u32 pl110_integrator_pixel_formats[] = { DRM_FORMAT_ABGR8888, DRM_FORMAT_XBGR8888, DRM_FORMAT_ARGB8888, DRM_FORMAT_XRGB8888, DRM_FORMAT_ABGR1555, DRM_FORMAT_XBGR1555, DRM_FORMAT_ARGB1555, DRM_FORMAT_XRGB1555, }; (...) /* * The IM-PD1 variant is a PL110 with a bunch of broken, or not * yet implemented features */ static const struct pl111_variant_data pl110_impd1 = { .name = "PL110 IM-PD1", .is_pl110 = true, .broken_clockdivider = true, .broken_vblank = true, .formats = pl110_integrator_pixel_formats, .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats), .fb_bpp = 16, }; Notice the absence of RGB565! Then we initialized the frambuffer like this: drm_fbdev_dma_setup(drm, priv->variant->fb_bpp); And as you see priv->variant->fb_bpp will be 16, so we want some 16bpp mode however the only supported depth is 15 (the 1555 modes) so it would use that by scaling back depth to 15. However after this patch that doesn't work anymore. Any hints on how we can fix this?According to a quick grep for fb_bpp, it's only used for the call to drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for the models without rgb565. The switch at [1] should then pick the correct values. The preferred_bpp parameter had a change in semantics. It used to be the number of bits per pixel. But that makes it hard to select between RGB1555 and RGB565. So it's now a special color-mode value that works like the kernel's video= parameter. Values of 15 and 32 are different from the rest. That switch at [1] explains it. Maybe you should rename fb_bpp to color_mode for clarity. Let me know if this helps.Shouldn't the helpers try to do this automatically? I think they kinda did that in the past in some limited circumstances like this ...
That was the intention, but it never really worked at all. IIRC the color-format selection mixed up depth and bpp values freely. Factor in the command-line override (video=@bpp) and some odd case has always been broken.
So fbdev emulation now mostly uses the color-mode value that works as on the command line. The current semantics is:
* select the user-given color mode, if non-zero * select the driver-given color mode, if non-zero * otherwise select a color mode of 32 by default (that's XRGB8888) And later during fb_probe: * try the selected color mode * otherwise try to auto-detect if the selected color mode doesn't work, * otherwise use XRGB8888 as a last resortThat nicely splits the code into color-mode selection and color-format selection. And all the public interfaces (command line, drm_fbdev_{}_setup(), etc) use the same semantics, which is the color mode.
Best regards Thomas
-DanielBest regards Thomas [1] https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L1827Yours, Linus Walleij-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
