[Intel-gfx] [PATCH v2 1/3] drm/i915/display/fbc: Make fences a nice-to-have for GEN11+

Fri Feb 7 00:55:41 UTC 2020

On Thu, 2020-02-06 at 15:46 +0200, Ville Syrjälä wrote:
> On Wed, Feb 05, 2020 at 06:08:49PM -0800, José Roberto de Souza
> wrote:
> > dGFX have local memory so it do not have aperture and do not
> > support
> > CPU fences but even for iGFX it have a small number of fences.
> > 
> > As replacement for fences to track frontbuffer modifications by CPU
> > we have a software tracking that is already in used by FBC and PSR.
> > PSR don't support fences so it shows that this tracking is
> > reliable.
> > 
> > So lets make fences a nice-to-have to activate FBC for GEN11+, this
> > will allow us to enable FBC for dGFXs and iGFXs even when there is
> > no
> > available fence.
> > 
> > We do not set fences to rotated planes but FBC only have
> > restrictions
> > against 16bpp, so adding it here.
> > 
> > Also adding a new check for the tiling format, fences are only set
> > to X and Y tiled planes but again FBC don't have any restrictions
> > against tiling so adding linear as supported as well, other formats
> > should be added after tested but IGT only supports drawing in thse
> > 3 formats.
> > 
> > intel_fbc_hw_tracking_covers_screen() maybe can also have the same
> > treatment as fences but BSpec is not clear if the size limitation
> > is
> > for hardware tracking or general use of FBC and I don't have a 5K
> > display to test it, so keeping as is for safety.
> > 
> > v2:
> > - Added tiling and pixel format rotation checks
> > - Changed the GEN version not requiring fences to 11 from 9, DDX
> > needs some changes but it don't have support for GEN11+
> 
> It's already borked, so shouldn't actually make any difference.
> So IMO just make it gen9+.

Okay changing back to GEN9

