Re: [RFC 03/15] drm/i915: Update sprite watermarks outside vblank evasion

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 20, 2015 at 07:12:15PM -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).  Ensure that we
> properly set the atomic.update_sprite_watermarks flag so that these
> updates will happen outside vblank evasion and we can drop the direct
> calls from the plane programming code.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++--
>  drivers/gpu/drm/i915/intel_sprite.c  | 22 ++++------------------
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0713258..e12b5a4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12902,10 +12902,17 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  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)
> +	    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;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 3a96956..394cf37 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -198,8 +198,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, bool force)
>  	/* Activate double buffered register update */
>  	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--;
> @@ -468,8 +462,6 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc, bool force)
>  	I915_WRITE(SPSURF(pipe, plane), 0);
>  
>  	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
> -
> -	intel_update_sprite_watermarks(dplane, crtc);
>  }
>  
>  
> @@ -534,8 +526,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--;
> @@ -671,8 +661,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--;
> @@ -921,18 +909,16 @@ finish:
>  		intel_crtc->atomic.fb_bits |=
>  			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>  
> -		if (intel_wm_need_update(plane, &state->base))
> -			intel_crtc->atomic.update_wm = true;
> +		if (intel_wm_need_update(plane, &state->base) || !state->visible)
> +			intel_crtc->atomic.update_sprite_watermarks |=
> +				(1 << drm_plane_index(plane));

I have a local kludge in my CHV WM tree now that splits this into pre
and post wm updates, and chooses one or the other based on whether the
plane is getting enabled or disabled. Of course that kind of kludge
is only needed if the proper two part watermark update isn't being done,
and I didn't want to reimplement it for CHV at this time.

Actually what I have for CHV now is starting to look quite a bit like
the ILK thing, so in the future we might be able to unify more. I
started by putting the wm state into the plane/crtc states, but those
didn't seem sane yet, so had to pull them out again.

Anyway this is all just FYI, but obviously I don't want to block progress
on the ILK stuff with any requirement to unify at this time.

>  
> -		if (!state->visible) {
> +		if (!state->visible)
>  			/*
>  			 * Avoid underruns when disabling the sprite.
>  			 * FIXME remove once watermark updates are done properly.
>  			 */
>  			intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.update_sprite_watermarks |=
> -				(1 << drm_plane_index(plane));
> -		}
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx