Thomas Zimmermann
2022-Nov-18 13:18 UTC
[PATCH 1/7] drm/hisilicon/hibmc: Fix preferred depth and bpp
Hi Javier Am 18.11.22 um 13:52 schrieb Javier Martinez Canillas:> Hello Thomas, > > On 11/16/22 17:09, Thomas Zimmermann wrote: >> Set the preferred color depth to 24 bits and the fbdev bpp to 32 >> bits. This will signal XRGB8888 as default format to clients. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> >> --- >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> index 22053c613644a..0c4aa4d9b0a77 100644 >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> @@ -106,7 +106,7 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv) >> dev->mode_config.max_width = 1920; >> dev->mode_config.max_height = 1200; >> >> - dev->mode_config.preferred_depth = 32; >> + dev->mode_config.preferred_depth = 24; > > In the cover letter you said "color depth is the number of color and alpha bits > that affect image composition" but it should be "only the number of color bits > excluding the alpha bits" a better description right?I was looking at drm_fourcc.c, where alpha formats, such as ARGB888, have alpha included in their depth value. [1] Alpha obviously effects image composition. But meaning of these terms is somewhat fuzzy, as the command-line arguments of video= include a BPP value that is more similar to DRM's depth value. [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fourcc.c#L175> > I also wonder if instead of using a 24 magic number, TRUE_COLOR_DEPTH constant > macro or XRGB8888_COLOR_DEPTH could be defined?Please not. What we should do is to replace the preferred depth and bpp with a single format constant (as 4cc or drm_format_info) that designates a preferred default. From that format constant, the values exported to userspace and fbdev emulation should be retrieved automatically. If anything, I'd add a TODO item to convert the DRM codebase.> >> dev->mode_config.prefer_shadow = 1; >> >> dev->mode_config.funcs = (void *)&hibmc_mode_funcs; >> @@ -340,7 +340,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev, >> goto err_unload; >> } >> >> - drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth); >> + drm_fbdev_generic_setup(dev, 32); >> > > And same here? Maybe TRUE_COLOR_ALPHA_BPP or XRGB8888_BPP? Can't think of a > good name really for this, but just to avoid using a constant number here. > > In any case the patch looks good to me: > > Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>Thanks a lot. Best regards Thomas>-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 N?rnberg, Germany (HRB 36809, AG N?rnberg) Gesch?ftsf?hrer: Ivo Totev -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20221118/61d3658e/attachment.sig>