Message-ID: <44ff7acd-b3f9-4d49-91e5-d139a2340375@kernel.org>
Date: Tue, 30 Sep 2025 12:07:30 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Jai Luthra <jai.luthra@...asonboard.com>,
 Hans Verkuil <hverkuil@...nel.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
 linux-media@...r.kernel.org, Ricardo Ribalda <ribalda@...omium.org>,
 Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
 Ma Ke <make24@...as.ac.cn>,
 Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/10] media: v4l2-dev: Add support for try state

On 30/09/2025 11:35, Jacopo Mondi wrote:
> Hi Hans
> 
> On Tue, Sep 30, 2025 at 09:30:55AM +0200, Hans Verkuil wrote:
>> On 29/09/2025 18:15, Jai Luthra wrote:
>>> Hi Hans,
>>>
>>> Quoting Hans Verkuil (2025-09-22 13:28:26)
>>>> On 22/09/2025 09:52, Hans Verkuil wrote:
>>>>> On 19/09/2025 11:55, Jai Luthra wrote:
>>>>>> Format negotiation performed via the TRY_FMT ioctl should only affect
>>>>>> the temporary context of a specific filehandle, not the active state
>>>>>> stored in the video device structure. To support this distinction, we
>>>>>> need separate storage for try and active states.
>>>>>>
>>>>>> Introduce an enum to distinguish between these two state types and store
>>>>>> the try state in struct v4l2_fh instead of the video device structure.
>>>>>> The try state is allocated during file handle initialization in
>>>>>> v4l2_fh_init() and released in v4l2_fh_exit().
>>>>>
>>>>> There is a major difference between TRY in video devices and TRY in subdev
>>>>> devices: TRY for video devices is not persistent, i.e. it doesn't need to
>>>>> be stored anywhere since nobody will be using that information.
>>>>>
>>>
>>> Yes, the v4l2 format sent with VIDIOC_TRY_FMT is currently not stored by
>>> the drivers, but simply modified and returned back to userspace. From the
>>> userspace point of view, that should still work the same way with this
>>> series.
>>>
>>> The drivers will get access to the correct state (active) for internal use
>>> through framework helpers (that will be introduced in next revision), so
>>> the try state doesn't have any real "use" today.
>>>
>>>>> If the plan is to change that in later patch series, then that should be
>>>>> very clearly stated. And I think I would need to know the details of those
>>>>> future plans before I can OK this.
>>>>>
>>>>> So how is this try state intended to be used in the future?
>>>>
>>>> Follow-up: if there are no plans to change/enhance the VIDIOC_TRY_FMT semantics,
>>>> then I don't really see the point of this since there is no need to store this
>>>> information.
>>>
>>> There are no plans to change the userspace side of this. The main
>>> motivation to allocate and keep a try state in the framework is to simplify
>>> the driver implementation. A single function can serve as both s_fmt and
>>> try_fmt ioctl handler, and operate on the passed state argument without
>>> caring if it will be applied on the device or just for negotiation.
>>>
>>> In future, drivers may subclass the state with their device specific data,
>>> so that nothing tied to the hardware state is stored in the driver
>>> structures directly, but I don't see why drivers will need to inspect TRY
>>> formats.
>>
>> I think having an unused try state is a bad idea and really confusing.
>>
>> Especially because for subdevices the try state is actually used, so
> 
> I might have missed where. TRY formats on subdev sink pads influence
> TRY formats on the source pads, are there other usages I might be
> overlooking ?

That's the main one. But also that the TRY format for a subdev filehandle
is persistent, i.e. you can query it later.

For video devices this is not stored, i.e. there is no G_TRY_FMT equivalent.
TRY_FMT takes the format, it validates it and returns it, but it is not
stored anywhere.

>> for normal devices you would automatically expect the same thing when
>> you see a try state.
>>
>> You should reconsider this approach.
> 
> The 'try' state will be stored in the v4l2_fh and won't be accessible
> to userspace.
> 
> Drivers instead, might accumulate the result of multiple TRY_FMT calls
> into the state stored in the v4l2_fh incrementally. Is this a concern
> for you ?

Basically the try state would be a scratch state, it's not used for anything
else. I think it is very confusing keeping it.

> 
> I still think having a single implementation for s_fmt and try_fmt is
> a win for drivers, even if the try state are now effectively stored
> somewhere.

In a properly written driver s_fmt will call try_fmt first to validate the given
format, then use that format to actually program the hardware.

Unfortunately, a lot of drivers have duplicate format validation code for
both try_fmt and s_fmt (hopefully, but not always, doing the same validation).

I think this is partially historic. When the vidioc_g/s/try_vid_cap etc. ops
were introduced, drivers had to be converted as well. And if memory serves often
drivers handled TRY and S_FMT in a single function, and that now had to be split
up, so I suspect that in quite a few cases the code was simply duplicated.

It's a long time ago though, so I may be wrong about that.

Also, older drivers do not always support TRY_FMT. Although I'm not sure how
many of such drivers remain.

Regards,

	Hans

> 
> Thanks
>   j
> 
>>
>> Regards,
>>
>> 	Hans

Powered by blists - more mailing lists