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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux