[PATCH] drm/pl111: Fix FB depth on IMPD-1 framebuffer
Thomas Zimmermann
tzimmermann at suse.de
Mon May 15 09:47:03 UTC 2023
Hi
Am 15.05.23 um 11:29 schrieb Linus Walleij:
> The last argument to the function drm_fbdev_dma_setup() was
> changed from desired BPP to desired depth.
>
> In our case the desired depth was 15 but BPP was 16, so we
> specified 16 as BPP and we relied on the FB emulation core to
> select a format with a suitable depth for the limited bandwidth
> and end up with e.g. XRGB1555 like in the past:
>
> [drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
> drm-clcd-pl111 c1000000.display: [drm] requested bpp 16, scaled depth down to 15
> drm-clcd-pl111 c1000000.display: enable IM-PD1 CLCD connectors
> Console: switching to colour frame buffer device 80x30
> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
>
> However the current code will fail at that:
>
> [drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
> drm-clcd-pl111 c1000000.display: [drm] bpp/depth value of 16/16 not supported
> drm-clcd-pl111 c1000000.display: [drm] No compatible format found
> drm-clcd-pl111 c1000000.display: [drm] *ERROR* fbdev: Failed to setup generic emulation (ret=-12)
>
> Fix this by passing the desired depth of 15 for the IM/PD-1 display
> instead of 16 to drm_fbdev_dma_setup().
>
> The desired depth is however in turn used for bandwidth limiting
> calculations and that was done with a simple / integer division,
> whereas we now have to modify that to use DIV_ROUND_UP() so that
> we get DIV_ROUND_UP(15, 2) = 2 not 15/2 = 1.
>
> After this the display works again on the Integrator/AP IM/PD-1.
>
> Cc: Emma Anholt <emma at anholt.net>
> Cc: stable at vger.kernel.org
> Suggested-by: Thomas Zimmermann <tzimmermann at suse.de>
> Fixes: 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format selection")
> Link: https://lore.kernel.org/dri-devel/20230102112927.26565-1-tzimmermann@suse.de/
> Signed-off-by: Linus Walleij <linus.walleij at linaro.org>
Reviewed-by: Thomas Zimmermann <tzimmermann at suse.de>
The only issue is the use of fb_depth here. Depth in DRM is the number
of color bits in a pixel. fb_depth is not correct if the value is 32.
32-bit color has only 24 color bits. It's your choice, but I'd rename
the field to color_mode. I expect that this will be the name we'll
eventually adopt throughout the code base.
Best regards
Thomas
> ---
> drivers/gpu/drm/pl111/pl111_display.c | 2 +-
> drivers/gpu/drm/pl111/pl111_drm.h | 4 ++--
> drivers/gpu/drm/pl111/pl111_drv.c | 8 ++++----
> drivers/gpu/drm/pl111/pl111_versatile.c | 10 +++++-----
> 4 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 6afdf260a4e2..b9fe926a49e8 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -53,7 +53,7 @@ pl111_mode_valid(struct drm_simple_display_pipe *pipe,
> {
> struct drm_device *drm = pipe->crtc.dev;
> struct pl111_drm_dev_private *priv = drm->dev_private;
> - u32 cpp = priv->variant->fb_bpp / 8;
> + u32 cpp = DIV_ROUND_UP(priv->variant->fb_depth, 8);
> u64 bw;
>
> /*
> diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
> index 2a46b5bd8576..d1fe756444ee 100644
> --- a/drivers/gpu/drm/pl111/pl111_drm.h
> +++ b/drivers/gpu/drm/pl111/pl111_drm.h
> @@ -114,7 +114,7 @@ struct drm_minor;
> * extensions to the control register
> * @formats: array of supported pixel formats on this variant
> * @nformats: the length of the array of supported pixel formats
> - * @fb_bpp: desired bits per pixel on the default framebuffer
> + * @fb_depth: desired depth per pixel on the default framebuffer
> */
> struct pl111_variant_data {
> const char *name;
> @@ -126,7 +126,7 @@ struct pl111_variant_data {
> bool st_bitmux_control;
> const u32 *formats;
> unsigned int nformats;
> - unsigned int fb_bpp;
> + unsigned int fb_depth;
> };
>
> struct pl111_drm_dev_private {
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index 4b2a9e9753f6..43049c8028b2 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -308,7 +308,7 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
> if (ret < 0)
> goto dev_put;
>
> - drm_fbdev_dma_setup(drm, priv->variant->fb_bpp);
> + drm_fbdev_dma_setup(drm, priv->variant->fb_depth);
>
> return 0;
>
> @@ -351,7 +351,7 @@ static const struct pl111_variant_data pl110_variant = {
> .is_pl110 = true,
> .formats = pl110_pixel_formats,
> .nformats = ARRAY_SIZE(pl110_pixel_formats),
> - .fb_bpp = 16,
> + .fb_depth = 16,
> };
>
> /* RealView, Versatile Express etc use this modern variant */
> @@ -376,7 +376,7 @@ static const struct pl111_variant_data pl111_variant = {
> .name = "PL111",
> .formats = pl111_pixel_formats,
> .nformats = ARRAY_SIZE(pl111_pixel_formats),
> - .fb_bpp = 32,
> + .fb_depth = 32,
> };
>
> static const u32 pl110_nomadik_pixel_formats[] = {
> @@ -405,7 +405,7 @@ static const struct pl111_variant_data pl110_nomadik_variant = {
> .is_lcdc = true,
> .st_bitmux_control = true,
> .broken_vblank = true,
> - .fb_bpp = 16,
> + .fb_depth = 16,
> };
>
> static const struct amba_id pl111_id_table[] = {
> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 1b436b75fd39..00c3ebd32359 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -316,7 +316,7 @@ static const struct pl111_variant_data pl110_integrator = {
> .broken_vblank = true,
> .formats = pl110_integrator_pixel_formats,
> .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
> - .fb_bpp = 16,
> + .fb_depth = 16,
> };
>
> /*
> @@ -330,7 +330,7 @@ static const struct pl111_variant_data pl110_impd1 = {
> .broken_vblank = true,
> .formats = pl110_integrator_pixel_formats,
> .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
> - .fb_bpp = 16,
> + .fb_depth = 15,
> };
>
> /*
> @@ -343,7 +343,7 @@ static const struct pl111_variant_data pl110_versatile = {
> .external_bgr = true,
> .formats = pl110_versatile_pixel_formats,
> .nformats = ARRAY_SIZE(pl110_versatile_pixel_formats),
> - .fb_bpp = 16,
> + .fb_depth = 16,
> };
>
> /*
> @@ -355,7 +355,7 @@ static const struct pl111_variant_data pl111_realview = {
> .name = "PL111 RealView",
> .formats = pl111_realview_pixel_formats,
> .nformats = ARRAY_SIZE(pl111_realview_pixel_formats),
> - .fb_bpp = 16,
> + .fb_depth = 16,
> };
>
> /*
> @@ -367,7 +367,7 @@ static const struct pl111_variant_data pl111_vexpress = {
> .name = "PL111 Versatile Express",
> .formats = pl111_realview_pixel_formats,
> .nformats = ARRAY_SIZE(pl111_realview_pixel_formats),
> - .fb_bpp = 16,
> + .fb_depth = 16,
> .broken_clockdivider = true,
> };
>
--
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230515/221f9ef8/attachment.sig>
More information about the dri-devel
mailing list