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

Hans Verkuil
2023-12-11T12:25:31Z

On 11/12/2023 12:53, Mauro Carvalho Chehab wrote:
> 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.

I think there are two types of cases here: the first case is when the subdev
does not support TRY at all for any of the pad ops, i.e. it can't be used by
MC-centric drivers.

In that case a top-level comment to that effect might be useful (that can be
done in a separate patch).

The second case is where it does support TRY for the other pad ops, just not
for g/s_frame_intervals. I checked the updated documentation in patch 3/8, but
it is not clear whether that would be considered a bug or not. That is something
that should be clarified.

Frankly, it is not clear to me if, for an MC-centric capable subdev, support
for FORMAT_TRY is required to be present for all pad ops, or not. And if not,
for which pad ops is it optional and what does that mean for applications.

>
> 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.

Yeah, I think I should add this. If a /dev/v4l-subdevX exists, then that means
it is very likely created through an MC-centric driver and TRY support should
be there.

Regards,

Hans