| Message ID | 20180904162902.2578-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Be defensive and don't assume PSR has any commit to sync against | expand |
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=107792 > 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.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. > + } > + if (commit) { > + ret = wait_for_completion_interruptible(&commit->hw_done); > + if (ret) > + return ret; > + } > > ret = mutex_lock_interruptible(&dev_priv->psr.lock); > if (ret) > -- > 2.19.0.rc1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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=107792 > > 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.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. -Chris
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=107792 > > > 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.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. So the only explanation I have now is that there has never been any commits on that crtc/connector before and that's why it's NULL. Based on that theory this does seem sane, so Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
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=107792 > > > > 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.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.
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=107792 > > > > > 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.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? -Chris
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@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.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? > -Chris
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@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.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?
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?id=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.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? > 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..
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.
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; + } + if (commit) { + ret = wait_for_completion_interruptible(&commit->hw_done); + if (ret) + return ret; + } ret = mutex_lock_interruptible(&dev_priv->psr.lock); if (ret)
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=107792 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.com> --- drivers/gpu/drm/i915/intel_psr.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)