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@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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
