Re: Non-blocking commits on -ERESTARTSYS

Re: Non-blocking commits on -ERESTARTSYS

[Maarten Lankhorst]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

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


[-- Attachment #2: drop-extra-refcount.patch --]
[-- Type: text/x-diff, Size: 553 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: 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);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for Non-blocking commits on -ERESTARTSYS

[Patchwork]

== Series Details ==

Series: Non-blocking commits on -ERESTARTSYS
URL   : https://patchwork.freedesktop.org/series/35299/
State : success

== Summary ==

Series 35299v1 Non-blocking commits on -ERESTARTSYS
https://patchwork.freedesktop.org/api/1.0/series/35299/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-kbl-r) fdo#104172 +1
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:437s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:440s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:384s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:514s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:278s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:507s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:502s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:488s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:468s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:1   dfail:0   fail:0   skip:108 time:267s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:534s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:407s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:416s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:396s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:429s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:529s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:469s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:530s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:536s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:490s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:450s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:418s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:593s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:491s

1a717c76c96c2883866e4041926285b0f576fb54 drm-tip: 2017y-12m-13d-14h-48m-36s UTC integration manifest
3f843da44156 Non-blocking commits on -ERESTARTSYS

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7488/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for Non-blocking commits on -ERESTARTSYS

[Patchwork]

== Series Details ==

Series: Non-blocking commits on -ERESTARTSYS
URL   : https://patchwork.freedesktop.org/series/35299/
State : failure

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-indfb-draw-mmap-gtt:
                pass       -> SKIP       (shard-hsw) fdo#101623 +1
        Subgroup fbc-1p-primscrn-pri-shrfb-draw-render:
                pass       -> FAIL       (shard-snb) fdo#103167
Test pm_rpm:
        Subgroup modeset-non-lpsp-stress-no-wait:
                pass       -> SKIP       (shard-hsw)
Test kms_cursor_crc:
        Subgroup cursor-256x256-suspend:
                skip       -> PASS       (shard-snb) fdo#103375 +1
Test kms_flip:
        Subgroup busy-flip:
                pass       -> INCOMPLETE (shard-snb)
                pass       -> INCOMPLETE (shard-hsw) fdo#103257 +1
        Subgroup busy-flip-interruptible:
                pass       -> INCOMPLETE (shard-snb)
Test kms_plane_lowres:
        Subgroup pipe-b-tiling-none:
                pass       -> INCOMPLETE (shard-hsw) fdo#103181
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#103257 https://bugs.freedesktop.org/show_bug.cgi?id=103257
fdo#103181 https://bugs.freedesktop.org/show_bug.cgi?id=103181
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2634 pass:1488 dwarn:1   dfail:0   fail:11  skip:1131 time:8753s
shard-snb        total:2708 pass:1305 dwarn:1   dfail:0   fail:12  skip:1388 time:7747s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7488/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx