[PATCH] drm/tilcdc: Set preferred depth

[PATCH] drm/tilcdc: Set preferred depth

[Kevin Hao]

The commit c91acda3a380 ("drm/gem: Check for valid formats") adds a
check for valid pixel formats on drm_gem_fb_create(), but this breaks
the X server on the beaglebone black board.

We have set 'DefaultDepth' to 16 in our xorg.conf. In the X modesetting
driver, the drmmode_get_default_bpp() is used to guess the default
depth/bpp. First it tries to get them via DRM_CAP_DUMB_PREFERRED_DEPTH
ioctl, and if it fail, then try to create a FB with 'depth = 24' and
'bpp = 32' to check whether this depth/dpp is a valid combo. Before the
kernel commit c91acda3a380, the FB always can be created successfully.
This will avoid the bpp to be set to 24 forcibly. But after kernel
commit c91acda3a380, the FB will not be created successfully due to the
check of the valid pixel format. Then the bpp is set to 24, but the
'depth = 16' and 'bpp = 24' combo is not a valid pixel format.

Fix this issue by explicitly setting the preferred_depth in this driver.
With this change, the modesetting driver would choose the correct
depth/bpp combo based on our setting in xorg.conf.

Fixes: c91acda3a380 ("drm/gem: Check for valid formats")
Cc: stable@vger.kernel.org
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index cd5eefa06060..d4bd4ebeff78 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -258,6 +258,7 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
 
 	pm_runtime_put_sync(dev);
 
