Hi Sakari,
On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
*crop)
> > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > {
> > - if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > - crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + if (sd->entity.num_pads && pad >= sd->entity.num_pads)
>
> One more comment.
>
> The num_pads doesn't really tell whether a given op is valid for a device.
> Well, in this case it would have to be a bug in the driver, but those do
> happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> non-NULL if the entity is registered with a media device, i.e. when these
> callback functions are supposed to be called.
Before I do that, let me undestand your point better.
My intentions were:
1) to provide a check for validity of a pad ID passed to an operation, not ann
eligibility of a driver to support the operation,
2) to not break drivers which don't set pad_num, especially when building them
with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.
Since pad IDs are verified against pad_num which may be not set, we should
obviously check validity of pad_num before comparing against it. Since media
controller compatible subdevices need at least one pad, I think the check for
non-zero pad_num is quite reasonable.
Moreover, old drivers are actually using those pad operations you describe as
not supposed to be called. They are using them because they were converted to
use them in place of former video ops. Already dealing with pad IDs, they may
decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for
example register pads, without implementing fulll media controller support.
Why should we refuse to perform pad ID verification for them?
Thanks,
Janusz
