[PATCH] drm/i915/fbc: Allow FBC with CCS modifiers on SKL+

[Shankar, Uma]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, January 24, 2024 9:13 AM
> To: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Cc: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/fbc: Allow FBC with CCS modifiers on SKL+
> 
> On Tue, Jan 23, 2024 at 05:44:06PM -0500, Rodrigo Vivi wrote:
> > On Tue, Jan 23, 2024 at 11:02:44AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > Only display workarounds 0391 and 0475 call for disabling FBC with
> > > render compression, and those are listed only for pre-prod SKL
> > > steppings. So it should be safe to enable
> > > FB+CCS on production hardware.
> > >
> > > AFAIK CCS is limited to 50% bandwidth reduction (perhaps clear color
> > > can do better?). FBC can exceed that number by quite a bit, given
> > > the right kind of framebuffer contents. So piling on both kinds of
> > > compressions could still make sense.
> >
> > yeap, I think so.
> > The risk is to hit a workaround that is not ducumented in the BSpec
> > cases after gen11...
> >
> > Uma, do you recall having seen lately any workaround with FBC and
> > render compression?

Sorry missed to reply earlier. 
I don't see any restriction called out on this, so we should be good enabling
them both.

Let's enable and uncover bugs if any 😊 (have been a touchy area anyways)

Regards,
Uma Shankar

> > >
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10125
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 13 +------------
> > >  1 file changed, 1 insertion(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index f17a1afb4929..b453fcbd67da 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -1087,18 +1087,7 @@ static bool i8xx_fbc_tiling_valid(const
> > > struct intel_plane_state *plane_state)
> > >
> > >  static bool skl_fbc_tiling_valid(const struct intel_plane_state
> > > *plane_state)  {
> > > -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > -
> > > -	switch (fb->modifier) {
> > > -	case DRM_FORMAT_MOD_LINEAR:
> > > -	case I915_FORMAT_MOD_Y_TILED:
> > > -	case I915_FORMAT_MOD_Yf_TILED:
> > > -	case I915_FORMAT_MOD_4_TILED:
> > > -	case I915_FORMAT_MOD_X_TILED:
> > > -		return true;
> > > -	default:
> > > -		return false;
> > > -	}
> > > +	return true;
> >
> > we could also simply kill this function... the compiler does the right
> > thing, but users navigating on the code needs to do an extra
> > ctag/cscope inspections to see that it is a simple return.  But well,
> > the code do gets prettier with the function :)
> 
> I've been thinking of converting a bunch of this stuff to vfuncs, so keeping the
> function around in anticipation of that seemed semi-reasonable.
> 
> > So, up to you:
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> 
> Thanks.
> 
> >
> > >  }
> > >
> > >  static bool tiling_is_valid(const struct intel_plane_state
> > > *plane_state)
> > > --
> > > 2.43.0
> > >
> 
> --
> Ville Syrjälä
> Intel