[Intel-gfx] [PATCH 1/4] drm/i915/fbc: Use the correct plane stride

[Ville Syrjälä]

On Mon, Jul 06, 2020 at 08:53:06PM +0000, Souza, Jose wrote:
> On Fri, 2020-07-03 at 14:38 +0300, Ville Syrjälä wrote:
> > On Thu, Jul 02, 2020 at 11:24:04PM +0000, Souza, Jose wrote:
> > > On Thu, 2020-07-02 at 18:37 +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > Consult the actual plane stride instead of the fb stride. The two
> > > > will disagree when we remap the gtt. The plane stride is what the
> > > > hw will be fed so that's what we should look at for the FBC
> > > > retrictions/cfb allocation.
> > > > 
> > > > Since we no longer require a fence we are going to attempt using
> > > > FBC with remapping, and so we should look at correct stride.
> > > > 
> > > > With 90/270 degree rotation the plane stride is stored in units
> > > > of pixels, so we need to conver it to bytes for the purposes
> > > > of calculating the cfb stride. Not entirely sure if this matches
> > > > the hw behaviour though. Need to reverse engineer that at some
> > > > point...
> > > > 
> > > > We also need to reorder the pixel format check vs. stride check
> > > > to avoid triggering a spurious WARN(stride & 63) with cpp==1 and
> > > > plane stride==32.
> > > > 
> > > > v2: Try to deal with rotated stride and related WARN
> > > > 
> > > 
> > > Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> > > 
> > > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > > Fixes: 691f7ba58d52 ("drm/i915/display/fbc: Make fences a nice-to-have for GEN9+")
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index 69a0682ddb6a..d30c2a389294 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -695,9 +695,13 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> > > >  	cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode;
> > > >  
> > > >  	cache->fb.format = fb->format;
> > > > -	cache->fb.stride = fb->pitches[0];
> > > >  	cache->fb.modifier = fb->modifier;
> > > >  
> > > > +	/* FIXME is this correct? */
> > > > +	cache->fb.stride = plane_state->color_plane[0].stride;
> > > > +	if (drm_rotation_90_or_270(plane_state->hw.rotation))
> > > 
> > > If it was wrong our CI would caught this in BDW or SNB for example.
> > 
> > Not really. Well, certainly not on pre-skl since they don't do 90/270
> > rotation. And probably not even on skl+ since any wrong assumption
> 
> Checking rotation_is_valid() GEN5 up to GEN8 allows 90/270 rotation.

The display engine on pre-skl does not support 90/270 rotaiton *at all*.

> 
> 
> > about how the cfb stride is calculated by the hw would just cause it
> > to scribble over stolen memory we didn't allocate. So unless we've
> > started to use stolen more extensively in recent times such problems
> > would probably go unnoticed.
> > 
> > > > +		cache->fb.stride *= fb->format->cpp[0];
> > > > +
> > > >  	/* FBC1 compression interval: arbitrary choice of 1 second */
> > > >  	cache->interval = drm_mode_vrefresh(&crtc_state->hw.adjusted_mode);
> > > >  
> > > > @@ -797,6 +801,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > > > +		fbc->no_fbc_reason = "pixel format is invalid";
> > > > +		return false;
> > > > +	}
> > > > +
> > > >  	if (!rotation_is_valid(dev_priv, cache->fb.format->format,
> > > >  			       cache->plane.rotation)) {
> > > >  		fbc->no_fbc_reason = "rotation unsupported";
> > > > @@ -813,11 +822,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) {
> > > > -		fbc->no_fbc_reason = "pixel format is invalid";
> > > > -		return false;
> > > > -	}
> > > > -
> > > >  	if (cache->plane.pixel_blend_mode != DRM_MODE_BLEND_PIXEL_NONE &&
> > > >  	    cache->fb.format->has_alpha) {
> > > >  		fbc->no_fbc_reason = "per-pixel alpha blending is incompatible with FBC";

-- 
Ville Syrjälä
Intel