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@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
