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

[Janusz Krzysztofik <jmkrzyszt@gmail.com>]

Hi Sakari,

On Monday, June 10, 2019 10:54:44 AM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Thu, Jun 06, 2019 at 08:13:36PM +0200, Janusz Krzysztofik wrote:
> > 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.
> 
> The primary users of these functions are drivers that do support subdev
> uAPI. Some drivers can function that disabled, so making these functions
> usable to those drivers in all cases simplifies those drivers.

Indeed.  I didn't take that group of drivers into account.

> > > > ...
> > > > 
> > > > > 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.
> 
> You can't rely on checks done by the v4l2_subdev_call macro as these
> parameters do not come from the caller; they are set by the driver itself.

If that' the case, you are right.

It looks like I'm missing some knowledge on V4L2 framework needed to provide a 
valuable review of your change, please ignore my comments.

Thanks,
Janusz