| Message ID | 1469952534-6516-1-git-send-email-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 31, 2016 at 01:08:54AM -0700, Keith Packard wrote: > When reconfiguring a plane position (as in moving the cursor), the > frame buffer for the cursor isn't changing, so don't call the prepare > or cleanup driver functions. > > This avoids making cursor position updates block on all pending rendering. Oops, I thought we've had this, but nope :( > v2: Track which planes have been prepared to know which to > cleanup. Otherwise, failure paths and success paths would need > different tests in the cleanup code as the plane state points to > different places in the two cases. > > cc: dri-devel@lists.freedesktop.org > cc: David Airlie <airlied@linux.ie> > Signed-off-by: Keith Packard <keithp@keithp.com> Hm, I can't see v1 anywhere, but I think it'd be better. You can't store any transient state related to the current update in struct drm_plane. In this case the cleanup_buffers from a previous update might overlap (for nonblocking atomic commits) with the prepare_planes for the next one. Either we need special cleanup vs. error-path code, or some flag somewhere in the drm_plane_state. btw if you go with the flag, vc4 could be cleaned up to use that too. At least I thought Eric hand-rolled this logic in there somewhere, can't find it quickly right now. Another bit worth considering: We could use this "has the plane switched" instead ->legacy_cursor_update in drm_atomic_helper_wait_for_vblanks - some drivers have iommus which get really angry when we unpin too early, and your patch alone might be good enough. -Daniel > --- > drivers/gpu/drm/drm_atomic_helper.c | 23 +++++++++++++---------- > include/drm/drm_crtc.h | 1 + > 2 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ddfa0d1..f7f3a51 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1246,18 +1246,20 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); > * Returns: > * 0 on success, negative error code on failure. > */ > + > int drm_atomic_helper_prepare_planes(struct drm_device *dev, > struct drm_atomic_state *state) > { > - int nplanes = dev->mode_config.num_total_plane; > + struct drm_plane *plane; > + struct drm_plane_state *plane_state; > int ret, i; > > - for (i = 0; i < nplanes; i++) { > + for_each_plane_in_state(state, plane, plane_state, i) { > const struct drm_plane_helper_funcs *funcs; > - struct drm_plane *plane = state->planes[i]; > - struct drm_plane_state *plane_state = state->plane_states[i]; > > - if (!plane) > + plane->prepared = false; > + > + if (plane->state->fb == plane_state->fb) > continue; > > funcs = plane->helper_private; > @@ -1267,24 +1269,22 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, > if (ret) > goto fail; > } > + plane->prepared = true; > } > > return 0; > > fail: > - for (i--; i >= 0; i--) { > + for_each_plane_in_state(state, plane, plane_state, i) { > const struct drm_plane_helper_funcs *funcs; > - struct drm_plane *plane = state->planes[i]; > - struct drm_plane_state *plane_state = state->plane_states[i]; > > - if (!plane) > + if (!plane->prepared) > continue; > > funcs = plane->helper_private; > > if (funcs->cleanup_fb) > funcs->cleanup_fb(plane, plane_state); > - > } > > return ret; > @@ -1527,6 +1527,9 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, > for_each_plane_in_state(old_state, plane, plane_state, i) { > const struct drm_plane_helper_funcs *funcs; > > + if (!plane->prepared) > + continue; > + > funcs = plane->helper_private; > > if (funcs->cleanup_fb) > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index d1559cd..08b2033 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1531,6 +1531,7 @@ struct drm_plane { > uint32_t *format_types; > unsigned int format_count; > bool format_default; > + bool prepared; > > struct drm_crtc *crtc; > struct drm_framebuffer *fb; > -- > 2.8.1 > > _______________________________________________ > 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 ddfa0d1..f7f3a51 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1246,18 +1246,20 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); * Returns: * 0 on success, negative error code on failure. */ + int drm_atomic_helper_prepare_planes(struct drm_device *dev, struct drm_atomic_state *state) { - int nplanes = dev->mode_config.num_total_plane; + struct drm_plane *plane; + struct drm_plane_state *plane_state; int ret, i; - for (i = 0; i < nplanes; i++) { + for_each_plane_in_state(state, plane, plane_state, i) { const struct drm_plane_helper_funcs *funcs; - struct drm_plane *plane = state->planes[i]; - struct drm_plane_state *plane_state = state->plane_states[i]; - if (!plane) + plane->prepared = false; + + if (plane->state->fb == plane_state->fb) continue; funcs = plane->helper_private; @@ -1267,24 +1269,22 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, if (ret) goto fail; } + plane->prepared = true; } return 0; fail: - for (i--; i >= 0; i--) { + for_each_plane_in_state(state, plane, plane_state, i) { const struct drm_plane_helper_funcs *funcs; - struct drm_plane *plane = state->planes[i]; - struct drm_plane_state *plane_state = state->plane_states[i]; - if (!plane) + if (!plane->prepared) continue; funcs = plane->helper_private; if (funcs->cleanup_fb) funcs->cleanup_fb(plane, plane_state); - } return ret; @@ -1527,6 +1527,9 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, for_each_plane_in_state(old_state, plane, plane_state, i) { const struct drm_plane_helper_funcs *funcs; + if (!plane->prepared) + continue; + funcs = plane->helper_private; if (funcs->cleanup_fb) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d1559cd..08b2033 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1531,6 +1531,7 @@ struct drm_plane { uint32_t *format_types; unsigned int format_count; bool format_default; + bool prepared; struct drm_crtc *crtc; struct drm_framebuffer *fb;
When reconfiguring a plane position (as in moving the cursor), the frame buffer for the cursor isn't changing, so don't call the prepare or cleanup driver functions. This avoids making cursor position updates block on all pending rendering. v2: Track which planes have been prepared to know which to cleanup. Otherwise, failure paths and success paths would need different tests in the cleanup code as the plane state points to different places in the two cases. cc: dri-devel@lists.freedesktop.org cc: David Airlie <airlied@linux.ie> Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/gpu/drm/drm_atomic_helper.c | 23 +++++++++++++---------- include/drm/drm_crtc.h | 1 + 2 files changed, 14 insertions(+), 10 deletions(-)