Re: [PATCH 11/20] drm/i915/fbc: Extract intel_fbc_max_cfb_height()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jul 10, 2024 at 08:26:54AM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> > Sent: Friday, July 5, 2024 8:23 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: intel-xe@lists.freedesktop.org
> > Subject: [PATCH 11/20] drm/i915/fbc: Extract intel_fbc_max_cfb_height()
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Pull the code to determine the maximum CFB height into a separate function. For
> > pre-HSW the maximum CFB height is the same as the maximum plane height (ie.
> > the older hardware supposedely doens't have the trick of leaving the extra lines
> > uncompressed).
> > 
> 
> Nit: Typo in supposedly and doesn't.
> 
> Limit is also altered along with refactor; would a separate patch be better ?

It doesn't actually change anything as that's just the max plane
height for older platforms, so anything taller would have been
already been rejected earlier.

I suppose I could have added the explicit limit first, and then
did the extraction. But seems harmless enough as is, so kept it
like this but adjusted the commit message a bit to highlight this
fact a little better.

> Leave it your judgement. Overall logic looks Good to me.
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 27 ++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index cf5750ed4681..47b715e5d533 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -201,17 +201,30 @@ static unsigned int intel_fbc_cfb_stride(const struct
> > intel_plane_state *plane_s
> >  	return _intel_fbc_cfb_stride(display, width, stride);  }
> > 
> > -static unsigned int intel_fbc_cfb_size(const struct intel_plane_state
> > *plane_state)
> > +/*
> > + * Maximum height the hardware will compress, on HSW+
> > + * additional lines (up to the actual plane height) will
> > + * remain uncompressed.
> > + */
> > +static unsigned int intel_fbc_max_cfb_height(struct intel_display
> > +*display)
> >  {
> > -	struct intel_display *display = to_intel_display(plane_state->uapi.plane-
> > >dev);
> > -	int height = drm_rect_height(&plane_state->uapi.src) >> 16;
> > +	struct drm_i915_private *i915 = to_i915(display->drm);
> > 
> >  	if (DISPLAY_VER(display) >= 8)
> > -		height = min(height, 2560);
> > -	else if (DISPLAY_VER(display) == 7)
> > -		height = min(height, 2048);
> > +		return 2560;
> > +	else if (DISPLAY_VER(display) >= 5 || IS_G4X(i915))
> > +		return 2048;
> > +	else
> > +		return 1536;
> > +}
> > 
> > -	return height * intel_fbc_cfb_stride(plane_state);
> > +static unsigned int intel_fbc_cfb_size(const struct intel_plane_state
> > +*plane_state) {
> > +	struct intel_display *display = to_intel_display(plane_state->uapi.plane-
> > >dev);
> > +	unsigned int height = drm_rect_height(&plane_state->uapi.src) >> 16;
> > +
> > +	return min(height, intel_fbc_max_cfb_height(display)) *
> > +		intel_fbc_cfb_stride(plane_state);
> >  }
> > 
> >  static u16 intel_fbc_override_cfb_stride(const struct intel_plane_state
> > *plane_state)
> > --
> > 2.44.2
> 

-- 
Ville Syrjälä
Intel