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

[Thomas Zimmermann]

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: <https://lists.freedesktop.org/archives/dri-devel/attachments/20221118/61d3658e/attachment-0001.sig>