[Intel-gfx] [PATCH 6/7] drm/i915: Improve how the memory for crtc state is allocated

[Daniel Vetter]

On Fri, Jan 16, 2015 at 03:48:56PM -0800, Matt Roper wrote:
> On Thu, Jan 15, 2015 at 02:55:26PM +0200, Ander Conselvan de Oliveira wrote:
> > The previous patch changed the config field in intel_crtc to a pointer,
> > but to keep the mechanical changes (done with spatch) separate from the
> > new code, the pointer was made to point to a new _config field with type
> > struct intel_crtc_state added to that struct. This patch improves that
> > code by getting rid of that field, allocating a state struct in
> > intel_crtc_init() a keeping it properly updated when a mode set
> > happens.
> > 
> > v2: Manual changes split from previous patch. (Matt)
> >     Don't leak the current state when the crtc is destroyed (Matt)
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 -
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index acdaed2..002e5a9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8926,6 +8926,13 @@ out:
> >  	intel_runtime_pm_put(dev_priv);
> >  }
> >  
> > +static void intel_crtc_set_state(struct intel_crtc *crtc,
> > +				 struct intel_crtc_state *crtc_state)
> > +{
> > +	kfree(crtc->config);
> > +	crtc->config = crtc_state;
> > +}
> > +
> >  static void intel_crtc_destroy(struct drm_crtc *crtc)
> >  {
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> >  
> >  	drm_crtc_cleanup(crtc);
> >  
> > +	intel_crtc_set_state(intel_crtc, NULL);
> 
> Actually I just looked at this again and I think this is going to cause
> a non-fatal warning on unload.  Given your update of the base state in
> patch 7, the drm_crtc_cleanup() here is going to see crtc->state as non-NULL and
> try to clean it up itself.  But since you haven't implemented
> crtc->funcs->atomic_destroy_state(), it will just throw a warning and
> continue on.
> 
> So I think you want to do one of the following:
>  * Move the intel_crtc_set_state() call above drm_crtc_cleanup() so that
>    the core won't see a non-NULL state and think it needs to clean up.
>  * Completely drop the intel_crtc_set_state() call and instead implement
>    crtc->funcs->atomic_destroy_state() so that the core can handle the
>    cleanup for us.

I figured we can do this in a follow-up patch, so went ahead and merged
the entire series. First approach is probably simpler until we have the
state handling all wired up, but I'll leave that to you two.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch