| Message ID | 20180405154449.23038-8-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel Vetter <daniel.vetter@ffwll.ch> writes: > I've done a lot of history digging. The first signs of this > optimization was introduced in i915: I can't come up with any reason this would matter. I almost came up with "You're doing tearing X11 front buffer rendering, and you do a modeset using the same fb, and so you block that modeset behind your rendering." Except that: 1) who cares 2) this helper is only for dma-bufs, not normal X11 rendering 3) your X11 driver should be doing pageflipping to be tear-free anyway, let's just fix that[1]. Reviewed-by: Eric Anholt <eric@anholt.net> [1] This is not actually me volunteering myself or anyone else to go fix that.
On Fri, Apr 20, 2018 at 03:11:24PM -0700, Eric Anholt wrote: > Daniel Vetter <daniel.vetter@ffwll.ch> writes: > > > I've done a lot of history digging. The first signs of this > > optimization was introduced in i915: > > I can't come up with any reason this would matter. I almost came up > with "You're doing tearing X11 front buffer rendering, and you do a > modeset using the same fb, and so you block that modeset behind your > rendering." Except that: > > 1) who cares > 2) this helper is only for dma-bufs, not normal X11 rendering > 3) your X11 driver should be doing pageflipping to be tear-free anyway, > let's just fix that[1]. > > Reviewed-by: Eric Anholt <eric@anholt.net> > > [1] This is not actually me volunteering myself or anyone else to go fix > that. Ok, merged everything except the final three patches. I'll poke Rob for a proper ack first before doing that. There's not much point in trying to unify behaviour if there's still a driver doing things differently :-) Thanks very much for your review. -Daniel
On Tue, Apr 24, 2018 at 02:04:10PM +0200, Daniel Vetter wrote: > On Fri, Apr 20, 2018 at 03:11:24PM -0700, Eric Anholt wrote: > > Daniel Vetter <daniel.vetter@ffwll.ch> writes: > > > > > I've done a lot of history digging. The first signs of this > > > optimization was introduced in i915: > > > > I can't come up with any reason this would matter. I almost came up > > with "You're doing tearing X11 front buffer rendering, and you do a > > modeset using the same fb, and so you block that modeset behind your > > rendering." Except that: > > > > 1) who cares > > 2) this helper is only for dma-bufs, not normal X11 rendering > > 3) your X11 driver should be doing pageflipping to be tear-free anyway, > > let's just fix that[1]. > > > > Reviewed-by: Eric Anholt <eric@anholt.net> > > > > [1] This is not actually me volunteering myself or anyone else to go fix > > that. > > Ok, merged everything except the final three patches. I'll poke Rob for a > proper ack first before doing that. There's not much point in trying to > unify behaviour if there's still a driver doing things differently :-) > > Thanks very much for your review. Ok with the msm conversion to atomic commit helpers msm aligned to what we're doing here without my patch. I pulled in the remaining to for 4.19. -Daniel
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index acfbc0641a06..2810d4131411 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -253,7 +253,7 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane, struct dma_buf *dma_buf; struct dma_fence *fence; - if (plane->state->fb == state->fb || !state->fb) + if (!state->fb) return 0; dma_buf = drm_gem_fb_get_obj(state->fb, 0)->dma_buf;
I've done a lot of history digging. The first signs of this optimization was introduced in i915: commit 25067bfc060d1a481584dcb51ef4b5680176ecb6 Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Date: Wed Sep 10 12:03:17 2014 -0300 drm/i915: pin sprite fb only if it changed without much justification. Pinning already pinned stuff is real cheap (it's just obj->pin_count++ really), and the missing implicit sync was entirely forgotten about it seems. It's at least not mentioned anywhere it the commit message. It was also promptly removed shortly afterwards in commit ea2c67bb4affa84080c616920f3899f123786e56 Author: Matt Roper <matthew.d.roper@intel.com> Date: Tue Dec 23 10:41:52 2014 -0800 drm/i915: Move to atomic plane helpers (v9) again without really mentioning the side-effect that plane updates with the same fb now again obey implicit syncing. Note that this only ever applied to the plane_update hook, all other legacy entry points (set_base, page_flip) always obeyed implicit sync in the drm/i915 driver. The real source of this code here seems to be msm, copied to vc4, then copied to tinydrm. I've also tried to dig around in all available msm sources, but the corresponding check for fb != old_fb is present ever since the initial merge in commit cf3a7e4ce08e6876cdcb80390876647f28a7cf8f Author: Rob Clark <robdclark@gmail.com> Date: Sat Nov 8 13:21:06 2014 -0500 drm/msm: atomic core bits The only older version I've found of msm atomic code predates the atomic helpers, and so didn't even use any of this. It also does not have a corresponding check (because it simply did no implicit sync at all). I've chatted with Rob on irc, and he didn't remember the reason for this either. Note we had epic amounts of fun with too much syncing against _vblank_, especially around cursor updates. But I don't ever discussing a need for less syncing against implicit fences. Also note that explicit fencing allows you to sidetrack all of this, at least for all the drivers correctly implemented using drm_atomic_set_fence_for_plane(). Given that it seems to be an accident of history, and that big drivers like i915 (and also nouveau it seems, I didn't follow the amdgpu/radeon sync code to figure this out properly there) never have done it, let's remove this. Cc: Rob Clark <robdclark@gmail.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: David Airlie <airlied@linux.ie> Cc: "Noralf Trønnes" <noralf@tronnes.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)