[2/2] drm/i915: Update sprite watermarks outside vblank evasion
diff mbox

Message ID 1435023033-18043-3-git-send-email-matthew.d.roper@intel.com
State New
Headers show

Commit Message

Matt Roper June 23, 2015, 1:30 a.m. UTC
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>
---
 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(-)

Comments

Daniel Vetter June 23, 2015, 7:34 a.m. UTC | #1
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
Matt Roper June 23, 2015, 2:29 p.m. UTC | #2
On Tue, Jun 23, 2015 at 09:34:50AM +0200, Daniel Vetter wrote:
> 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 ...

Some platforms stage a copy of plane/pipe state in 'wm parameter'
structures and only update the pipe wm params with the new plane values
when update_sprite_wm is called.  That's definitely something my main
watermark series kills off, but until we get to that point and remove
the extra state staging in 'params' I think we still need to make sure
the update_sprite_wm is called appropriately to make sure watermarks use
the right values on all platforms.  I figured these two patches were a
quick fix for a known bug while we get the main watermark series
hammered out.


Matt

> 
> 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
Daniel Vetter June 23, 2015, 3:24 p.m. UTC | #3
On Tue, Jun 23, 2015 at 07:29:58AM -0700, Matt Roper wrote:
> On Tue, Jun 23, 2015 at 09:34:50AM +0200, Daniel Vetter wrote:
> > 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 ...
> 
> Some platforms stage a copy of plane/pipe state in 'wm parameter'
> structures and only update the pipe wm params with the new plane values
> when update_sprite_wm is called.  That's definitely something my main
> watermark series kills off, but until we get to that point and remove
> the extra state staging in 'params' I think we still need to make sure
> the update_sprite_wm is called appropriately to make sure watermarks use
> the right values on all platforms.  I figured these two patches were a
> quick fix for a known bug while we get the main watermark series
> hammered out.

Oh I thought that update_wm hooks already walk over all planes for all
platforms and so if we call that for all (atomic) modeset we should be
covered. I did see that update_sprite_wm do update some of those cached
values before calling intel_update_wm though and so wondered whether we
still need this all. Sounds like we still do :(
-Daniel

> 
> 
> Matt
> 
> > 
> > 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
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

Patch
diff mbox

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