| Message ID | 19f321ab-8896-f746-3d75-e50e680b8c00@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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
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);