| Message ID | 1472143678-1277-1-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 25-08-16 om 18:47 schreef Gustavo Padovan: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > If userspace is running an synchronously atomic commit and interrupts the > atomic operation during fence_wait() it will hang until the timer expires, > so here we change the wait to be interruptible so it stop immediately when > userspace wants to quit. > > Also adds the necessary error checking for fence_wait(). > > v2: Comment by Daniel Vetter > - Add error checking for fence_wait() > > v3: Rebase on top of new atomic noblocking support > > v4: Comment by Maarten Lankhorst > - remove 'swapped' bitfield as it was duplicating information > > v5: Comments by Maarten Lankhorst > - assign plane->state to plane_state if !intr > - squash previous patch into this one > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan <gustavo@padovan.org> wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > If userspace is running an synchronously atomic commit and interrupts the > atomic operation during fence_wait() it will hang until the timer expires, > so here we change the wait to be interruptible so it stop immediately when > userspace wants to quit. > > Also adds the necessary error checking for fence_wait(). > > v2: Comment by Daniel Vetter > - Add error checking for fence_wait() > > v3: Rebase on top of new atomic noblocking support > > v4: Comment by Maarten Lankhorst > - remove 'swapped' bitfield as it was duplicating information > > v5: Comments by Maarten Lankhorst > - assign plane->state to plane_state if !intr > - squash previous patch into this one > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++++++++++++++++-------- > drivers/gpu/drm/msm/msm_atomic.c | 2 +- > include/drm/drm_atomic_helper.h | 5 +++-- > 3 files changed, 37 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index e1f5de2..f752f9c 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1007,29 +1007,47 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables); > * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state > * @dev: DRM device > * @state: atomic state object with old state structures > + * @intr: if true, do an interruptible wait > * > * For implicit sync, driver should fish the exclusive fence out from the > * incoming fb's and stash it in the drm_plane_state. This is called after > * drm_atomic_helper_swap_state() so it uses the current plane state (and > * just uses the atomic state to find the changed planes) > + * > + * Returns zero if sucess or < 0 if fence_wait() fails. > */ > -void drm_atomic_helper_wait_for_fences(struct drm_device *dev, > - struct drm_atomic_state *state) > +int drm_atomic_helper_wait_for_fences(struct drm_device *dev, > + struct drm_atomic_state *state, > + bool intr) > { > struct drm_plane *plane; > struct drm_plane_state *plane_state; > - int i; > + int i, ret; > > for_each_plane_in_state(state, plane, plane_state, i) { > - if (!plane->state->fence) > + /* > + * If the caller asks for an interruptible wait it means > + * that the state were not swapped yet and the operation > + * can still be interrupted by userspace, so we need > + * to look to plane_state instead. > + */ > + if (!intr) > + plane_state = plane->state; The "intr" name is tripping me up here, since it doesn't _really_ imply that the state hasn't been swapped yet (this is just an implementation detail of the caller(s). Perhaps we could change the name to "pre_swap", and remove the comment above this conditonal. Below, above fence_wait, add a new comment to the effect of: /* * If waiting for fences pre-swap (ie: nonblock), userspace can still interrupt * the operation. Instead of blocking until the timer expires, make the wait * interruptible. */ Or perhaps I'm being too pedantic (or missing the point) :) > + > + if (!plane_state->fence) > continue; > > - WARN_ON(!plane->state->fb); > + WARN_ON(!plane_state->fb); > + > + ret = fence_wait(plane_state->fence, intr); > + if (ret) > + return ret; > > - fence_wait(plane->state->fence, false); > - fence_put(plane->state->fence); > - plane->state->fence = NULL; > + fence_put(plane_state->fence); > + plane_state->fence = NULL; > } > + > + return 0; > } > EXPORT_SYMBOL(drm_atomic_helper_wait_for_fences); > > @@ -1176,7 +1194,7 @@ static void commit_tail(struct drm_atomic_state *state) > > funcs = dev->mode_config.helper_private; > > - drm_atomic_helper_wait_for_fences(dev, state); > + drm_atomic_helper_wait_for_fences(dev, state, false); > > drm_atomic_helper_wait_for_dependencies(state); > > @@ -1235,6 +1253,12 @@ int drm_atomic_helper_commit(struct drm_device *dev, > if (ret) > return ret; > > + if (!nonblock) { > + ret = drm_atomic_helper_wait_for_fences(dev, state, true); > + if (ret) > + return ret; > + } > + > /* > * This is the point of no return - everything below never fails except > * when the hw goes bonghits. Which means we can commit the new state on > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index 4a8a6f1..9518e43 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -112,7 +112,7 @@ static void complete_commit(struct msm_commit *c, bool async) > struct msm_drm_private *priv = dev->dev_private; > struct msm_kms *kms = priv->kms; > > - drm_atomic_helper_wait_for_fences(dev, state); > + drm_atomic_helper_wait_for_fences(dev, state, false); > > kms->funcs->prepare_commit(kms, state); > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index d86ae5d..a42c34b 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -43,8 +43,9 @@ int drm_atomic_helper_commit(struct drm_device *dev, > struct drm_atomic_state *state, > bool nonblock); > > -void drm_atomic_helper_wait_for_fences(struct drm_device *dev, > - struct drm_atomic_state *state); > +int drm_atomic_helper_wait_for_fences(struct drm_device *dev, > + struct drm_atomic_state *state, > + bool intr); > bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev, > struct drm_atomic_state *old_state, > struct drm_crtc *crtc); > -- > 2.5.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sean, 2016-09-12 Sean Paul <seanpaul@google.com>: > On Thu, Aug 25, 2016 at 12:47 PM, Gustavo Padovan <gustavo@padovan.org> wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > If userspace is running an synchronously atomic commit and interrupts the > > atomic operation during fence_wait() it will hang until the timer expires, > > so here we change the wait to be interruptible so it stop immediately when > > userspace wants to quit. > > > > Also adds the necessary error checking for fence_wait(). > > > > v2: Comment by Daniel Vetter > > - Add error checking for fence_wait() > > > > v3: Rebase on top of new atomic noblocking support > > > > v4: Comment by Maarten Lankhorst > > - remove 'swapped' bitfield as it was duplicating information > > > > v5: Comments by Maarten Lankhorst > > - assign plane->state to plane_state if !intr > > - squash previous patch into this one > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++++++++++++++++-------- > > drivers/gpu/drm/msm/msm_atomic.c | 2 +- > > include/drm/drm_atomic_helper.h | 5 +++-- > > 3 files changed, 37 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index e1f5de2..f752f9c 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1007,29 +1007,47 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables); > > * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state > > * @dev: DRM device > > * @state: atomic state object with old state structures > > + * @intr: if true, do an interruptible wait > > * > > * For implicit sync, driver should fish the exclusive fence out from the > > * incoming fb's and stash it in the drm_plane_state. This is called after > > * drm_atomic_helper_swap_state() so it uses the current plane state (and > > * just uses the atomic state to find the changed planes) > > + * > > + * Returns zero if sucess or < 0 if fence_wait() fails. > > */ > > -void drm_atomic_helper_wait_for_fences(struct drm_device *dev, > > - struct drm_atomic_state *state) > > +int drm_atomic_helper_wait_for_fences(struct drm_device *dev, > > + struct drm_atomic_state *state, > > + bool intr) > > { > > struct drm_plane *plane; > > struct drm_plane_state *plane_state; > > - int i; > > + int i, ret; > > > > for_each_plane_in_state(state, plane, plane_state, i) { > > - if (!plane->state->fence) > > + /* > > + * If the caller asks for an interruptible wait it means > > + * that the state were not swapped yet and the operation > > + * can still be interrupted by userspace, so we need > > + * to look to plane_state instead. > > + */ > > + if (!intr) > > + plane_state = plane->state; > > The "intr" name is tripping me up here, since it doesn't _really_ imply that the > state hasn't been swapped yet (this is just an implementation detail > of the caller(s). > > Perhaps we could change the name to "pre_swap", and remove the comment > above this conditonal. Below, above > fence_wait, add a new comment to the effect of: > > /* > * If waiting for fences pre-swap (ie: nonblock), userspace can still interrupt > * the operation. Instead of blocking until the timer expires, make the wait > * interruptible. > */ Indeed, that is much better naming. In this case specifically intr has the same meaning as pre_swap, but as you said it is an implementation detail and we need to avoid people to get it wrong. I'll update it and send a v4. Gustavo
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e1f5de2..f752f9c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1007,29 +1007,47 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables); * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state * @dev: DRM device * @state: atomic state object with old state structures + * @intr: if true, do an interruptible wait * * For implicit sync, driver should fish the exclusive fence out from the * incoming fb's and stash it in the drm_plane_state. This is called after * drm_atomic_helper_swap_state() so it uses the current plane state (and * just uses the atomic state to find the changed planes) + * + * Returns zero if sucess or < 0 if fence_wait() fails. */ -void drm_atomic_helper_wait_for_fences(struct drm_device *dev, - struct drm_atomic_state *state) +int drm_atomic_helper_wait_for_fences(struct drm_device *dev, + struct drm_atomic_state *state, + bool intr) { struct drm_plane *plane; struct drm_plane_state *plane_state; - int i; + int i, ret; for_each_plane_in_state(state, plane, plane_state, i) { - if (!plane->state->fence) + /* + * If the caller asks for an interruptible wait it means + * that the state were not swapped yet and the operation + * can still be interrupted by userspace, so we need + * to look to plane_state instead. + */ + if (!intr) + plane_state = plane->state; + + if (!plane_state->fence) continue; - WARN_ON(!plane->state->fb); + WARN_ON(!plane_state->fb); + + ret = fence_wait(plane_state->fence, intr); + if (ret) + return ret; - fence_wait(plane->state->fence, false); - fence_put(plane->state->fence); - plane->state->fence = NULL; + fence_put(plane_state->fence); + plane_state->fence = NULL; } + + return 0; } EXPORT_SYMBOL(drm_atomic_helper_wait_for_fences); @@ -1176,7 +1194,7 @@ static void commit_tail(struct drm_atomic_state *state) funcs = dev->mode_config.helper_private; - drm_atomic_helper_wait_for_fences(dev, state); + drm_atomic_helper_wait_for_fences(dev, state, false); drm_atomic_helper_wait_for_dependencies(state); @@ -1235,6 +1253,12 @@ int drm_atomic_helper_commit(struct drm_device *dev, if (ret) return ret; + if (!nonblock) { + ret = drm_atomic_helper_wait_for_fences(dev, state, true); + if (ret) + return ret; + } + /* * This is the point of no return - everything below never fails except * when the hw goes bonghits. Which means we can commit the new state on diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 4a8a6f1..9518e43 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -112,7 +112,7 @@ static void complete_commit(struct msm_commit *c, bool async) struct msm_drm_private *priv = dev->dev_private; struct msm_kms *kms = priv->kms; - drm_atomic_helper_wait_for_fences(dev, state); + drm_atomic_helper_wait_for_fences(dev, state, false); kms->funcs->prepare_commit(kms, state); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index d86ae5d..a42c34b 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -43,8 +43,9 @@ int drm_atomic_helper_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock); -void drm_atomic_helper_wait_for_fences(struct drm_device *dev, - struct drm_atomic_state *state); +int drm_atomic_helper_wait_for_fences(struct drm_device *dev, + struct drm_atomic_state *state, + bool intr); bool drm_atomic_helper_framebuffer_changed(struct drm_device *dev, struct drm_atomic_state *old_state, struct drm_crtc *crtc);