Non-blocking commits on -ERESTARTSYS

[Leo Li]

On 2017-12-13 02:24 PM, Leo Li wrote:
> 
> 
> On 2017-12-13 12:23 PM, Maarten Lankhorst wrote:
>> Op 13-12-17 om 17:19 schreef Leo Li:
>>> Hi Daniel, Maarten,
>>>
>>> Just digging an old thread out of the grave:
>>> https://lists.freedesktop.org/archives/dri-devel/2017-August/150495.html
>>>
>>> It's suppose to fix a memory leak on the drm_commit object during
>>> non-blocking commits. Within drm_atomic_helper_setup_commit, a reference
>>> to the commit object is obtained by the new_crtc_state. This reference
>>> is suppose to be 'put' once flip_done is signaled (via the
>>> release_crtc_commit callback), but never happens if .prepare_fb returns
>>> -ERESTARTSYS: drm_atomic_helper_commit early returns before the
>>> commit_tail work is queued.
>>>
>>> We're starting to bump into this issue again. Regarding Daniel's
>>> suggestion for an IGT test, has there been any work done on it? I'd be
>>> interested in taking a look otherwise. As a side note, I can also
>>> reproduce this on i915.
>>>
>>> Thanks,
>>> Leo
>>
>> I'm curious, isn't it better to handle this in 
>> __drm_atomic_helper_crtc_destroy_state with the attached patch?
> 
> Good point, it seems sane to me. I gave it a spin and it fixes the issue.
> 
> I was concerned that it'll contend with the worker thread, possibly
> freeing the crtc_commit before the flip is done. It seems the
> atomic_state_get before the work is queued will prevent that.
> 
> Leo
> 
>>
>> No idea if sane though, but drivers are supposed to clear 
>> crtc_state->event on success..

Hi Maarten,

If there are no objections, can I submit a patch with your change below?

Thanks,
Leo

>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 593b30d38ce0..e71233b4c651 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3435,6 +3435,8 @@ 
>> EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state 
>> *state)
>>  {
>>      if (state->commit) {
>> +        if (state->event)
>> +            drm_crtc_commit_put(state->commit);
>>          kfree(state->commit->event);
>>          state->commit->event = NULL;
>>          drm_crtc_commit_put(state->commit);
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel