Non-blocking commits on -ERESTARTSYS
diff mbox

Message ID 19f321ab-8896-f746-3d75-e50e680b8c00@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Dec. 13, 2017, 5:23 p.m. UTC
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?

No idea if sane though, but drivers are supposed to clear crtc_state->event on success..

Comments

Leo Dec. 13, 2017, 7:24 p.m. UTC | #1
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);
Leo Dec. 14, 2017, 2:43 p.m. UTC | #2
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox

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