Re: [PATCH 03/19] drm: Look up the format info earlier

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

On Thu, Apr 10, 2025 at 10:33:02PM +0300, Laurent Pinchart wrote:
> Hi Ville,
> 
> Thank you for the patch.
> 
> On Thu, Apr 10, 2025 at 07:32:02PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Looks up the format info in already drm_internal_framebuffer_create()
> > so that we can later pass it along to .fb_create(). Currently various
> > drivers are doing additional lookups in their .fb_create()
> > implementations, and these lookups are rather expensive now (given
> > how many different pixel formats we have).
> 
> That's a separate issue, but would it be worth using a data structure
> that supports more efficient lookup ?

I think the obvious solution would be to to just sort the array
and use a binary search. Ideally we'd get the compiler to do that
for us at build time and then get rid of the unsorted array entirely,
but sadly we can't do that in C. The alternative of keeping the array
sorted by hand sounds very annoying (at least without having a way
to validate that it is correctly sorted at build time).

> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 18a0267e374e..ae09ef6977b2 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -153,18 +153,11 @@ int drm_mode_addfb_ioctl(struct drm_device *dev,
> >  }
> >  
> >  static int framebuffer_check(struct drm_device *dev,
> > +			     const struct drm_format_info *info,
> >  			     const struct drm_mode_fb_cmd2 *r)
> >  {
> > -	const struct drm_format_info *info;
> >  	int i;
> >  
> > -	/* check if the format is supported at all */
> > -	if (!__drm_format_info(r->pixel_format)) {
> > -		drm_dbg_kms(dev, "bad framebuffer format %p4cc\n",
> > -			    &r->pixel_format);
> > -		return -EINVAL;
> > -	}
> > -
> >  	if (r->width == 0) {
> >  		drm_dbg_kms(dev, "bad framebuffer width %u\n", r->width);
> >  		return -EINVAL;
> > @@ -175,9 +168,6 @@ static int framebuffer_check(struct drm_device *dev,
> >  		return -EINVAL;
> >  	}
> >  
> > -	/* now let the driver pick its own format info */
> > -	info = drm_get_format_info(dev, r->pixel_format, r->modifier[0]);
> > -
> >  	for (i = 0; i < info->num_planes; i++) {
> >  		unsigned int width = drm_format_info_plane_width(info, r->width, i);
> >  		unsigned int height = drm_format_info_plane_height(info, r->height, i);
> > @@ -272,6 +262,7 @@ drm_internal_framebuffer_create(struct drm_device *dev,
> >  				struct drm_file *file_priv)
> >  {
> >  	struct drm_mode_config *config = &dev->mode_config;
> > +	const struct drm_format_info *info;
> >  	struct drm_framebuffer *fb;
> >  	int ret;
> >  
> > @@ -297,7 +288,17 @@ drm_internal_framebuffer_create(struct drm_device *dev,
> >  		return ERR_PTR(-EINVAL);
> >  	}
> >  
> > -	ret = framebuffer_check(dev, r);
> > +	/* check if the format is supported at all */
> > +	if (!__drm_format_info(r->pixel_format)) {
> > +		drm_dbg_kms(dev, "bad framebuffer format %p4cc\n",
> > +			    &r->pixel_format);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	/* now let the driver pick its own format info */
> > +	info = drm_get_format_info(dev, r->pixel_format, r->modifier[0]);
> > +
> > +	ret = framebuffer_check(dev, info, r);
> >  	if (ret)
> >  		return ERR_PTR(ret);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Ville Syrjälä
Intel