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

[Egbert Eich <eich@freedesktop.org>]

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?

Cheers,
	Egbert.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx