On Friday, May 17, 2019 5:58:40 PM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> > 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.
>
> Indeed.
>
> But these checks still allow calling the pad operations on sub-devices that
> have no pads. That should not be allowed. Pads are a Media controller
> concept, they do not exist outside it; therefore checking for pads only if
> the subdev is a part of the media device would be entirely correct.
OK, now I see your point. You don't want the check to succeed if a media
entity has num_pads == 0.
> It should probably accompany a check that requires the pad number is zero
> if the subdev doesn't have a graph object, even if the pad field isn't
> supposedly used for any purpose. Would that address your concern?
Yes, that's acceptable. Let's require subdevice drivers to register as media
entities if they want to use pads > 0.
I'll update the patches and submit as v7 soon.
Thanks,
Janusz
> > 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?
>
>
