Re: [PATCH i-g-t 2/4] lib/i915/fbc: add fbc frame size check helper

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Fri, 2024-06-07 at 20:42 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Vinod Govindapillai
> Sent: Friday, June 7, 2024 7:58 AM
> To: igt-dev@lists.freedesktop.org
> Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Heikkila, Juha-pekka
> <juha-pekka.heikkila@intel.com>; Hogander, Jouni <jouni.hogander@intel.com>
> Subject: [PATCH i-g-t 2/4] lib/i915/fbc: add fbc frame size check helper
> > 
> > Add a helper function to check maximum plane size fbc can be
> > supported in a display version.
> > 
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  lib/i915/intel_fbc.c | 36 ++++++++++++++++++++++++++++++++++++
> >  lib/i915/intel_fbc.h |  1 +
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/lib/i915/intel_fbc.c b/lib/i915/intel_fbc.c
> > index 07ed7f469..992e9c4de 100644
> > --- a/lib/i915/intel_fbc.c
> > +++ b/lib/i915/intel_fbc.c
> > @@ -99,3 +99,39 @@ bool intel_fbc_wait_until_enabled(int device, enum pipe pipe)
> >  
> >         return enabled;
> >  }
> > +
> > +/**
> > + * intel_fbc_plane_size_supported
> > + *
> > + * @fd: fd of the device
> > + * @width: width of the plane
> > + * @height: height of the plane
> > + *
> > + * Checks if the plane size supported for FBC
> > + *
> > + * Returns:
> > + * true if plane size is within the range as per the FBC supported size restrictions per
> > platform
> > + */
> > +bool intel_fbc_plane_size_supported(int fd, uint32_t width, uint32_t height)
> > +{
> > +       const uint32_t dev_id = intel_get_drm_devid(fd);
> > +       const struct intel_device_info *info = intel_get_device_info(dev_id);
> > +       int ver = info->graphics_ver;
> > +       unsigned int max_w, max_h;
> > +
> > +       if (ver >= 10) {
> > +               max_w = 5120;
> > +               max_h = 4096;
> > +       } else if (ver >= 8 || IS_HASWELL(fd)) {
> > +               max_w = 4096;
> > +               max_h = 4096;
> > +       } else if (IS_G4X(fd) || ver >= 5) {
> > +               max_w = 4096;
> > +               max_h = 2048;
> > +       } else {
> > +               max_w = 2048;
> > +               max_h = 1536;
> > +       }
> > +
> > +       return width <= max_w && height <= max_h;
> > +}
> 
> It might also be helpful to have a separate function that returns the max fcb
> plane size supported as a raw value in case any tests wanted to operate on the
> full fcb plane.  Say:
> 
> int intel_fbc_max_plane_size(int device, uint32_t *width, uint32_t *height)
> 
> This could store the width and height generated above and be used as a helper
> function for intel_fbc_plane_size_supported.  I also don't know if fcb could be
> unsupported at some point, but the int return value could be used for any errors.

Thanks for the good suggestion! I have now updated the patch series. There was a build issue as well
related to another patch in the prev series.

I cant think of a error case when the size check is performed! So right now not returning any error
code! Any FBC support related error scenario should be handled outside of this size check I think.

BR
Vinod
> 
> This is just something to consider.  I won't block on it:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> -Jonathan Cavitt
> 
> 
> > diff --git a/lib/i915/intel_fbc.h b/lib/i915/intel_fbc.h
> > index 995dc7f1e..f05b351f6 100644
> > --- a/lib/i915/intel_fbc.h
> > +++ b/lib/i915/intel_fbc.h
> > @@ -14,5 +14,6 @@
> >  bool intel_fbc_supported_on_chipset(int device, enum pipe pipe);
> >  bool intel_fbc_wait_until_enabled(int device, enum pipe pipe);
> >  bool intel_fbc_is_enabled(int device, enum pipe pipe, int log_level);
> > +bool intel_fbc_plane_size_supported(int device, uint32_t width, uint32_t height);
> >  
> >  #endif
> > -- 
> > 2.34.1
> > 
> >