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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]