[Intel-gfx] [PATCH 2/2] drm/i915: Update sprite watermarks outside vblank evasion

Daniel Vetter daniel at ffwll.ch
Tue Jun 23 08:24:58 PDT 2015


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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the Intel-gfx mailing list