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@xxxxxxx> --- 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@xxxxxxxxxx>
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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
