Re: [RFC 3/8] drm/i915/ivb: Move WaCxSRDisabledForSpriteScaling w/a to atomic check

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 02, 2015 at 06:03:57PM +0200, Egbert Eich wrote:
> Regarding commit 7809e5ae35b9d8d0710f0874b2e3f10be144e38b
> 
> On Wed, Jul 01, 2015 at 07:25:56PM -0700, Matt Roper wrote:
> > Determine whether we need to apply this workaround at atomic check time
> > and just set a flag that will be used by the main watermark update
> > routine.
> > 
> > Moving this workaround into the atomic framework reduces
> > ilk_update_sprite_wm() to just a standard watermark update, so drop it
> > completely and just ensure that ilk_update_wm() is called whenever a
> > sprite plane is updated in a way that would affect watermarks.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> This patch causes intel_update_watermarks() to be called much more
> frequently although the watermark values don't change. 
> The change responsible for this is:
> 
> > index 5de1ded..46ef981 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11560,23 +11560,38 @@ retry:
> >  static bool intel_wm_need_update(struct drm_plane *plane,
> >  				 struct drm_plane_state *state)
> >  {
> > -	/* Update watermarks on tiling changes. */
> > +	struct intel_plane_state *new = to_intel_plane_state(state);
> > +	struct intel_plane_state *cur = to_intel_plane_state(plane->state);
> > +
> > +	/* Update watermarks on tiling or size changes. */
> >  	if (!plane->state->fb || !state->fb ||
> >  	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
> > -	    plane->state->rotation != state->rotation)
> > -		return true;
> > -
> 
> > -	if (plane->state->crtc_w != state->crtc_w)
> 
> A quick look thru intel_pm.c reveals that this is  relevant for
> WM caluclations for gen=4 and any chipsets for which is_g4x is true.
> Should this really be removed?
> 
> > +	    plane->state->rotation != state->rotation ||
> 
> > +	    drm_rect_width(&new->src) != drm_rect_width(&cur->src) ||
> > +	    drm_rect_height(&new->src) != drm_rect_height(&cur->src) ||
> > +	    drm_rect_width(&new->dst) != drm_rect_width(&cur->dst) ||
> > +	    drm_rect_height(&new->dst) != drm_rect_height(&cur->dst))
> 
> these values seem to be used only for watermark calculations on gen < 9 when
> HAS_PCH_SPLIT() is true.
> 
> Still these are responsible for most of the watermark recalculations (when the mouse
> cursor is moved towards the edge for example). On the system I'm looking at the moment
> (Q35) changes in these values don't change the WMs.
> 
> Since WM calculation is very chip generation specific and differs considerably between
> generations, wouldn't it make sense to either have chipset specific functions to determin
> whether a recalculation is needed - or even perfrom this in the update_wm() function
> itself?

The chipset-specific functions to recalc wm values should check for any
real changes and no-op out if none of the registers have changed. The idea
is that wm calculations are really complex and trying to keep both the "do
we need to recalc" and the actual calculations perfectly in sync is a hard
problem. Hence why we recalc aggressively to avoid hard-to-find bugs where
wm values are stale. So overhead should be just a bit of wasted cpu time.
Do you see bad things happening because of all these recalculations?

I guess if it's real trouble we can make a special case for cursors on
pre-gen9 on plane window changes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx