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

[Maarten Lankhorst]

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 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?
>
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..