Re: [PATCH 1/7] drm/hisilicon/hibmc: Fix preferred depth and bpp

[Javier Martinez Canillas <javierm@redhat.com>]

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@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 also wonder if instead of using a 24 magic number, TRUE_COLOR_DEPTH constant
macro or XRGB8888_COLOR_DEPTH could be defined?

>  	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@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat