[Intel-gfx] Non-blocking commits on -ERESTARTSYS

[Leo Li]

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..
> 
> 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);