Re: [PATCH 2/2] drm/i915: Update sprite watermarks outside vblank evasion

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 22, 2015 at 06:30:33PM -0700, Matt Roper wrote:
> We never removed the sprite watermark updates from our low-level
> foo_update_plane() functions; since our hardware updates happen under
> vblank evasion, we're not supposed to be calling potentially sleeping
> functions there (since interrupts are disabled).  We do already have a
> mechanism that's supposed to be used to update sprite watermarks in the
> post-evasion function intel_post_plane_update(), but at the moment it's
> only being used for updates caused by plane disables.
> 
> To fix this oversight we need to make a few changes:
> 
>  * When intel_plane_atomic_calc_changes() calls intel_wm_need_update()
>    to determine whether watermarks need an update, we need to set the
>    'atomic.update_sprite_watermarks' flag rather than the
>    'atomic.update_wm' flag when the plane in question is a sprite.  Some
>    platforms don't care about this change since the two types of update
>    do the same thing, but some platforms (e.g., ILK-style watermarks)
>    need to specifically use the sprite update function to update cached
>    watermark parameters.
> 
>  * intel_wm_need_update() needs to also look for plane size changes
>    (previously it only checked tiling & rotation).
> 
> With the above changes, the need for sprite watermark updates should be
> properly flagged at atomic 'check' time and handled at 'commit' time in
> post-evasion, so we can drop the direct calls to
> intel_update_sprite_watermarks() from all of the
> intel_plane->update_plane() handlers.  We'll also toss a
> WARN_ON(irqs_disabled()) into the watermark update functions to avoid
> such mistakes in the future.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Why do even still need a separate sprite wm update hooks? Aren't we yet
recomputing wm values for all planes if something changes, then diffing
them with the current one and updating if anything changed? That seems to
be what update_sprite_wm essentially does, except it's pre-atomic and
open-codes the updates ...

Ofc there's still that ilk w/a around which needs to be moved to a
suitable place.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_pm.c      |  2 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 12 ------------
>  3 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e8e01c..181aedd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11575,13 +11575,17 @@ 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 changes 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)
> +	    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))
>  		return true;
>  
>  	return false;
> @@ -11645,8 +11649,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			 plane->base.id, was_visible, visible,
>  			 turn_off, turn_on, mode_changed);
>  
> -	if (intel_wm_need_update(plane, plane_state))
> -		intel_crtc->atomic.update_wm = true;
> +	if (intel_wm_need_update(plane, plane_state)) {
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> +			intel_crtc->atomic.update_sprite_watermarks |=
> +				1 << i;
> +		else
> +			intel_crtc->atomic.update_wm = true;
> +	}
>  
>  	switch (plane->type) {
>  	case DRM_PLANE_TYPE_PRIMARY:
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4d3cb70..2470ede 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3750,6 +3750,7 @@ void intel_update_watermarks(struct drm_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  
> +	WARN_ON(irqs_disabled());
>  	if (dev_priv->display.update_wm)
>  		dev_priv->display.update_wm(crtc);
>  }
> @@ -3770,6 +3771,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
>  	unsigned int dst_h = drm_rect_height(&state->dst);
>  	bool scaled = (src_w != dst_w || src_h != dst_h);
>  
> +	WARN_ON(irqs_disabled());
>  	if (dev_priv->display.update_sprite_wm)
>  		dev_priv->display.update_sprite_wm(plane, crtc,
>  						   sprite_width, sprite_height,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index b627067..ce2fa92 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -199,8 +199,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	rotation = drm_plane->state->rotation;
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	intel_update_sprite_watermarks(drm_plane, crtc);
> -
>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  					       fb->pixel_format);
>  
> @@ -282,8 +280,6 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
> -
> -	intel_update_sprite_watermarks(dplane, crtc);
>  }
>  
>  static void
> @@ -399,8 +395,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	if (obj->tiling_mode != I915_TILING_NONE)
>  		sprctl |= SP_TILED;
>  
> -	intel_update_sprite_watermarks(dplane, crtc);
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -465,8 +459,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  
>  	I915_WRITE(SPSURF(pipe, plane), 0);
>  	POSTING_READ(SPSURF(pipe, plane));
> -
> -	intel_update_sprite_watermarks(dplane, crtc);
>  }
>  
>  static void
> @@ -530,8 +522,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
>  
> -	intel_update_sprite_watermarks(plane, crtc);
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -665,8 +655,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	if (IS_GEN6(dev))
>  		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
>  
> -	intel_update_sprite_watermarks(plane, crtc);
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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