| Message ID | 20170816142224.25173-1-Jerry.Zuo@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 16, 2017 at 10:22 AM, Jerry Zuo <Jerry.Zuo@amd.com> wrote: > During page flip atomic_check and atomic_commit can return > -ERESTARTSYS to restart the ioctl. When this happens we fail to > put the commit object leading to a memory leak. > > Signed-off-by: Jerry Zuo <Jerry.Zuo@amd.com> The subject should be: drm/atomic: put commit when -ERESTARTSYS received Since the change is in the core atomic code. Alex > --- > drivers/gpu/drm/drm_atomic.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index f32506a7c1d6..f2f623dacf90 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1642,14 +1642,35 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state) > { > struct drm_mode_config *config = &state->dev->mode_config; > int ret; > + int i; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > > ret = drm_atomic_check_only(state); > - if (ret) > + if (ret) { > + if (ret == -ERESTARTSYS) > + goto fail; > + > return ret; > + } > > DRM_DEBUG_ATOMIC("commiting %p nonblocking\n", state); > > - return config->funcs->atomic_commit(state->dev, state, true); > + ret = config->funcs->atomic_commit(state->dev, state, true); > + if (ret == -ERESTARTSYS) > + goto fail; > + > + return ret; > + > + /* cleanup commit object if commit fails with ERESTARTSYS */ > +fail: > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + if (state->crtcs[i].commit) { > + drm_crtc_commit_put(state->crtcs[i].commit); > + } > + } > + > + return ret; > } > EXPORT_SYMBOL(drm_atomic_nonblocking_commit); > > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 16, 2017 at 7:12 PM, Alex Deucher <alexdeucher@gmail.com> wrote: > On Wed, Aug 16, 2017 at 10:22 AM, Jerry Zuo <Jerry.Zuo@amd.com> wrote: >> During page flip atomic_check and atomic_commit can return >> -ERESTARTSYS to restart the ioctl. When this happens we fail to >> put the commit object leading to a memory leak. >> >> Signed-off-by: Jerry Zuo <Jerry.Zuo@amd.com> > > The subject should be: > drm/atomic: put commit when -ERESTARTSYS received > Since the change is in the core atomic code. Do we have an igt testcase to exercise this? This is the kind of error case handling igt really is made for, and igt has ready-made helpers to interrupt ioctls. I think Maarten was even working on such a testcase, adding him. -Daniel > > Alex > >> --- >> drivers/gpu/drm/drm_atomic.c | 25 +++++++++++++++++++++++-- >> 1 file changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index f32506a7c1d6..f2f623dacf90 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -1642,14 +1642,35 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state) >> { >> struct drm_mode_config *config = &state->dev->mode_config; >> int ret; >> + int i; >> + struct drm_crtc *crtc; >> + struct drm_crtc_state *crtc_state; >> >> ret = drm_atomic_check_only(state); >> - if (ret) >> + if (ret) { >> + if (ret == -ERESTARTSYS) >> + goto fail; >> + >> return ret; >> + } >> >> DRM_DEBUG_ATOMIC("commiting %p nonblocking\n", state); >> >> - return config->funcs->atomic_commit(state->dev, state, true); >> + ret = config->funcs->atomic_commit(state->dev, state, true); >> + if (ret == -ERESTARTSYS) >> + goto fail; >> + >> + return ret; >> + >> + /* cleanup commit object if commit fails with ERESTARTSYS */ >> +fail: >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + if (state->crtcs[i].commit) { >> + drm_crtc_commit_put(state->crtcs[i].commit); >> + } >> + } >> + >> + return ret; >> } >> EXPORT_SYMBOL(drm_atomic_nonblocking_commit); >> >> -- >> 2.11.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2017-08-16 01:22 PM, Daniel Vetter wrote: > On Wed, Aug 16, 2017 at 7:12 PM, Alex Deucher <alexdeucher@gmail.com> wrote: >> On Wed, Aug 16, 2017 at 10:22 AM, Jerry Zuo <Jerry.Zuo@amd.com> wrote: >>> During page flip atomic_check and atomic_commit can return >>> -ERESTARTSYS to restart the ioctl. When this happens we fail to >>> put the commit object leading to a memory leak. >>> >>> Signed-off-by: Jerry Zuo <Jerry.Zuo@amd.com> >> >> The subject should be: >> drm/atomic: put commit when -ERESTARTSYS received >> Since the change is in the core atomic code. > > Do we have an igt testcase to exercise this? This is the kind of error > case handling igt really is made for, and igt has ready-made helpers > to interrupt ioctls. I think Maarten was even working on such a > testcase, adding him. I'm not aware of an IGT test for this. We triggered this scenario when running mode changes while glxgears is running. We observed no user-visible issue, only a memory leak. Is IGT able to capture those (semi-)reliably? I agree that it would be great to have an IGT test for these corner cases. Harry > -Daniel > >> >> Alex >> >>> --- >>> drivers/gpu/drm/drm_atomic.c | 25 +++++++++++++++++++++++-- >>> 1 file changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>> index f32506a7c1d6..f2f623dacf90 100644 >>> --- a/drivers/gpu/drm/drm_atomic.c >>> +++ b/drivers/gpu/drm/drm_atomic.c >>> @@ -1642,14 +1642,35 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state) >>> { >>> struct drm_mode_config *config = &state->dev->mode_config; >>> int ret; >>> + int i; >>> + struct drm_crtc *crtc; >>> + struct drm_crtc_state *crtc_state; >>> >>> ret = drm_atomic_check_only(state); >>> - if (ret) >>> + if (ret) { >>> + if (ret == -ERESTARTSYS) >>> + goto fail; >>> + >>> return ret; >>> + } >>> >>> DRM_DEBUG_ATOMIC("commiting %p nonblocking\n", state); >>> >>> - return config->funcs->atomic_commit(state->dev, state, true); >>> + ret = config->funcs->atomic_commit(state->dev, state, true); >>> + if (ret == -ERESTARTSYS) >>> + goto fail; >>> + >>> + return ret; >>> + >>> + /* cleanup commit object if commit fails with ERESTARTSYS */ >>> +fail: >>> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >>> + if (state->crtcs[i].commit) { >>> + drm_crtc_commit_put(state->crtcs[i].commit); >>> + } >>> + } >>> + >>> + return ret; >>> } >>> EXPORT_SYMBOL(drm_atomic_nonblocking_commit); >>> >>> -- >>> 2.11.0 >>> >>> _______________________________________________ >>> 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.c b/drivers/gpu/drm/drm_atomic.c index f32506a7c1d6..f2f623dacf90 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1642,14 +1642,35 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state) { struct drm_mode_config *config = &state->dev->mode_config; int ret; + int i; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; ret = drm_atomic_check_only(state); - if (ret) + if (ret) { + if (ret == -ERESTARTSYS) + goto fail; + return ret; + } DRM_DEBUG_ATOMIC("commiting %p nonblocking\n", state); - return config->funcs->atomic_commit(state->dev, state, true); + ret = config->funcs->atomic_commit(state->dev, state, true); + if (ret == -ERESTARTSYS) + goto fail; + + return ret; + + /* cleanup commit object if commit fails with ERESTARTSYS */ +fail: + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (state->crtcs[i].commit) { + drm_crtc_commit_put(state->crtcs[i].commit); + } + } + + return ret; } EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
During page flip atomic_check and atomic_commit can return -ERESTARTSYS to restart the ioctl. When this happens we fail to put the commit object leading to a memory leak. Signed-off-by: Jerry Zuo <Jerry.Zuo@amd.com> --- drivers/gpu/drm/drm_atomic.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)