1. Patchew
  2. linux
  3. [PATCH v2 0/3] drm: Allow simpledrm to setup its emulated FB as firmware provided
  4. View patch

[PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()

Javier Martinez Canillas posted 3 patches 2 years, 4 months ago
There is a newer version of this series
[PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Javier Martinez Canillas 2 years, 4 months ago 
The drm_fbdev_generic_setup() function already sets the preferred bits per
pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp
value is zero.

Passing the same value to the function is unnecessary. Let's cleanup that
in the two drivers that do it.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---

(no changes since v1)

 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +-
 drivers/gpu/drm/tiny/cirrus.c                   | 2 +-
 2 files 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 fe4269c5aa0a..ace92459e462 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -349,7 +349,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, 0);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index c8e791840862..ed5a2e14894a 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -601,7 +601,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, 0);
 	return 0;
 }
 
-- 
2.35.1
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Laurent Pinchart 2 years, 4 months ago 
Hi Javier,

Thank you for the patch.

On Mon, May 02, 2022 at 05:38:58PM +0200, Javier Martinez Canillas wrote:
> The drm_fbdev_generic_setup() function already sets the preferred bits per
> pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp
> value is zero.
> 
> Passing the same value to the function is unnecessary. Let's cleanup that
> in the two drivers that do it.

This looks fine, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

but why do we have two different mechanisms to set the preferred depth ?
Could we get all drivers to set dev->mode_config.preferred_depth and
drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
comment in drm_fbdev_generic_setup() that could be related.

> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 2 +-
>  drivers/gpu/drm/tiny/cirrus.c                   | 2 +-
>  2 files 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 fe4269c5aa0a..ace92459e462 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -349,7 +349,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, 0);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index c8e791840862..ed5a2e14894a 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -601,7 +601,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, 0);
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Javier Martinez Canillas 2 years, 4 months ago 
Hello Laurent,

On 5/2/22 18:06, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Mon, May 02, 2022 at 05:38:58PM +0200, Javier Martinez Canillas wrote:
>> The drm_fbdev_generic_setup() function already sets the preferred bits per
>> pixel for the device to dev->mode_config.preferred_depth, if preferred_bpp
>> value is zero.
>>
>> Passing the same value to the function is unnecessary. Let's cleanup that
>> in the two drivers that do it.
> 
> This looks fine, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> but why do we have two different mechanisms to set the preferred depth ?
> Could we get all drivers to set dev->mode_config.preferred_depth and

Yes, that's the plan and the reason why when we were discussing with Thomas
about how to pass this option to the FB helper layer, we agreed on reusing
the @preferred_bpp parameter rather than adding a third parameter to
drm_fbdev_generic_setup(). Since in the future drivers shouldn't pass that
information to the FB helper and just get it from the default mode config.

But doing that would require more auditing to all drivers and it could add
regressions while patches 1/2 and 2/2 in this series shouldn't cause any
behavioral changes.

> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
> comment in drm_fbdev_generic_setup() that could be related.
>

A FIXME makes sense, I'll add that to when posting a v3.
 Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Javier Martinez Canillas 2 years, 4 months ago 
On 5/2/22 18:55, Javier Martinez Canillas wrote:

[snip]

> 
>> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
>> comment in drm_fbdev_generic_setup() that could be related.
>>
> 
> A FIXME makes sense, I'll add that to when posting a v3.

There's actually a FIXME already in drm_fbdev_generic_setup(), so it's
a documented issue [0]:

void drm_fbdev_generic_setup(struct drm_device *dev,
			     unsigned int preferred_bpp)
{
...
	/*
	 * 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.
	 */
	if (!preferred_bpp)
		preferred_bpp = dev->mode_config.preferred_depth;
	if (!preferred_bpp)
		preferred_bpp = 32;
	fb_helper->preferred_bpp = preferred_bpp;
...
}

[0]: https://elixir.bootlin.com/linux/v5.18-rc5/source/drivers/gpu/drm/drm_fb_helper.c#L2553

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Laurent Pinchart 2 years, 4 months ago 
On Mon, May 02, 2022 at 07:15:16PM +0200, Javier Martinez Canillas wrote:
> On 5/2/22 18:55, Javier Martinez Canillas wrote:
> 
> [snip]
> 
> >> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
> >> comment in drm_fbdev_generic_setup() that could be related.
> > 
> > A FIXME makes sense, I'll add that to when posting a v3.
> 
> There's actually a FIXME already in drm_fbdev_generic_setup(), so it's
> a documented issue [0]:

That's what I meant by "there's a FIXME" :-) It doesn't have to be
addressed by this series, but it would be good to fix it.

> void drm_fbdev_generic_setup(struct drm_device *dev,
> 			     unsigned int preferred_bpp)
> {
> ...
> 	/*
> 	 * 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.
> 	 */
> 	if (!preferred_bpp)
> 		preferred_bpp = dev->mode_config.preferred_depth;
> 	if (!preferred_bpp)
> 		preferred_bpp = 32;
> 	fb_helper->preferred_bpp = preferred_bpp;
> ...
> }
> 
> [0]: https://elixir.bootlin.com/linux/v5.18-rc5/source/drivers/gpu/drm/drm_fb_helper.c#L2553

-- 
Regards,

Laurent Pinchart
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Javier Martinez Canillas 2 years, 4 months ago 
On 5/2/22 20:36, Laurent Pinchart wrote:
> On Mon, May 02, 2022 at 07:15:16PM +0200, Javier Martinez Canillas wrote:
>> On 5/2/22 18:55, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>>> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
>>>> comment in drm_fbdev_generic_setup() that could be related.
>>>
>>> A FIXME makes sense, I'll add that to when posting a v3.
>>
>> There's actually a FIXME already in drm_fbdev_generic_setup(), so it's
>> a documented issue [0]:
> 
> That's what I meant by "there's a FIXME" :-) It doesn't have to be
> addressed by this series, but it would be good to fix it.
>

doh, I misread your original email. Yes, it's the same issue as you
said and something that I plan to look at some point as a follow-up.
 
I hope that we could just replace fbcon with a kms/systemd-consoled/foo
user-space implementation before fixing all the stuff in the DRM fbdev
emulation layer :)

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 1/3] drm: Remove superfluous arg when calling to drm_fbdev_generic_setup()
Posted by Laurent Pinchart 2 years, 4 months ago 
On Mon, May 02, 2022 at 09:28:45PM +0200, Javier Martinez Canillas wrote:
> On 5/2/22 20:36, Laurent Pinchart wrote:
> > On Mon, May 02, 2022 at 07:15:16PM +0200, Javier Martinez Canillas wrote:
> >> On 5/2/22 18:55, Javier Martinez Canillas wrote:
> >>
> >> [snip]
> >>
> >>>> drop the depth option to drm_fbdev_generic_setup() ? There's a FIXME
> >>>> comment in drm_fbdev_generic_setup() that could be related.
> >>>
> >>> A FIXME makes sense, I'll add that to when posting a v3.
> >>
> >> There's actually a FIXME already in drm_fbdev_generic_setup(), so it's
> >> a documented issue [0]:
> > 
> > That's what I meant by "there's a FIXME" :-) It doesn't have to be
> > addressed by this series, but it would be good to fix it.
> 
> doh, I misread your original email. Yes, it's the same issue as you
> said and something that I plan to look at some point as a follow-up.
>  
> I hope that we could just replace fbcon with a kms/systemd-consoled/foo
> user-space implementation before fixing all the stuff in the DRM fbdev
> emulation layer :)

If you can do that, I'll provide champagne :-)

-- 
Regards,

Laurent Pinchart

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.