| Message ID | 20230825132229.1109445-1-thierry.reding@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | drm/tegra: Remove existing framebuffer only if we support display | expand |
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?
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)
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) >
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) > > >
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
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
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
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 <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 >
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)