Re: [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure

From: Lukas Wunner <lukas@wunner.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Alex Deucher <alexander.deucher@amd.com>,
	nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	Dave Airlie <airlied@redhat.com>
Subject: Re: [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure
Date: Wed, 25 May 2016 00:07:53 +0200	[thread overview]
Message-ID: <20160524220753.GA5941@wunner.de> (raw)
In-Reply-To: <20160524213042.GC27098@phenom.ffwll.local>

Good evening Daniel,

On Tue, May 24, 2016 at 11:30:42PM +0200, Daniel Vetter wrote:
> On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> > When a drm_crtc structure is destroyed with drm_crtc_cleanup(), the DRM
> > core does not turn off the crtc first and neither do the drivers. With
> > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on
> > driver unload if at least one crtc was enabled.
> > 
> > (See usage of have_disp_power_ref in nouveau_crtc_set_config(),
> > radeon_crtc_set_config() and amdgpu_crtc_set_config()).
> > 
> > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> > Cc: Dave Airlie <airlied@redhat.com>
> > Tested-by: Karol Herbst <nouveau@karolherbst.de>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> This is a core regression, we fixed it again. Previously when unreference
> drm_planes the core made sure that it's not longer in use, which had the
> side effect of shutting everything off in module unload.
> 
> For a bunch of reasons we've stopped doing that, but that turned out to be
> a mistake. It's fixed since
> 
> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Wed May 4 14:38:26 2016 +0200
> 
>     drm/core: Do not preserve framebuffer on rmfb, v4.

Okay, I will test it.

May I ask you a question while you have this topic swapped into your
brain: nouveau, radeon and amdgpu currently hold one runtime pm ref
if any crtc is turned on. I'm pondering how to make this work for
muxed dual GPU laptops. When switching GPUs, the now inactive GPU
should turn off the crtc used by the panel to save power (if it's
*only* used by the panel) and release that runtime pm ref. Likewise,
the now active GPU needs to turn on the crtc used by the panel and
take a runtime pm ref.

The whole thing becomes a bit complicated because MacBook Pros with
Thunderbolt can only drive external displays with their discrete GPU.
So when switching to the integrated GPU, the discrete GPU may turn
off the crtc for the panel but the crtc for the external display needs
to stay alive if it's in use and the GPU may not suspend.

What I have in mind is to change the scheme nouveau/radeon/amdgpu are
currently using by taking a runtime pm ref when enabling a crtc and
releasing it when disabling the crtc. This is different from the status
quo where only a *single* runtime pm ref is taken if *any* crtc is enabled.

Upon switching, the runtime pm ref for the crtc previously used by the
panel is released and the crtc should be turned off. If it was the only
active crtc the GPU automatically goes to sleep.

I'm thinking about putting the pm_runtime_get() and pm_runtime_put()
in the DRM core. (Actually I already have a commit to do just that in
my local repo.) That way we're using less code in nouveau/radeon/amdgpu
because we're doing the runtime pm handling centrally in the core.
I'm wondering if that would impact other DRM drivers negatively.

Basically the idea is to harmonize runtime pm handling among DRM drivers.
What do you think about that?

Thanks,

Lukas

> 
> Your patch shouldn't be needed with that any more. If it still is it's
> most likely the fbdev cleanup done too late, but you /should/ get a big
> WARNING splat in that case from drm_mode_config_cleanup().
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index d2a6d95..0cd6f00 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -716,12 +716,23 @@ EXPORT_SYMBOL(drm_crtc_init_with_planes);
> >   *
> >   * This function cleans up @crtc and removes it from the DRM mode setting
> >   * core. Note that the function does *not* free the crtc structure itself,
> > - * this is the responsibility of the caller.
> > + * this is the responsibility of the caller. If @crtc is currently enabled,
> > + * it is turned off first.
> >   */
> >  void drm_crtc_cleanup(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  
> > +	if (crtc->enabled) {
> > +		struct drm_mode_set modeset = {
> > +			.crtc = crtc,
> > +		};
> > +
> > +		drm_modeset_lock_all(dev);
> > +		drm_mode_set_config_internal(&modeset);
> > +		drm_modeset_unlock_all(dev);
> > +	}
> > +
> >  	kfree(crtc->gamma_store);
> >  	crtc->gamma_store = NULL;
> >  
> > -- 
> > 2.8.1
> > 
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel