[PATCH] drm/tegra: Remove existing framebuffer only if we support display

[PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Tegra DRM doesn't support display on Tegra234 and later, so make sure
not to remove any existing framebuffers in that case.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index b1e1a78e30c6..7a38dadbc264 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device *dev)
 
 	drm_mode_config_reset(drm);
 
-	err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
-	if (err < 0)
-		goto hub;
+	if (drm->mode_config.num_crtc > 0) {
+		err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
+		if (err < 0)
+			goto hub;
+	}
 
 	err = drm_dev_register(drm, 0);
 	if (err < 0)
-- 
2.41.0

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Javier Martinez Canillas]

Thierry Reding <thierry.reding@gmail.com> writes:

Hello Thierry,

> From: Thierry Reding <treding@nvidia.com>
>
> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> not to remove any existing framebuffers in that case.
>

I see, this makes sense to me.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

A couple of comments below though:

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index b1e1a78e30c6..7a38dadbc264 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device *dev)
>  
>  	drm_mode_config_reset(drm);
>  
> -	err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> -	if (err < 0)
> -		goto hub;
> +	if (drm->mode_config.num_crtc > 0) {

Maybe you can add a comment here explaining why the check is needed?

I also wonder if is worth to move the drm_num_crtcs() function from
drivers/gpu/drm/drm_crtc.c to include/drm/drm_crtc.h and use that helper
instead?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1374 bytes --]

Hi

Am 25.08.23 um 15:22 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> not to remove any existing framebuffers in that case.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/gpu/drm/tegra/drm.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index b1e1a78e30c6..7a38dadbc264 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device *dev)
>   
>   	drm_mode_config_reset(drm);
>   
> -	err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> -	if (err < 0)
> -		goto hub;
> +	if (drm->mode_config.num_crtc > 0) {

If you don't support the hardware, wouldn't it be better to return 
-ENODEV if !num_crtc?

Best regards
Thomas

> +		err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> +		if (err < 0)
> +			goto hub;
> +	}
>   
>   	err = drm_dev_register(drm, 0);
>   	if (err < 0)

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Mikko Perttunen]

On 8/30/23 13:19, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.08.23 um 15:22 schrieb Thierry Reding:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Tegra DRM doesn't support display on Tegra234 and later, so make sure
>> not to remove any existing framebuffers in that case.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>>   drivers/gpu/drm/tegra/drm.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index b1e1a78e30c6..7a38dadbc264 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct 
>> host1x_device *dev)
>>       drm_mode_config_reset(drm);
>> -    err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
>> -    if (err < 0)
>> -        goto hub;
>> +    if (drm->mode_config.num_crtc > 0) {
> 
> If you don't support the hardware, wouldn't it be better to return 
> -ENODEV if !num_crtc?

While display is not supported through TegraDRM on Tegra234+, certain 
multimedia accelerators are supported, so we need to finish probe for those.

Cheers,
Mikko

> 
> Best regards
> Thomas
> 
>> +        err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
>> +        if (err < 0)
>> +            goto hub;
>> +    }
>>       err = drm_dev_register(drm, 0);
>>       if (err < 0)
> 

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Daniel Vetter]

On Thu, 31 Aug 2023 at 08:33, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>
> On 8/30/23 13:19, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 25.08.23 um 15:22 schrieb Thierry Reding:
> >> From: Thierry Reding <treding@nvidia.com>
> >>
> >> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> >> not to remove any existing framebuffers in that case.
> >>
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >> ---
> >>   drivers/gpu/drm/tegra/drm.c | 8 +++++---
> >>   1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> >> index b1e1a78e30c6..7a38dadbc264 100644
> >> --- a/drivers/gpu/drm/tegra/drm.c
> >> +++ b/drivers/gpu/drm/tegra/drm.c
> >> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct
> >> host1x_device *dev)
> >>       drm_mode_config_reset(drm);
> >> -    err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> >> -    if (err < 0)
> >> -        goto hub;
> >> +    if (drm->mode_config.num_crtc > 0) {
> >
> > If you don't support the hardware, wouldn't it be better to return
> > -ENODEV if !num_crtc?
>
> While display is not supported through TegraDRM on Tegra234+, certain
> multimedia accelerators are supported, so we need to finish probe for those.

Ideally you also register the tegra driver without DRIVER_MODESET |
DRIVER_ATOMIC in that case, to avoid unecessary userspace confusion.
Most userspace can cope with a display driver without any crtc, but I
think xorg-modesettting actually falls over. Or at least I've seen
some hacks that the agx people added to make sure X doesn't
accidentally open the wrong driver.
-Sima

>
> Cheers,
> Mikko
>
> >
> > Best regards
> > Thomas
> >
> >> +        err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> >> +        if (err < 0)
> >> +            goto hub;
> >> +    }
> >>       err = drm_dev_register(drm, 0);
> >>       if (err < 0)
> >
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Thierry Reding]

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

On Wed, Aug 30, 2023 at 08:13:04AM +0200, Javier Martinez Canillas wrote:
> Thierry Reding <thierry.reding@gmail.com> writes:
> 
> Hello Thierry,
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > not to remove any existing framebuffers in that case.
> >
> 
> I see, this makes sense to me.
> 
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> A couple of comments below though:
> 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/tegra/drm.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index b1e1a78e30c6..7a38dadbc264 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct host1x_device *dev)
> >  
> >  	drm_mode_config_reset(drm);
> >  
> > -	err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> > -	if (err < 0)
> > -		goto hub;
> > +	if (drm->mode_config.num_crtc > 0) {
> 
> Maybe you can add a comment here explaining why the check is needed?

Sure, will do.

> I also wonder if is worth to move the drm_num_crtcs() function from
> drivers/gpu/drm/drm_crtc.c to include/drm/drm_crtc.h and use that helper
> instead?

I've been looking at this, there's a few things that come to mind. It
seems like we have a couple of different ways to get the number of CRTCs
for a device. We have struct drm_device's num_crtcs, which is set during
drm_vblank_init(), then we have struct drm_mode_config's num_crtc, which
is incremented every time a new CRTC is added (and decremented when a
CRTC is removed), and finally we've got the drm_num_crtcs() which
"computes" the number of CRTCs registered by iterating over all CRTCs
that have been registered.

Are there any cases where these three can yield different values? Would
it not make sense to consolidate these into a single variable?

Thierry

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

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Thierry Reding]

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

On Thu, Aug 31, 2023 at 10:04:29AM +0200, Daniel Vetter wrote:
> On Thu, 31 Aug 2023 at 08:33, Mikko Perttunen <cyndis@kapsi.fi> wrote:
> >
> > On 8/30/23 13:19, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 25.08.23 um 15:22 schrieb Thierry Reding:
> > >> From: Thierry Reding <treding@nvidia.com>
> > >>
> > >> Tegra DRM doesn't support display on Tegra234 and later, so make sure
> > >> not to remove any existing framebuffers in that case.
> > >>
> > >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> > >> ---
> > >>   drivers/gpu/drm/tegra/drm.c | 8 +++++---
> > >>   1 file changed, 5 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > >> index b1e1a78e30c6..7a38dadbc264 100644
> > >> --- a/drivers/gpu/drm/tegra/drm.c
> > >> +++ b/drivers/gpu/drm/tegra/drm.c
> > >> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct
> > >> host1x_device *dev)
> > >>       drm_mode_config_reset(drm);
> > >> -    err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> > >> -    if (err < 0)
> > >> -        goto hub;
> > >> +    if (drm->mode_config.num_crtc > 0) {
> > >
> > > If you don't support the hardware, wouldn't it be better to return
> > > -ENODEV if !num_crtc?
> >
> > While display is not supported through TegraDRM on Tegra234+, certain
> > multimedia accelerators are supported, so we need to finish probe for those.
> 
> Ideally you also register the tegra driver without DRIVER_MODESET |
> DRIVER_ATOMIC in that case, to avoid unecessary userspace confusion.
> Most userspace can cope with a display driver without any crtc, but I
> think xorg-modesettting actually falls over. Or at least I've seen
> some hacks that the agx people added to make sure X doesn't
> accidentally open the wrong driver.

That's a good point. However I recall from earlier attempts at doing
something like this in Nouveau (although this is now very long ago) that
it's not very easy. The problem, as I recall, is that the driver is a
singleton, so we would essentially be supporting either modesetting or
not, for any device in the system.

Now, it's unlikely that we'd have a mix of one Tegra DRM driver with
display support and another without, but it's something that I recall
back at the time with Nouveau was problematic because you could have the
Tegra integrated graphics (without display support) and a PCI-connected
discrete GPU (with display support) within the same system.

I need to look into it a bit more to see if I can come up with something
good to account for this.

Thierry

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

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Javier Martinez Canillas]

Thierry Reding <thierry.reding@gmail.com> writes:

Hello Thierry,

> On Wed, Aug 30, 2023 at 08:13:04AM +0200, Javier Martinez Canillas wrote:

[...]

>> I also wonder if is worth to move the drm_num_crtcs() function from
>> drivers/gpu/drm/drm_crtc.c to include/drm/drm_crtc.h and use that helper
>> instead?
>
> I've been looking at this, there's a few things that come to mind. It
> seems like we have a couple of different ways to get the number of CRTCs
> for a device. We have struct drm_device's num_crtcs, which is set during
> drm_vblank_init(), then we have struct drm_mode_config's num_crtc, which
> is incremented every time a new CRTC is added (and decremented when a
> CRTC is removed), and finally we've got the drm_num_crtcs() which
> "computes" the number of CRTCs registered by iterating over all CRTCs
> that have been registered.
>
> Are there any cases where these three can yield different values? Would
> it not make sense to consolidate these into a single variable?
>

I als was confused by that when looked at the implementation of the
mentioned helpers and couldn't find a reason why there are different
ways to calculate the number of CRTCs.

Maybe Sima or someone else can shed some light?

> Thierry

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3149 bytes --]

Hi

Am 07.09.23 um 10:03 schrieb Thierry Reding:
> On Thu, Aug 31, 2023 at 10:04:29AM +0200, Daniel Vetter wrote:
>> On Thu, 31 Aug 2023 at 08:33, Mikko Perttunen <cyndis@kapsi.fi> wrote:
>>>
>>> On 8/30/23 13:19, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 25.08.23 um 15:22 schrieb Thierry Reding:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Tegra DRM doesn't support display on Tegra234 and later, so make sure
>>>>> not to remove any existing framebuffers in that case.
>>>>>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>> ---
>>>>>    drivers/gpu/drm/tegra/drm.c | 8 +++++---
>>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>>>>> index b1e1a78e30c6..7a38dadbc264 100644
>>>>> --- a/drivers/gpu/drm/tegra/drm.c
>>>>> +++ b/drivers/gpu/drm/tegra/drm.c
>>>>> @@ -1220,9 +1220,11 @@ static int host1x_drm_probe(struct
>>>>> host1x_device *dev)
>>>>>        drm_mode_config_reset(drm);
>>>>> -    err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
>>>>> -    if (err < 0)
>>>>> -        goto hub;
>>>>> +    if (drm->mode_config.num_crtc > 0) {
>>>>
>>>> If you don't support the hardware, wouldn't it be better to return
>>>> -ENODEV if !num_crtc?
>>>
>>> While display is not supported through TegraDRM on Tegra234+, certain
>>> multimedia accelerators are supported, so we need to finish probe for those.
>>
>> Ideally you also register the tegra driver without DRIVER_MODESET |
>> DRIVER_ATOMIC in that case, to avoid unecessary userspace confusion.
>> Most userspace can cope with a display driver without any crtc, but I
>> think xorg-modesettting actually falls over. Or at least I've seen
>> some hacks that the agx people added to make sure X doesn't
>> accidentally open the wrong driver.
> 
> That's a good point. However I recall from earlier attempts at doing
> something like this in Nouveau (although this is now very long ago) that
> it's not very easy. The problem, as I recall, is that the driver is a
> singleton, so we would essentially be supporting either modesetting or
> not, for any device in the system.

Take a look at struct drm_device.driver_features. It let's you clear the 
flags that your device doesn't support.

https://elixir.bootlin.com/linux/v6.5/source/include/drm/drm_device.h#L128

Best regards
Thomas

> 
> Now, it's unlikely that we'd have a mix of one Tegra DRM driver with
> display support and another without, but it's something that I recall
> back at the time with Nouveau was problematic because you could have the
> Tegra integrated graphics (without display support) and a PCI-connected
> discrete GPU (with display support) within the same system.
> 
> I need to look into it a bit more to see if I can come up with something
> good to account for this.
> 
> Thierry

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH] drm/tegra: Remove existing framebuffer only if we support display

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

[...]

>> That's a good point. However I recall from earlier attempts at doing
>> something like this in Nouveau (although this is now very long ago) that
>> it's not very easy. The problem, as I recall, is that the driver is a
>> singleton, so we would essentially be supporting either modesetting or
>> not, for any device in the system.
>
> Take a look at struct drm_device.driver_features. It let's you clear the 
> flags that your device doesn't support.
>
> https://elixir.bootlin.com/linux/v6.5/source/include/drm/drm_device.h#L128
>

That sounds indeed as the best approach and I see that at least i915 does it:

https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/i915/intel_device_info.c#L418

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat