From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967268AbcHBOIe (ORCPT ); Tue, 2 Aug 2016 10:08:34 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34513 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967088AbcHBOIB (ORCPT ); Tue, 2 Aug 2016 10:08:01 -0400 Date: Tue, 2 Aug 2016 15:54:17 +0200 From: Daniel Vetter To: Keith Packard Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm: Don't prepare or cleanup unchanging frame buffers [v2] Message-ID: <20160802135417.GO6232@phenom.ffwll.local> Mail-Followup-To: Keith Packard , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org References: <1469952534-6516-1-git-send-email-keithp@keithp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469952534-6516-1-git-send-email-keithp@keithp.com> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Keith Packard 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch