| Message ID | 1515095253-29817-1-git-send-email-sunpeng.li@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-01-04 02:47 PM, sunpeng.li@amd.com wrote: > From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com> > > During a non-blocking commit, it is possible to return before the > commit_tail work is queued (-ERESTARTSYS, for example). > > Since a reference on the crtc commit object is obtained for the pending > vblank event when preparing the commit, the above situation will leave > us with an extra reference. > > Therefore, if the commit_tail worker has not consumed the event at the > end of a commit, release it's reference. > > Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com> No expert on this but looks sane to me. Acked-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ab40321..4253f57 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3421,6 +3421,15 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); > void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) > { > if (state->commit) { > + /* > + * In the event that a non-blocking commit returns > + * -ERESTARTSYS before the commit_tail work is queued, we will > + * have an extra reference to the commit object. Release it, if > + * the event has not been consumed by the worker. > + */ > + if (state->event) > + drm_crtc_commit_put(state->commit); > + > kfree(state->commit->event); > state->commit->event = NULL; > drm_crtc_commit_put(state->commit); >
On Mon, Jan 8, 2018 at 4:30 PM, Harry Wentland <harry.wentland@amd.com> wrote: > On 2018-01-04 02:47 PM, sunpeng.li@amd.com wrote: >> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com> >> >> During a non-blocking commit, it is possible to return before the >> commit_tail work is queued (-ERESTARTSYS, for example). >> >> Since a reference on the crtc commit object is obtained for the pending >> vblank event when preparing the commit, the above situation will leave >> us with an extra reference. >> >> Therefore, if the commit_tail worker has not consumed the event at the >> end of a commit, release it's reference. >> >> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com> > > No expert on this but looks sane to me. > > Acked-by: Harry Wentland <harry.wentland@amd.com> Pushed to drm-misc-next. Thanks, Alex > > Harry > >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index ab40321..4253f57 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -3421,6 +3421,15 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); >> void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) >> { >> if (state->commit) { >> + /* >> + * In the event that a non-blocking commit returns >> + * -ERESTARTSYS before the commit_tail work is queued, we will >> + * have an extra reference to the commit object. Release it, if >> + * the event has not been consumed by the worker. >> + */ >> + 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
On Tue, Jan 09, 2018 at 01:13:33PM -0500, Alex Deucher wrote: > On Mon, Jan 8, 2018 at 4:30 PM, Harry Wentland <harry.wentland@amd.com> wrote: > > On 2018-01-04 02:47 PM, sunpeng.li@amd.com wrote: > >> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com> > >> > >> During a non-blocking commit, it is possible to return before the > >> commit_tail work is queued (-ERESTARTSYS, for example). > >> > >> Since a reference on the crtc commit object is obtained for the pending > >> vblank event when preparing the commit, the above situation will leave > >> us with an extra reference. > >> > >> Therefore, if the commit_tail worker has not consumed the event at the > >> end of a commit, release it's reference. > >> > >> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com> > > > > No expert on this but looks sane to me. > > > > Acked-by: Harry Wentland <harry.wentland@amd.com> > > > Pushed to drm-misc-next. This blows up all over the place in our CI: https://bugs.freedesktop.org/show_bug.cgi?id=104566 I think as a leak fix it should also be pushed to -fixes. And we didn't really figure out what's wrong, so we're going with the quick revert until you folks are all awake. Cheers, Daniel > > Thanks, > > Alex > > > > > Harry > > > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index ab40321..4253f57 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -3421,6 +3421,15 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); > >> void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) > >> { > >> if (state->commit) { > >> + /* > >> + * In the event that a non-blocking commit returns > >> + * -ERESTARTSYS before the commit_tail work is queued, we will > >> + * have an extra reference to the commit object. Release it, if > >> + * the event has not been consumed by the worker. > >> + */ > >> + 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
Op 08-01-18 om 22:30 schreef Harry Wentland: > On 2018-01-04 02:47 PM, sunpeng.li@amd.com wrote: >> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com> >> >> During a non-blocking commit, it is possible to return before the >> commit_tail work is queued (-ERESTARTSYS, for example). >> >> Since a reference on the crtc commit object is obtained for the pending >> vblank event when preparing the commit, the above situation will leave >> us with an extra reference. >> >> Therefore, if the commit_tail worker has not consumed the event at the >> end of a commit, release it's reference. >> >> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com> > No expert on this but looks sane to me. > > Acked-by: Harry Wentland <harry.wentland@amd.com> > > Harry Hey, I've reverted the patch for now. I planned to apply it to the drm-misc-fixes branch with the appropriate commits referenced, but it is causing crashes on the kms_flip testcase. https://bugs.freedesktop.org/show_bug.cgi?id=104566 I'll investigate it some more then send out a fixed patch. :) ~Maarten
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ab40321..4253f57 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3421,6 +3421,15 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state) { if (state->commit) { + /* + * In the event that a non-blocking commit returns + * -ERESTARTSYS before the commit_tail work is queued, we will + * have an extra reference to the commit object. Release it, if + * the event has not been consumed by the worker. + */ + if (state->event) + drm_crtc_commit_put(state->commit); + kfree(state->commit->event); state->commit->event = NULL; drm_crtc_commit_put(state->commit);