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