+	ddev->mode_config.preferred_depth = 16;
 	if (priv->rev == 1) {
 		DBG("Revision 1 LCDC supports only RGB565 format");
 		priv->pixelformats = tilcdc_rev1_formats;
@@ -273,6 +274,7 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
 			priv->num_pixelformats =
 				ARRAY_SIZE(tilcdc_crossed_formats);
 			bpp = 32; /* Choose bpp with RGB support for fbdef */
+			ddev->mode_config.preferred_depth = 24;
 		} else if (0 == strcmp(str, "straight")) {
 			DBG("Configured for straight blue and red wires");
 			priv->pixelformats = tilcdc_straight_formats;
-- 
2.44.0

Re: [PATCH] drm/tilcdc: Set preferred depth

[Frej Drejhammar]

Hi Kevin,

Kevin Hao <haokexin@gmail.com> writes:

> But after kernel commit c91acda3a380, the FB will not be created
> successfully due to the check of the valid pixel format. Then the bpp
> is set to 24, but the 'depth = 16' and 'bpp = 24' combo is not a valid
> pixel format.
>
> Fix this issue by explicitly setting the preferred_depth in this driver.
> With this change, the modesetting driver would choose the correct
> depth/bpp combo based on our setting in xorg.conf.

Check the fix in [1], with it not only does the X-server work with a
color depth of 16 bits, it also enables the use of a 24 bit color depth.

As I'm the author of the solution in [1] I'm partial to it as it is a
more general solution to the regression. I have no standing in this
community as [1] is my first contribution to the DRM system, but if I
had, I would NAK this patch as it only fixes the regression for one
driver and does not enable the use of a 24 bit color depth which is
something the hardware is capable of.

Best regards,

--Frej

[1] https://lore.kernel.org/all/e7ef6d422365986f49746b596735f7a0b939574d.1710698387.git.frej.drejhammar@gmail.com/

Re: [PATCH] drm/tilcdc: Set preferred depth

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On Sun, Mar 17, 2024 at 08:18:57PM +0100, Frej Drejhammar wrote:
> Hi Kevin,
> 
> Kevin Hao <haokexin@gmail.com> writes:
> 
> > But after kernel commit c91acda3a380, the FB will not be created
> > successfully due to the check of the valid pixel format. Then the bpp
> > is set to 24, but the 'depth = 16' and 'bpp = 24' combo is not a valid
> > pixel format.
> >
> > Fix this issue by explicitly setting the preferred_depth in this driver.
> > With this change, the modesetting driver would choose the correct
> > depth/bpp combo based on our setting in xorg.conf.
> 
> Check the fix in [1], with it not only does the X-server work with a
> color depth of 16 bits, it also enables the use of a 24 bit color depth.

Thank you, Frej. I didn't notice your patch before sending mine.

> 
> As I'm the author of the solution in [1] I'm partial to it as it is a
> more general solution to the regression.

Agreed.

>I have no standing in this
> community as [1] is my first contribution to the DRM system, but if I
> had, I would NAK this patch as it only fixes the regression for one
> driver and does not enable the use of a 24 bit color depth which is
> something the hardware is capable of.

I had also thought about a similar modification before, but personally,
I considered such changes a bit aggressive for a patch that needs to be
backported to a stable kernel (especially for a LTS kernel such as v6.6
which I am working on). That's why I opted for minimal changes to fix this
regression, reducing the risk when we backport it to the stable kernel.
Additionally, my patch and your patch don't conflict semantically, and
setting a driver's preferred_depth shouldn't cause any other issues.

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drm/tilcdc: Set preferred depth

[Frej Drejhammar]

Hi, Kevin

Kevin Hao <haokexin@gmail.com> writes:

> I had also thought about a similar modification before, but personally,
> I considered such changes a bit aggressive for a patch that needs to be
> backported to a stable kernel (especially for a LTS kernel such as v6.6
> which I am working on). That's why I opted for minimal changes to fix this
> regression, reducing the risk when we backport it to the stable kernel.

Personally I only work with the latest stable mainline kernel, so I want
to fix both the regression and enable a 24 bit color depth...

> Additionally, my patch and your patch don't conflict semantically, and
> setting a driver's preferred_depth shouldn't cause any other issues.

True, we'll see what the maintainers say.

Regards,

--Frej

Re: [PATCH] drm/tilcdc: Set preferred depth

[sarha]

March 17, 2024 at 9:18 PM, "Frej Drejhammar" <frej.drejhammar@gmail.com mailto:frej.drejhammar@gmail.com?to=%22Frej%20Drejhammar%22%20%3Cfrej.drejhammar%40gmail.com%3E > wrote:
> 
> Hi Kevin,
> 
> Kevin Hao <haokexin@gmail.com> writes:
> 
> > 
> > But after kernel commit c91acda3a380, the FB will not be created
> > successfully due to the check of the valid pixel format. Then the bpp
> > is set to 24, but the 'depth = 16' and 'bpp = 24' combo is not a valid
> > pixel format.
> > 
> > Fix this issue by explicitly setting the preferred_depth in this driver.
> > With this change, the modesetting driver would choose the correct
> > depth/bpp combo based on our setting in xorg.conf.
> > 
> 
> Check the fix in [1], with it not only does the X-server work with a
> color depth of 16 bits, it also enables the use of a 24 bit color depth.
> 
> As I'm the author of the solution in [1] I'm partial to it as it is a
> more general solution to the regression. I have no standing in this
> community as [1] is my first contribution to the DRM system, but if I
> had, I would NAK this patch as it only fixes the regression for one
> driver and does not enable the use of a 24 bit color depth which is
> something the hardware is capable of.
> 
> Best regards,
> 

So the trouble here is the 16bpp and 24/32bpp color wiring being crossed, right? I'd just like to remind that there is another option to overcome the issue by editing device tree files. The current configuration from some 8 years back supports RG16, BG24, and XB24 formats, but on Beaglebone-Black its possible - thanks to tda998x - change the support to BG16, RG24, and XR24, by changing these lines before building a new dtb-file:

Change value of blue-and-red-wiring to "crossed" here:
https://github.com/torvalds/linux/blob/b3603fcb79b1036acae10602bffc4855a4b9af80/arch/arm/boot/dts/ti/omap/am335x-boneblack-hdmi.dtsi#L61

and comment this this line:
https://github.com/torvalds/linux/blob/b3603fcb79b1036acae10602bffc4855a4b9af80/arch/arm/boot/dts/ti/omap/am335x-boneblack-hdmi.dtsi#L82

There is also the option of removing the blue-and-red-wiring property all together if one does not really mind about the colors. In this case the driver reports the formats RG16, RG24, and XR24 being supported, despite the colors being wrong in either 16-bit or 24-bit color modes.

Please let me know If Frej's patch does not make it (I am not in CC and I do not have the bandwidth to follow dri-devel), and I will merge Kevin's patch through drm-misc-next.

Best regards,
Jyri

> --Frej
> 
> [1] https://lore.kernel.org/all/e7ef6d422365986f49746b596735f7a0b939574d.1710698387.git.frej.drejhammar@gmail.com/
>

Re: [PATCH] drm/tilcdc: Set preferred depth

[Frej Drejhammar]

Hi, Jyri

sarha@kapsi.fi writes:

> So the trouble here is the 16bpp and 24/32bpp color wiring being
> crossed, right? I'd just like to remind that there is another option
> to overcome the issue by editing device tree files. The current
> configuration from some 8 years back supports RG16, BG24, and XB24
> formats, but on Beaglebone-Black its possible - thanks to tda998x -
> change the support to BG16, RG24, and XR24, by changing these lines
> before building a new dtb-file:

I don't think that will solve the problem. The unmodified
drm_driver_legacy_fb_format() gives RG16 if it is asked for a 16 bit
depth (and the corresponding bpp in the way that the X server does) and
RG24 if asked for 24 bits. So swapping the bits around will always break
one of the hardware-supported depths unless you can cross/uncross the
wires dynamically when setting the frame buffer.

Best regards,

--Frej