> 
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 42
> > ++++++++++++++++++++----
> >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> >  2 files changed, 36 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index ddf8d3bb7a7d..3a9e41e93ebf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -585,7 +585,7 @@ static bool stride_is_valid(struct
> > drm_i915_private *dev_priv,
> >  }
> >  
> >  static bool pixel_format_is_valid(struct drm_i915_private
> > *dev_priv,
> > -				  u32 pixel_format)
> > +				  u32 pixel_format, unsigned int
> > rotation)
> >  {
> >  	switch (pixel_format) {
> >  	case DRM_FORMAT_XRGB8888:
> > @@ -599,6 +599,9 @@ static bool pixel_format_is_valid(struct
> > drm_i915_private *dev_priv,
> >  		/* WaFbcOnly1to1Ratio:ctg */
> >  		if (IS_G4X(dev_priv))
> >  			return false;
> > +		if ((rotation & (DRM_MODE_ROTATE_90 |
> > DRM_MODE_ROTATE_270)) &&
> 
> There is a function for that. Also wrong place for the check.

We have this check:

if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
    cache->plane.rotation != DRM_MODE_ROTATE_0) {
	fbc->no_fbc_reason = "rotation unsupported";
	return false;
}

I put in the pixel_format_is_valid() because only 16bpp formats do not
support this rotations.

> 
> > +		    INTEL_GEN(dev_priv) >= 9)
> 
> Pointless gen check. Older hw doesn't do 90/270 degree rotation
> anyway.

GEN7 and GEN8 don't have this restrictions on BSpec but anyways we do
not put fence in a surface if it is rotate so platforms older than GEN9
will not even reach to this check.

> 
> I *think* it should actually be possible to use FBC with 90/270
> degree
> rotation. The only complication was the host tracking but if we don't
> think we need it anwyay then rotation also should work. But that is
> material for a followup.
> 
> > +			return false;
> >  		return true;
> >  	default:
> >  		return false;
> > @@ -639,6 +642,18 @@ static bool
> > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> >  	return effective_w <= max_w && effective_h <= max_h;
> >  }
> >  
> > +static bool tiling_is_valid(uint64_t modifier)
> > +{
> > +	switch (modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +	case I915_FORMAT_MOD_X_TILED:
> > +	case I915_FORMAT_MOD_Y_TILED:
> > +		return true;
> 
> There should be a gen check here.

Okay moving from the caller to here.

> 
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
> >  					 const struct intel_crtc_state
> > *crtc_state,
> >  					 const struct intel_plane_state
> > *plane_state)
> > @@ -672,6 +687,7 @@ static void intel_fbc_update_state_cache(struct
> > intel_crtc *crtc,
> >  
> >  	cache->fb.format = fb->format;
> >  	cache->fb.stride = fb->pitches[0];
> > +	cache->fb.modifier = fb->modifier;
> >  
> >  	drm_WARN_ON(&dev_priv->drm, plane_state->flags &
> > PLANE_HAS_FENCE &&
> >  		    !plane_state->vma->fence);
> > @@ -720,23 +736,34 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > -	/* The use of a CPU fence is mandatory in order to detect
> > writes
> > -	 * by the CPU to the scanout and trigger updates to the FBC.
> > +	/* The use of a CPU fence is one of two ways to detect writes
> > by the
> > +	 * CPU to the scanout and trigger updates to the FBC.
> > +	 *
> > +	 * The other method is by software tracking(see
> > +	 * intel_fbc_invalidate/flush()), it will manually notify FBC
> > and nuke
> > +	 * the current compressed buffer and recompress it.
> >  	 *
> >  	 * Note that is possible for a tiled surface to be unmappable
> > (and
> > -	 * so have no fence associated with it) due to aperture
> > constaints
> > +	 * so have no fence associated with it) due to aperture
> > constraints
> >  	 * at the time of pinning.
> >  	 *
> >  	 * FIXME with 90/270 degree rotation we should use the fence on
> >  	 * the normal GTT view (the rotated view doesn't even have a
> >  	 * fence). Would need changes to the FBC fence Y offset as
> > well.
> > -	 * For now this will effecively disable FBC with 90/270 degree
> > +	 * For now this will effectively disable FBC with 90/270 degree
> >  	 * rotation.
> >  	 */
> > -	if (cache->fence_id < 0) {
> > +	if (INTEL_GEN(dev_priv) < 11 && cache->fence_id < 0) {
> >  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> >  		return false;
> >  	}
> > +
> > +	/* Only check tiling for platforms that fence is not mandatory
> > */
> > +	if (INTEL_GEN(dev_priv) >= 11 && !tiling_is_valid(cache-
> > >fb.modifier)) {
> > +		fbc->no_fbc_reason = "tiling unsupported";
> > +		return false;
> > +	}
> > +
> >  	if (INTEL_GEN(dev_priv) <= 4 && !IS_G4X(dev_priv) &&
> >  	    cache->plane.rotation != DRM_MODE_ROTATE_0) {
> >  		fbc->no_fbc_reason = "rotation unsupported";
> > @@ -748,7 +775,8 @@ static bool intel_fbc_can_activate(struct
> > intel_crtc *crtc)
> >  		return false;
> >  	}
> >  
> > -	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format)) 
> > {
> > +	if (!pixel_format_is_valid(dev_priv, cache->fb.format->format,
> > +				   cache->plane.rotation)) {
> >  		fbc->no_fbc_reason = "pixel format is invalid";
> >  		return false;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3452926d7b77..a481a0454e69 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -413,6 +413,7 @@ struct intel_fbc {
> >  		struct {
> >  			const struct drm_format_info *format;
> >  			unsigned int stride;
> > +			uint64_t modifier;
> >  		} fb;
> >  		u16 gen9_wa_cfb_stride;
> >  		s8 fence_id;
> > -- 
> > 2.25.0