From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752170AbcHHJBC (ORCPT ); Mon, 8 Aug 2016 05:01:02 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33789 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbcHHJBA (ORCPT ); Mon, 8 Aug 2016 05:01:00 -0400 Date: Mon, 8 Aug 2016 11:00:56 +0200 From: Daniel Vetter To: Keith Packard Cc: Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, airlied@linux.ie Subject: Re: [PATCH] drm: Don't prepare or cleanup unchanging frame buffers [v2] Message-ID: <20160808090056.GW6232@phenom.ffwll.local> Mail-Followup-To: Keith Packard , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, airlied@linux.ie References: <1469952534-6516-1-git-send-email-keithp@keithp.com> <20160802135417.GO6232@phenom.ffwll.local> <86a8gpkv4e.fsf@hiro.keithp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <86a8gpkv4e.fsf@hiro.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 Sat, Aug 06, 2016 at 10:26:09PM -0700, Keith Packard wrote: > Daniel Vetter writes: > > > 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. > > Ok, here's pretty much the previous version, which works now that I've > fixed the intel driver. Instead of just comparing the fb's, I'm using > the framebuffer_changed function, which seems like a nice bit of > documentation if nothing else. > > From a75251d5762b1c200ed2f3dca2a5b00cc85bea95 Mon Sep 17 00:00:00 2001 > From: Keith Packard > Date: Sat, 4 Jun 2016 01:16:22 -0700 > Subject: [PATCH] drm: Don't prepare or cleanup unchanging frame buffers [v3] > > 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. > > v3: use drm_atomic_helper_framebuffer_changed in both prepare and > cleanup phases instead of keeping state in the plane. > > cc: dri-devel@lists.freedesktop.org > cc: David Airlie > cc: Daniel Vetter > Signed-off-by: Keith Packard Rebased onto 4.8 while applying, which did simplify the patch a lot (4.8 is already using for_each_plane_in_state, but slightly differently). Thanks, Daniel > --- > drivers/gpu/drm/drm_atomic_helper.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ddfa0d1..72e50bc 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1246,18 +1246,19 @@ 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; > + int max_prepared_i = 0; > > - 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) > + if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc)) > continue; > > funcs = plane->helper_private; > @@ -1267,24 +1268,25 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, > if (ret) > goto fail; > } > + max_prepared_i = i; > } > > 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 (i > max_prepared_i) > + break; > + > + if (!drm_atomic_helper_framebuffer_changed(dev, state, plane_state->crtc)) > continue; > > funcs = plane->helper_private; > > if (funcs->cleanup_fb) > funcs->cleanup_fb(plane, plane_state); > - > } > > return ret; > @@ -1527,6 +1529,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 (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc)) > + continue; > + > funcs = plane->helper_private; > > if (funcs->cleanup_fb) > -- > 2.8.1 > > > -- > -keith -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch