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@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.
> > >
> > > /**
> > > * 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;
> > >
> > >
> >
> >
> >
> >
>
>
