[Intel-gfx] [PATCH] drm/i915: Be defensive and don't assume PSR has any commit to sync against

[Ville Syrjälä]

On Tue, Sep 04, 2018 at 08:54:03PM +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-09-04 at 19:12 +0100, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2018-09-04 19:06:29)
> > > On Tue, Sep 04, 2018 at 08:59:32PM +0300, Ville Syrjälä wrote:
> > > > On Tue, Sep 04, 2018 at 06:44:14PM +0100, Chris Wilson wrote:
> > > > > Quoting Ville Syrjälä (2018-09-04 18:39:53)
> > > > > > On Tue, Sep 04, 2018 at 05:29:02PM +0100, Chris Wilson wrote:
> > > > > > > If the previous modeset commit has completed and is no
> > > > > > > longer part of
> > > > > > > the crtc state, skip waiting for it.
> > > > > > > 
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1077
> > > > > > > 92
> > > > > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR at
> > > > > > > runtime through debugfs, v6")
> > > > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++------
> > > > > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > index 21984d4c08ed..bddc9c7c681e 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > @@ -834,6 +834,7 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >       struct drm_device *dev = &dev_priv->drm;
> > > > > > >       struct drm_connector_state *conn_state;
> > > > > > >       struct intel_crtc_state *crtc_state = NULL;
> > > > > > > +     struct drm_crtc_commit *commit = NULL;
> > > > > > >       struct drm_crtc *crtc;
> > > > > > >       struct intel_dp *dp;
> > > > > > >       int ret;
> > > > > > > @@ -860,12 +861,15 @@ int intel_psr_set_debugfs_mode(struct
> > > > > > > drm_i915_private *dev_priv,
> > > > > > >                       return ret;
> > > > > > >  
> > > > > > >               crtc_state = to_intel_crtc_state(crtc-
> > > > > > > >state);
> > > > > > > -             ret =
> > > > > > > wait_for_completion_interruptible(&crtc_state->base.commit-
> > > > > > > >hw_done);
> > > > > > > -     } else
> > > > > > > -             ret =
> > > > > > > wait_for_completion_interruptible(&conn_state->commit-
> > > > > > > >hw_done);
> > > > > > > -
> > > > > > > -     if (ret)
> > > > > > > -             return ret;
> > > > > > > +             commit = crtc_state->base.commit;
> > > > > > > +     } else {
> > > > > > > +             commit = conn_state->commit;
> > > > > > 
> > > > > > I can't even find where we clear state->commit after its
> > > > > > done.
> > > > > > Do we just leave it pointing at freed memory or something?
> > > > > > Also I
> > > > > > can't figure out why drm_atomic_helper_commit_hw_done()
> > > > > > copies
> > > > > > the commit also to the old state.
> > > > > 
> > > > > Let me be the messenger then ;) commit is NULL at this point, I
> > > > > just
> > > > > presumed it was intentional.
> > > > 
> > > > My expectation would be that it gets cleared somewhere, but I
> > > > simply
> > > > can't find any such code.
> > > 
> > > Actually it looks like there is no such code. The event based
> > > release_crtc_commit() thing gets its own reference so presumably
> > > the
> > > original reference stays with the state until the state itself gets
> > > destroyed.
> > 
> > Happy with the it never had a commit theory, or is this a deeper
> > problem
> > that needs root causing?
> 
> Just so that I understand this correctly, even if there was a prior
> commit, state->commit would have been freed when completion was
> signaled. Is that right?

Nah, looks like it's going to hang on to the commit as long as the
state exists. At least that's my reading of the atomic helper.
Or maybe I'm missing something clever? Daniel/Maarten?

-- 
Ville Syrjälä
Intel