Re: [PATCH 1/7] drm/hisilicon/hibmc: Fix preferred depth and bpp
On 11/18/22 14:18, Thomas Zimmermann wrote: [...] >> >> 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. > I see. > [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 XRGB_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. > Right. That makes more sense indeed. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [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 XRGB as default format to clients. Signed-off-by: Thomas Zimmermann --- 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 XRGB_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 *)_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 XRGB_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 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 OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 1/7] drm/hisilicon/hibmc: Fix preferred depth and bpp
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 XRGB as default format to clients. > > Signed-off-by: Thomas Zimmermann > --- > 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 XRGB_COLOR_DEPTH could be defined? > dev->mode_config.prefer_shadow = 1; > > dev->mode_config.funcs = (void *)_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 XRGB_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 -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
[PATCH 1/7] drm/hisilicon/hibmc: Fix preferred depth and bpp
Set the preferred color depth to 24 bits and the fbdev bpp to 32 bits. This will signal XRGB as default format to clients. Signed-off-by: Thomas Zimmermann --- 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; dev->mode_config.prefer_shadow = 1; dev->mode_config.funcs = (void *)_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); return 0; -- 2.38.1
