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@xxxxxxxxxxxxxxxxxx > > > > > > > > > > > > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel. > > > > > > > > > com> > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxx > > > > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
