[PATCH 0/7] drm: Fix the color-depth/bpp confusion

[PATCH 0/7] drm: Fix the color-depth/bpp confusion

[Thomas Zimmermann]

A number of drivers mix up the meaning of bits-per-pixel and color
depth. For each of them, set the correct values. As a rule of thumb,
the color depth is the number of color and alpha bits that affect
image composition. The bpp value is the color depth in the pixel
plus the filler bits.

The color depth is exported to userspace, while the bpp value only
affects fbdev emulation. Currently, fbdev fails if it selects a color
format that is unsupported by the driver. The fix would be to fall
back to a driver default value for the bpp. Getting the default fixed
in drivers will then allow us to fix the fbdev format selection.

Thomas Zimmermann (7):
  drm/hisilicon/hibmc: Fix preferred depth and bpp
  drm/logicvc: Fix preferred fbdev cpp
  drm/cirrus: Decouple fbdev bpp value from color depth
  drm/ofdrm: Set preferred depth from format of scanout buffer
  drm/simpledrm: Set preferred depth from format of scanout buffer
  drm/solomon: Set preferred color depth and bpp to the correct values
  drm/fb-helper: Don't use the preferred depth for the BPP default

 drivers/gpu/drm/drm_fbdev_generic.c             | 15 +++++++++------
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c |  4 ++--
 drivers/gpu/drm/logicvc/logicvc_drm.c           | 14 +++++++++++++-
 drivers/gpu/drm/solomon/ssd130x.c               |  4 ++--
 drivers/gpu/drm/tiny/cirrus.c                   |  2 +-
 drivers/gpu/drm/tiny/ofdrm.c                    | 13 +------------
 drivers/gpu/drm/tiny/simpledrm.c                |  4 ++--
 7 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Thomas Zimmermann]

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;
 	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);
 
 	return 0;
 
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/7] drm/logicvc: Fix preferred fbdev cpp

[Thomas Zimmermann]

Logicvc can have different values for the preferred color depth. Set
the fbdev bpp value depending on the runtime value.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/logicvc/logicvc_drm.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
index 9de24d9f0c963..d9cd5d967e31f 100644
--- a/drivers/gpu/drm/logicvc/logicvc_drm.c
+++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
@@ -301,6 +301,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 	struct regmap *regmap = NULL;
 	struct resource res;
 	void __iomem *base;
+	unsigned int preferred_bpp;
 	int irq;
 	int ret;
 
@@ -438,7 +439,18 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 		goto error_mode;
 	}
 
-	drm_fbdev_generic_setup(drm_dev, drm_dev->mode_config.preferred_depth);
+	switch (drm_dev->mode_config.preferred_depth) {
+	case 15:
+	case 16:
+		preferred_bpp = 16;
+		break;
+	case 24:
+	case 32:
+	default:
+		preferred_bpp = 32;
+		break;
+	}
+	drm_fbdev_generic_setup(drm_dev, preferred_bpp);
 
 	return 0;
 
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 3/7] drm/cirrus: Decouple fbdev bpp value from color depth

[Thomas Zimmermann]

Cirrus has a preferred color depth of 16 bit; also use it as fbdev
bpp value. Don't use the color depth directly. It has a different
meaning than bpp and both cannot be used interchangeably.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/cirrus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index 678c2ef1cae70..cf35b60905032 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -604,7 +604,7 @@ static int cirrus_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
-	drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth);
+	drm_fbdev_generic_setup(dev, 16);
 	return 0;
 }
 
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 4/7] drm/ofdrm: Set preferred depth from format of scanout buffer

[Thomas Zimmermann]

Set the preferred depth from the format of the scanout buffer. The
value cannot be hardcoded, as the scanout buffer's is only known at
runtime. Keeping the existing switch statement just duplicates the
driver's existing logic for format detection.

Also remove the FIXME comment from the call to drm_fbdev_generic_setup()
as the driver now handles color depth and bpp values correctly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/ofdrm.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index dc9e4d71b12ae..33eefeba437c5 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -1284,14 +1284,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 	dev->mode_config.min_height = height;
 	dev->mode_config.max_height = max_height;
 	dev->mode_config.funcs = &ofdrm_mode_config_funcs;
-	switch (depth) {
-	case 32:
-		dev->mode_config.preferred_depth = 24;
-		break;
-	default:
-		dev->mode_config.preferred_depth = depth;
-		break;
-	}
+	dev->mode_config.preferred_depth = format->depth;
 	dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
 
 	/* Primary plane */
@@ -1390,10 +1383,6 @@ static int ofdrm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/*
-	 * FIXME: 24-bit color depth does not work reliably with a 32-bpp
-	 * value. Force the bpp value of the scanout buffer's format.
-	 */
 	drm_fbdev_generic_setup(dev, drm_format_info_bpp(odev->format, 0));
 
 	return 0;
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 5/7] drm/simpledrm: Set preferred depth from format of scanout buffer

[Thomas Zimmermann]

Set the preferred depth from the format of the scanout buffer. The
value cannot be hardcoded, as the scanout buffer's is only known at
runtime. Also derive the fbdev emulations bpp value from the scanout
format.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 162eb44dcba89..30e928d627e8f 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -739,7 +739,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	dev->mode_config.max_width = max_width;
 	dev->mode_config.min_height = height;
 	dev->mode_config.max_height = max_height;
-	dev->mode_config.preferred_depth = format->cpp[0] * 8;
+	dev->mode_config.preferred_depth = format->depth;
 	dev->mode_config.funcs = &simpledrm_mode_config_funcs;
 
 	/* Primary plane */
@@ -834,7 +834,7 @@ static int simpledrm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	drm_fbdev_generic_setup(dev, 0);
+	drm_fbdev_generic_setup(dev, drm_format_info_bpp(sdev->format, 0));
 
 	return 0;
 }
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 6/7] drm/solomon: Set preferred color depth and bpp to the correct values

[Thomas Zimmermann]

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/solomon/ssd130x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 53464afc2b9ac..c3bf3a18302ea 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -876,7 +876,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
 	drm->mode_config.max_width = max_width;
 	drm->mode_config.min_height = mode->vdisplay;
 	drm->mode_config.max_height = max_height;
-	drm->mode_config.preferred_depth = 32;
+	drm->mode_config.preferred_depth = 24;
 	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
 
 	/* Primary plane */
@@ -1006,7 +1006,7 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return ERR_PTR(dev_err_probe(dev, ret, "DRM device register failed\n"));
 
-	drm_fbdev_generic_setup(drm, 0);
+	drm_fbdev_generic_setup(drm, 32);
 
 	return ssd130x;
 }
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 7/7] drm/fb-helper: Don't use the preferred depth for the BPP default

[Thomas Zimmermann]

If no preferred value for bits-per-pixel has been given, fall back
to 32. Never use the preferred depth. The color depth is the number
of color/alpha bits per pixel, while bpp is the overall number of
bits in most cases.

Most noteworthy, XRGB8888 has a depth of 24 and a bpp value of 32.
Using depth for bpp would make the value 24 as well and format
selection in fbdev helpers fails. Unfortunately XRGB8888 is the most
common format and the old heuristic therefore fails for most of
the drivers (unless they implement the 24-bit RGB888 format).

Picking a bpp of 32 will lateron result in a default depth of 24
and the format XRGB8888. As XRGB8888 is the default format for most
of the current and legacy graphics stack, all drivers must support
it. So it is the safe choice.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbdev_generic.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index ab86956692795..0a4c160e0e58a 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -431,7 +431,6 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  * drm_fbdev_generic_setup() - Setup generic fbdev emulation
  * @dev: DRM device
  * @preferred_bpp: Preferred bits per pixel for the device.
- *                 @dev->mode_config.preferred_depth is used if this is zero.
  *
  * This function sets up generic fbdev emulation for drivers that supports
  * dumb buffers with a virtual address and that can be mmap'ed.
@@ -475,12 +474,16 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
 	}
 
 	/*
-	 * FIXME: This mixes up depth with bpp, which results in a glorious
-	 * mess, resulting in some drivers picking wrong fbdev defaults and
-	 * others wrong preferred_depth defaults.
+	 * Pick a preferred bpp of 32 if no value has been given. This
+	 * will select XRGB8888 for the framebuffer formats. All drivers
+	 * have to support XRGB8888 for backwards compatibility with legacy
+	 * userspace, so it's the safe choice here.
+	 *
+	 * TODO: Replace struct drm_mode_config.preferred_depth and this
+	 *       bpp value with a preferred format that is given as struct
+	 *       drm_format_info. Then derive all other values from the
+	 *       format.
 	 */
-	if (!preferred_bpp)
-		preferred_bpp = dev->mode_config.preferred_depth;
 	if (!preferred_bpp)
 		preferred_bpp = 32;
 	fb_helper->preferred_bpp = preferred_bpp;
-- 
2.38.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2992 bytes --]

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@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@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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/7] drm/logicvc: Fix preferred fbdev cpp

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2810 bytes --]

Hi

Am 18.11.22 um 14:08 schrieb Javier Martinez Canillas:
> On 11/16/22 17:09, Thomas Zimmermann wrote:
>> Logicvc can have different values for the preferred color depth. Set
>> the fbdev bpp value depending on the runtime value.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/logicvc/logicvc_drm.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
>> index 9de24d9f0c963..d9cd5d967e31f 100644
>> --- a/drivers/gpu/drm/logicvc/logicvc_drm.c
>> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
>> @@ -301,6 +301,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
>>   	struct regmap *regmap = NULL;
>>   	struct resource res;
>>   	void __iomem *base;
>> +	unsigned int preferred_bpp;
>>   	int irq;
>>   	int ret;
>>   
>> @@ -438,7 +439,18 @@ static int logicvc_drm_probe(struct platform_device *pdev)
>>   		goto error_mode;
>>   	}
>>   
>> -	drm_fbdev_generic_setup(drm_dev, drm_dev->mode_config.preferred_depth);
>> +	switch (drm_dev->mode_config.preferred_depth) {
>> +	case 15:
> 
> Why could have 15? IIUC the formats supported by this driver are:
> 
> static uint32_t logicvc_layer_formats_rgb16[] = {
> 	DRM_FORMAT_RGB565,
> 	DRM_FORMAT_BGR565,
> 	DRM_FORMAT_INVALID,
> };
> 
> static uint32_t logicvc_layer_formats_rgb24[] = {
> 	DRM_FORMAT_XRGB8888,
> 	DRM_FORMAT_XBGR8888,
> 	DRM_FORMAT_INVALID,
> };
> 
> /*
>   * What we call depth in this driver only counts color components, not alpha.
>   * This allows us to stay compatible with the LogiCVC bistream definitions.
>   */
> static uint32_t logicvc_layer_formats_rgb24_alpha[] = {
> 	DRM_FORMAT_ARGB8888,
> 	DRM_FORMAT_ABGR8888,
> 	DRM_FORMAT_INVALID,
> };
> 
> So shouldn't be just 16, 24 and 32 ?

That makes sense.

> 
>> +	case 16:
>> +		preferred_bpp = 16;
>> +		break;
>> +	case 24:
>> +	case 32:
>> +	default:
>> +		preferred_bpp = 32;
>> +		break;
> 
> I'm also not sure if this is needed. Since IIUC in logicvc_mode_init() the
> driver does:
> 
> 	preferred_depth = layer_primary->formats->depth;
> 
> 	/* DRM counts alpha in depth, our driver doesn't. */
> 	if (layer_primary->formats->alpha)
> 		preferred_depth += 8;
> 
> 	...
> 	mode_config->preferred_depth = preferred_depth;
> 
> So it seems this patch is not needed? Unless I'm misunderstanding the code.

The driver uses XRGB8888, so the 24-bit color depth has a 32-bit bpp 
value. Hence the switch.

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/7] drm/logicvc: Fix preferred fbdev cpp

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1495 bytes --]

Hi

Am 18.11.22 um 14:41 schrieb Javier Martinez Canillas:
> On 11/18/22 14:22, Thomas Zimmermann wrote:
> 
> [...]
> 
>>>
>>> I'm also not sure if this is needed. Since IIUC in logicvc_mode_init() the
>>> driver does:
>>>
>>> 	preferred_depth = layer_primary->formats->depth;
>>>
>>> 	/* DRM counts alpha in depth, our driver doesn't. */
>>> 	if (layer_primary->formats->alpha)
>>> 		preferred_depth += 8;
>>>
>>> 	...
>>> 	mode_config->preferred_depth = preferred_depth;
>>>
>>> So it seems this patch is not needed? Unless I'm misunderstanding the code.
>>
>> The driver uses XRGB8888, so the 24-bit color depth has a 32-bit bpp
>> value. Hence the switch.
>>
> 
> Ah, I see. So is to set 32-bit bpp for both XRGB8888 and ARGB8888. But then
> I think that you also need to fix logicvc_mode_init() to remove that += 8?
> 
> Because right now the preferred_depth += 8 would set a preferred_depth of 32
> when should be just 24 even if alpha is enabled?
> 
> Or am I confusing again the meaning of the color depth?

For DRM, it's defined in drm_fourcc.c. ARGB8888 has a depth of 32 and 
XRGB8888 has a depth of 24. Both have a bpp of 32.

BUT in logicvc's internal data structure, both formats have a color 
depth of 24 and a bpp of 32.

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 7/7] drm/fb-helper: Don't use the preferred depth for the BPP default

[Daniel Vetter]

On Wed, Nov 16, 2022 at 05:09:17PM +0100, Thomas Zimmermann wrote:
> If no preferred value for bits-per-pixel has been given, fall back
> to 32. Never use the preferred depth. The color depth is the number
> of color/alpha bits per pixel, while bpp is the overall number of
> bits in most cases.
> 
> Most noteworthy, XRGB8888 has a depth of 24 and a bpp value of 32.
> Using depth for bpp would make the value 24 as well and format
> selection in fbdev helpers fails. Unfortunately XRGB8888 is the most
> common format and the old heuristic therefore fails for most of
> the drivers (unless they implement the 24-bit RGB888 format).
> 
> Picking a bpp of 32 will lateron result in a default depth of 24
> and the format XRGB8888. As XRGB8888 is the default format for most
> of the current and legacy graphics stack, all drivers must support
> it. So it is the safe choice.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fbdev_generic.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index ab86956692795..0a4c160e0e58a 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -431,7 +431,6 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   * drm_fbdev_generic_setup() - Setup generic fbdev emulation
>   * @dev: DRM device
>   * @preferred_bpp: Preferred bits per pixel for the device.
> - *                 @dev->mode_config.preferred_depth is used if this is zero.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
>   * dumb buffers with a virtual address and that can be mmap'ed.
> @@ -475,12 +474,16 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
>  	}
>  
>  	/*
> -	 * FIXME: This mixes up depth with bpp, which results in a glorious
> -	 * mess, resulting in some drivers picking wrong fbdev defaults and
> -	 * others wrong preferred_depth defaults.
> +	 * Pick a preferred bpp of 32 if no value has been given. This
> +	 * will select XRGB8888 for the framebuffer formats. All drivers
> +	 * have to support XRGB8888 for backwards compatibility with legacy
> +	 * userspace, so it's the safe choice here.
> +	 *
> +	 * TODO: Replace struct drm_mode_config.preferred_depth and this
> +	 *       bpp value with a preferred format that is given as struct
> +	 *       drm_format_info. Then derive all other values from the
> +	 *       format.

I concur on this being the right design. What I've attempted years ago,
but never managed to finish, is sort the formats list on the primary plane
in preference order (since that seems useful for other reasons), and then
let everyone derive the preferred_whatever from the first format of the
first primary plane automatically.

But doing that is a pretty huge refactor, since you get to audit every
driver. So I kinda gave up on that. But I still thing something in that
direction would be a good design overall, since then userspace could also
use the same trick to infer format preferences ...

Anyway on the series, since it pushes in a direction I wanted to fix years
ago but gave up because too ambitious :-)

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	 */
> -	if (!preferred_bpp)
> -		preferred_bpp = dev->mode_config.preferred_depth;
>  	if (!preferred_bpp)
>  		preferred_bpp = 32;
>  	fb_helper->preferred_bpp = preferred_bpp;
> -- 
> 2.38.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization