diff mbox series

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

Message ID 20240223150333.1401582-1-thierry.reding@gmail.com (mailing list archive)
State Accepted
Headers show
Series [v2] drm/tegra: Remove existing framebuffer only if we support display | expand

Commit Message

Thierry Reding Feb. 23, 2024, 3:03 p.m. UTC
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.

v2: - add comments explaining how this situation can come about
    - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits

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

Comments

Javier Martinez Canillas Feb. 23, 2024, 3:26 p.m. UTC | #1
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.
>
> v2: - add comments explaining how this situation can come about
>     - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Thomas Zimmermann Feb. 26, 2024, 8:24 a.m. UTC | #2
Hi

Am 23.02.24 um 16:03 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.
>
> v2: - add comments explaining how this situation can come about
>      - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Makes sense as far as the aperture helpers are concerned.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> ---
>   drivers/gpu/drm/tegra/drm.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index b1e1a78e30c6..2e1cfe6f6d74 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1220,9 +1220,26 @@ 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;
> +	/*
> +	 * Only take over from a potential firmware framebuffer if any CRTCs
> +	 * have been registered. This must not be a fatal error because there
> +	 * are other accelerators that are exposed via this driver.
> +	 *
> +	 * Another case where this happens is on Tegra234 where the display
> +	 * hardware is no longer part of the host1x complex, so this driver
> +	 * will not expose any modesetting features.
> +	 */
> +	if (drm->mode_config.num_crtc > 0) {
> +		err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
> +		if (err < 0)
> +			goto hub;
> +	} else {
> +		/*
> +		 * Indicate to userspace that this doesn't expose any display
> +		 * capabilities.
> +		 */
> +		drm->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
> +	}
>   
>   	err = drm_dev_register(drm, 0);
>   	if (err < 0)
Javier Martinez Canillas Feb. 26, 2024, 11:36 a.m. UTC | #3
Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 23.02.24 um 16:03 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.
>>
>> v2: - add comments explaining how this situation can come about
>>      - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>
> Makes sense as far as the aperture helpers are concerned.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>

I believe this is drm-misc-fixes material. Since the tegra DRM will remove
simple{fb,drm} for Tegra234, even when the driver does not support display
on that platform, leaving the system with no display output at all.

Are you going to push this patch or is going to be done by Thierry?

> Best regards
> Thomas
>
Robert Foss Feb. 26, 2024, 12:08 p.m. UTC | #4
On Mon, Feb 26, 2024 at 12:36 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> > Hi
> >
> > Am 23.02.24 um 16:03 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.
> >>
> >> v2: - add comments explaining how this situation can come about
> >>      - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
> >>

Fixes: 6848c291a54f ("drm/aperture: Convert drivers to aperture interfaces")

Maybe this is more of a philosophical question, but either the
introduction of this hardware generation is where this regression was
introduced or this possibly this commit.

Either way, I'd like to get this into the drm-misc-fixes branch.

> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >
> > Makes sense as far as the aperture helpers are concerned.
> >
> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> >
>
> I believe this is drm-misc-fixes material. Since the tegra DRM will remove
> simple{fb,drm} for Tegra234, even when the driver does not support display
> on that platform, leaving the system with no display output at all.
>
> Are you going to push this patch or is going to be done by Thierry?

I'm on it.

>
> > Best regards
> > Thomas
> >
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
Robert Foss Feb. 26, 2024, 12:37 p.m. UTC | #5
On Fri, 23 Feb 2024 16:03:33 +0100, Thierry Reding wrote:
> 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.
> 
> v2: - add comments explaining how this situation can come about
>     - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
> 
> [...]

Applied, thanks!

[1/1] drm/tegra: Remove existing framebuffer only if we support display
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=86bf8cfda6d2



Rob
Thierry Reding Feb. 26, 2024, 2:04 p.m. UTC | #6
On Mon Feb 26, 2024 at 1:08 PM CET, Robert Foss wrote:
> On Mon, Feb 26, 2024 at 12:36 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
> >
> > Thomas Zimmermann <tzimmermann@suse.de> writes:
> >
> > > Hi
> > >
> > > Am 23.02.24 um 16:03 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.
> > >>
> > >> v2: - add comments explaining how this situation can come about
> > >>      - clear DRIVER_MODESET and DRIVER_ATOMIC feature bits
> > >>
>
> Fixes: 6848c291a54f ("drm/aperture: Convert drivers to aperture interfaces")
>
> Maybe this is more of a philosophical question, but either the
> introduction of this hardware generation is where this regression was
> introduced or this possibly this commit.
>
> Either way, I'd like to get this into the drm-misc-fixes branch.

That commit looks about right. Technically Tegra234 support was
introduced in Linux 5.10 but the first platform where you we would've
seen this wasn't added until 5.17. The above commit is from 5.14, which
puts it about right in between there, so I think that's fine.

Backporting to anything before 5.14 would need to be manual and isn't
worth it.

Thierry
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index b1e1a78e30c6..2e1cfe6f6d74 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1220,9 +1220,26 @@  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;
+	/*
+	 * Only take over from a potential firmware framebuffer if any CRTCs
+	 * have been registered. This must not be a fatal error because there
+	 * are other accelerators that are exposed via this driver.
+	 *
+	 * Another case where this happens is on Tegra234 where the display
+	 * hardware is no longer part of the host1x complex, so this driver
+	 * will not expose any modesetting features.
+	 */
+	if (drm->mode_config.num_crtc > 0) {
+		err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
+		if (err < 0)
+			goto hub;
+	} else {
+		/*
+		 * Indicate to userspace that this doesn't expose any display
+		 * capabilities.
+		 */
+		drm->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+	}
 
 	err = drm_dev_register(drm, 0);
 	if (err < 0)