Re: [PATCH v4 3/8] media: v4l2-subdev: Add which field to struct v4l2_subdev_frame_interval

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Mon, 11 Dec 2023 09:59:25 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 09/12/2023 12:11, Laurent Pinchart wrote:
> > On Sat, Dec 09, 2023 at 06:55:01AM +0100, Mauro Carvalho Chehab wrote:  
> >> Em Fri,  8 Dec 2023 20:16:43 +0200
> >> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> >>  
> >>> Due to a historical mishap, the v4l2_subdev_frame_interval structure
> >>> is the only part of the V4L2 subdev userspace API that doesn't contain a
> >>> 'which' field. This prevents trying frame intervals using the subdev
> >>> 'TRY' state mechanism.
> >>>
> >>> Adding a 'which' field is simple as the structure has 8 reserved fields.
> >>> This would however break userspace as the field is currently set to 0,
> >>> corresponding to V4L2_SUBDEV_FORMAT_TRY, while the corresponding ioctls
> >>> currently operate on the 'ACTIVE' state. We thus need to add a new
> >>> subdev client cap, V4L2_SUBDEV_CLIENT_CAP_WHICH_INTERVAL, to indicate
> >>> that userspace is aware of this new field.
> >>>
> >>> All drivers that implement the subdev .get_frame_interval() and
> >>> .set_frame_interval() operations are updated to return -EINVAL when
> >>> operating on the TRY state, preserving the current behaviour.
> >>>
> >>> While at it, fix a bad copy&paste in the documentation of the struct
> >>> v4l2_subdev_frame_interval_enum 'which' field.  
> >>
> >> The uAPI change looks ok to me. However, having to add an special
> >> logic on every client using (get/set)_time_interval seems risky,
> >> as it will require an extra check before accepting new subdev
> >> drivers.
> >>
> >> Please move such check to drivers/media/v4l2-core/v4l2-subdev.c:
> >>
> >> 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> >> 		return -EINVAL;  
> > 
> > But then no new driver will be able to use this API. Look at patch 8/8
> > in this series, the whole point of this exercise is to support
> > V4L2_SUBDEV_FORMAT_TRY in drivers. The added checks in existing drivers
> > is because they don't support V4L2_SUBDEV_FORMAT_TRY. Moving forward,
> > the drivers that are still maintained should be converted eventually.

Then please add a FIXME note there warning that the logic there
exists just because the driver doesn't yet support V4L2_SUBDEV_FORMAT_TRY.

With that, feel free to add: 

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> > All new drivers should use the V4L2 subdev active state API and support
> > V4L2_SUBDEV_FORMAT_TRY right away.  

OK.

> I agree with Laurent here. Note that the logic isn't 'special', it is standard
> handling of the 'which' field. It was never there for g/s_frame_interval
> because that was the only one that didn't have a 'which' field, but now that
> it does, the drivers need to be adapted to handle this new field.
> 
> It shouldn't be hidden in some core code, that would only confuse matters.

If the idea is to move forward without implementing support for such
features at the existing drivers, we should at least have something
annotated at the drivers that this is something that will require
further changes in the future to support the current behavior.

I also expect that the API compliance tool to produce warnings
on drivers using the old behavior.

Thanks,
Mauro