Re: [PATCHv4,04/36] drm/gem-fb-helper: Add special version of drm_gem_fb_size_check

From: "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>
To: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: nd@arm.com, kernel@collabora.com,
	Mihail Atanassov <mihail.atanassov@arm.com>,
	David Airlie <airlied@linux.ie>,
	Liviu Dudau <liviu.dudau@arm.com>,
	Sandy Huang <hjc@rock-chips.com>,
	dri-devel@lists.freedesktop.org,
	Ayan Halder <Ayan.Halder@arm.com>, Sean Paul <sean@poorly.run>
Subject: Re: [PATCHv4,04/36] drm/gem-fb-helper: Add special version of drm_gem_fb_size_check
Date: Mon, 17 Feb 2020 19:34:32 +0800	[thread overview]
Message-ID: <20200217113432.GA6208@jamwan02-TSP300> (raw)
In-Reply-To: <e5b798c2-baf0-deae-b4ae-ac0e19abd710@collabora.com>

On Mon, Feb 17, 2020 at 11:55:50AM +0100, Andrzej Pietrasiewicz wrote:
> Hi James,
> 
> Thank you for the review.
> 
> Did you intentionally review patches from the v4 series or you simply
> didn't notice the v5? There are some differences, the most notable one
> is using proper way of subclassing a drm_framebuffer.
> The v5 series was sent on 17th December 2019.

Hi Andrzej:
Sorry, I did the review based pathwork:
https://patchwork.freedesktop.org/series/70889/#rev35
seems it has problem which only have V4.

I will re-do the review for V5 ASAP.

Sorry again.

James.

> Andrzej
> 
> W dniu 17.02.2020 oï¿½09:16, james qian wang (Arm Technology China) pisze:
> > Hi Andrzej:
> > 
> > Really a good idea for introducing this custom size check, it's very
> > useful for some Komeda/malidp format, espcially pitch_multiplier, maybe
> > in future we can add it into into the drm_format_info.
> > 
> > On Fri, Dec 13, 2019 at 04:58:35PM +0100, Andrzej Pietrasiewicz wrote:
> > > The new version accepts a struct describing deviations from standard way of
> > > doing the size checks. The caller must provide the respective values.
> > > 
> > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > > ---
> > >   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 47 ++++++++++++++++----
> > >   include/drm/drm_gem_framebuffer_helper.h     | 16 +++++++
> > >   2 files changed, 55 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > index 787edb9a916b..4201dc1f32a5 100644
> > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > > @@ -201,8 +201,9 @@ int drm_gem_fb_lookup(struct drm_device *dev,
> > >   EXPORT_SYMBOL_GPL(drm_gem_fb_lookup);
> > >   /**
> > > - * drm_gem_fb_size_check() - Helper function for use in
> > > - *			     &drm_mode_config_funcs.fb_create implementations
> > > + * drm_gem_fb_size_check_special() - Helper function for use in
> > > + *				     &drm_mode_config_funcs.fb_create
> > > + *				     implementations
> > >    * @dev: DRM device
> > >    * @mode_cmd: Metadata from the userspace framebuffer creation request
> > >    *
> > > @@ -212,9 +213,10 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_lookup);
> > >    * Returns:
> > >    * Zero on success or a negative error code on failure.
> > >    */
> > > -int drm_gem_fb_size_check(struct drm_device *dev,
> > > -			  const struct drm_mode_fb_cmd2 *mode_cmd,
> > > -			  struct drm_gem_object **objs)
> > > +int drm_gem_fb_size_check_special(struct drm_device *dev,
> > 
> > How about name it to drm_gem_fb_custom_size_check()
> > 
> > > +				  const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +				  const struct drm_size_check *check,
> > > +				  struct drm_gem_object **objs)
> > >   {
> > >   	const struct drm_format_info *info;
> > >   	int i;
> > > @@ -227,10 +229,19 @@ int drm_gem_fb_size_check(struct drm_device *dev,
> > >   		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > >   		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > >   		unsigned int min_size;
> > > +		u32 pitch = mode_cmd->pitches[i];
> > > +
> > > +		if (check && check->use_pitch_multiplier)
> > > +			if ((pitch * check->pitch_multiplier[i]) %
> > > +			    check->pitch_modulo)
> > > +				return -EINVAL;
> > > -		min_size = (height - 1) * mode_cmd->pitches[i]
> > > -			 + drm_format_info_min_pitch(info, i, width)
> > > -			 + mode_cmd->offsets[i];
> > > +		if (check && check->use_min_size)
> > > +			min_size = check->min_size[i];
> > > +		else
> > > +			min_size = (height - 1) * pitch
> > > +				 + drm_format_info_min_pitch(info, i, width)
> > > +				 + mode_cmd->offsets[i];
> > >   		if (objs[i]->size < min_size)
> > >   			return -EINVAL;
> > > @@ -239,6 +250,26 @@ int drm_gem_fb_size_check(struct drm_device *dev,
> > >   	return 0;
> > >   }
> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_size_check_special);
> > > +
> > > +/**
> > > + * drm_gem_fb_size_check() - Helper function for use in
> > > + *			     &drm_mode_config_funcs.fb_create implementations
> > > + * @dev: DRM device
> > > + * @mode_cmd: Metadata from the userspace framebuffer creation request
> > > + *
> > > + * This function can be used to verify buffer sizes for all planes.
> > > + * It is caller's responsibility to put the objects on failure.
> > > + *
> > > + * Returns:
> > > + * Zero on success or a negative error code on failure.
> > > + */
> > > +int drm_gem_fb_size_check(struct drm_device *dev,
> > > +			  const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +			  struct drm_gem_object **objs)
> > > +{
> > > +	return drm_gem_fb_size_check_special(dev, mode_cmd, NULL, objs);
> > > +}
> > >   EXPORT_SYMBOL_GPL(drm_gem_fb_size_check);
> > >   /**
> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> > > index c85d4b152e91..74304a268694 100644
> > > --- a/include/drm/drm_gem_framebuffer_helper.h
> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
> > > @@ -11,6 +11,18 @@ struct drm_mode_fb_cmd2;
> > >   struct drm_plane;
> > >   struct drm_plane_state;
> > >   struct drm_simple_display_pipe;
> > > +struct drm_size_check;
> > > +
> > > +/**
> > > + * struct drm_size_check - Description of special requirements for size checks.
> > > + */
> > > +struct drm_size_check {
> > > +	unsigned int min_size[4];
> > > +	bool use_min_size;
> > > +	u32 pitch_multiplier[4];
> > > +	u32 pitch_modulo;
> > > +	bool use_pitch_multiplier;
> > > +};
> > >   struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> > >   					  unsigned int plane);
> > > @@ -32,6 +44,10 @@ int drm_gem_fb_lookup(struct drm_device *dev,
> > >   		      struct drm_file *file,
> > >   		      const struct drm_mode_fb_cmd2 *mode_cmd,
> > >   		      struct drm_gem_object **objs);
> > > +int drm_gem_fb_size_check_special(struct drm_device *dev,
> > > +				  const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +				  const struct drm_size_check *check,
> > > +				  struct drm_gem_object **objs);
> > >   int drm_gem_fb_size_check(struct drm_device *dev,
> > >   			  const struct drm_mode_fb_cmd2 *mode_cmd,
> > >   			  struct drm_gem_object **objs);
> > 
> > For this common case can we just define it as a MACRO, or a inline
> > func here in this header.
> > 
> > Thanks
> > James
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel