Re: [RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Janusz Krzysztofik <jmkrzyszt@gmail.com>]

Hi Sakari,

On Thursday, June 6, 2019 3:56:42 PM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Wed, Jun 05, 2019 at 09:33:41PM +0200, Janusz Krzysztofik wrote:
> > Hi Sakari,
> > 
> > On Tuesday, June 4, 2019 7:57:31 PM CEST Sakari Ailus wrote:
> > > Rework the macros for accessing subdev try formats to work meaningfully
> > > and relatively safely without V4L2 sub-device uAPI (and without MC). This
> > > is done by simply reverting to accessing the pad number zero if
> > > CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel warning
> > > if the pad is non-zero in that case.
> > > 
> > > The functions are seen useful if subdev uAPI is disabled for drivers that
> > > can work without the Kconfig option but benefit from the option if it's
> > > enabled.
> > 
> > I'm not sure which drivers you (we) consider valid users of those functions.  
> > Subdevice drivers only? Or other drivers which interact with subdevices as 
> > well?  An answer to that question determines my possible other comments.
> 
> These functions are only by drivers for the devices they control directly
> only.

That's what I expected.

Exposing those functions to drivers not supporting V4L2 subdev uAPI is 
not a bad idea but it would make more sense to me if we also updated potential 
users.  Otherwise I just don't believe anyone will care for as long as a 
driver is not refreshed to support MC and V4L2 subdev uAPI.

> > ...
> > > As a by-product, the patch simplifies individual inline functions by
> > > removing two lines of code from each of them and moving the functionality
> > > to a common macro.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > Hi guys,
> > > 
> > > This might not be pretty but should provide some comfort for drivers
> > > working with different Kconfig options. Comments are welcome...
> > > 
> > >  include/media/v4l2-subdev.h | 24 ++++++++++++++----------
> > >  1 file changed, 14 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index e1e3c18c3fd6..3328f302326b 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
> > >  	container_of(fh, struct v4l2_subdev_fh, vfh)
> > >  
> > >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > > +	(WARN_ON(!(__cfg)) ? NULL :		\
> > > +	 ((__sd)->entity.graph_obj.mdev ?				
> > \strange
> > > +	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
> > > +	   NULL : &(__cfg)[__pad].__field) :				
> > \
> > > +	  &(__cfg)[WARN_ON(__pad) && 0].__field))
> > > +#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > > +	(WARN_ON(!(__cfg)) ? NULL :				
> > 	\
> > > +	 &(__cfg)[WARN_ON(__pad) && 0].__field)
> > > +#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */

I think that as long as we already have in place the same checks performed by 
v4l2_subdev_call() which seems the only user entry point to a subdevice 
driver, invalid cfg or pad values you are trying to catch here will never be 
passed unless the driver performs unusual operations and, moreover, is 
internally broken.

> > >  
> > >  /**
> > >   * v4l2_subdev_get_try_format - ancillary routine to call
> > > @@ -944,9 +955,7 @@ static inline struct v4l2_mbus_framefmt
> > >  			    struct v4l2_subdev_pad_config *cfg,
> > >  			    unsigned int pad)
> > >  {
> > > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > > -		pad = 0;

Dropping this check from here and below makes sense to me anyway, for the same 
reason as explained above.

> > > -	return &cfg[pad].try_fmt;
> > > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_fmt);

If we agree that correctness of cfg and pad has already been verified, this 
change and similar below, perfectly correct otherwise, seem of limited value 
to me.

Thanks,
Janusz

> > >  }
> > >  
> > >  /**
> > > @@ -962,9 +971,7 @@ static inline struct v4l2_rect
> > >  			  struct v4l2_subdev_pad_config *cfg,
> > >  			  unsigned int pad)
> > >  {
> > > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > > -		pad = 0;
> > > -	return &cfg[pad].try_crop;
> > > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_crop);
> > >  }
> > >  
> > >  /**
> > > @@ -980,11 +987,8 @@ static inline struct v4l2_rect
> > >  			     struct v4l2_subdev_pad_config *cfg,
> > >  			     unsigned int pad)
> > >  {
> > > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > > -		pad = 0;
> > > -	return &cfg[pad].try_compose;
> > > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_compose);
> > >  }
> > > -#endif
> > >  
> > >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > >  
> > > 
> > 
> > 
> > 
> > 
> 
>