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

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Wed, 2018-09-05 at 12:51 +0200, Maarten Lankhorst wrote:
> Op 05-09-18 om 12:22 schreef 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?i
> > > > > > > > > d=1077
> > > > > > > > > 92
> > > > > > > > > Fixes: c44301fce614 ("drm/i915: Allow control of PSR
> > > > > > > > > at
> > > > > > > > > runtime through debugfs, v6")
> > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk
> > > > > > > > > >
> > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.
> > > > > > > > > com>
> > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.co
> > > > > > > > > m>
> > > > > > > > > ---
> > > > > > > > >  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?
> > 
> 
> The commit is refcounted, and the original state has a reference. As
> long as that's not freed, the commit will hang around. So that can't
> cause a hang..
> 
Okay, the state corresponding to reading CRCs in the test, got freed
before PSR was enabled.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx