Re: [PATCH 6/7] drm/i915/skl: Update watermarks for Y tiling

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 25, 2015 at 10:50:04AM +0000, Tvrtko Ursulin wrote:
> On 02/24/2015 09:42 PM, Daniel Vetter wrote:
> >On Mon, Feb 23, 2015 at 03:56:00PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Display watermarks need different programming for different tiling
> >>modes.
> >>
> >>Set the relevant flag so this happens during the plane commit and
> >>add relevant data into a structure made available to the watermark
> >>computation code.
> >>
> >>v2: Pass in tiling info to sprite plane updates as well.
> >>v3: Rebased for plane handling changes.
> >>v4: Handle fb == NULL when plane is disabled.
> >>v5: Refactored for addfb2 interface.
> >>v6: Refactored for fb modifier changes.
> >>v7: Updated for atomic commit by only updating watermarks when tiling changes.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Acked-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>Acked-by: Matt Roper <matthew.d.roper@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c |  6 ++++++
> >>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>  drivers/gpu/drm/i915/intel_pm.c      | 33 ++++++++++++++++++++++++++++-----
> >>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++++
> >>  4 files changed, 41 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index c622b11..74d4923 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -11985,6 +11985,12 @@ intel_check_primary_plane(struct drm_plane *plane,
> >>  			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> >>
> >>  		intel_crtc->atomic.update_fbc = true;
> >>+
> >>+		/* Update watermarks on tiling changes. */
> >>+		if (!plane->state->fb || !state->base.fb ||
> >>+		    plane->state->fb->modifier[0] !=
> >>+		    state->base.fb->modifier[0])
> >>+			intel_crtc->atomic.update_wm = true;
> >
> >Imo this is fragile. We should unconditionally recomupte watermarks imo
> >and just ellide the hw update if nothing changed. Spending a few cpu
> >cycles on this per frame shouldn't hurt, and it will avoid duplicated
> >logic and checks for watermark recomputation splattered all over the code.
> >
> >I know that it's kinda there already, but still minimal enough to patch
> >up. But if Matt knows that this will get removed again with the two-stage
> >wm code then I'm ok with leaving it in, with a FIXME comment added.
> 
> "Splattered all over the code" equals two places going to one when primary
> and sprite planes are unified.

Yeah splattered was over the top, it's really just the duplication between
the implicit "wm state changed because values are different" and what we
think all affects wm state. Someone somewhere will forgot to double-check
this and then suddenly some pageflip with different fb formats fails with
underruns. But since everyone just tests with full modeset that never gets
noticed in testing.

> I wouldn't wish for this to become an unbounded task since I am not sure it
> is not good enough as it is. I checked with Matt and Ander and they think it
> is good enough for now.
> 
> Not sure if FIXME is warranted here - to say what? That it will be
> refactored with the two-stage wm rewrite?

I guess I could smash a fixme comment patch on top so I don't forget. If
Ander&Matt are ok with this until full two-stage wm update has landed I'm
ok too.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx