Re: [PATCH v2 02/10] media: v4l2-dev: Add support for try state

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
>> --
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: Hans Verkuil <hverkuil@kernel.org>
>> Cc: Ricardo Ribalda <ribalda@chromium.org>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Cc: Jai Luthra <jai.luthra@ideasonboard.com>
>> Cc: Ma Ke <make24@iscas.ac.cn>
>> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> Cc: linux-media@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c |  7 +++++--
>>  drivers/media/v4l2-core/v4l2-fh.c  |  6 ++++++
>>  include/media/v4l2-dev.h           | 17 ++++++++++++++++-
>>  include/media/v4l2-fh.h            |  2 ++
>>  4 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 997255709448510fcd17b6de798a3df99cd7ea09..26b6b2f37ca55ce981aa17a28a875dc3cf253d9b 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -164,7 +164,8 @@ void video_device_release_empty(struct video_device *vdev)
>>  EXPORT_SYMBOL(video_device_release_empty);
>>  
>>  struct video_device_state *
>> -__video_device_state_alloc(struct video_device *vdev)
>> +__video_device_state_alloc(struct video_device *vdev,
>> +			   enum video_device_state_whence which)
>>  {
>>  	struct video_device_state *state =
>>  		kzalloc(sizeof(struct video_device_state), GFP_KERNEL);
>> @@ -172,6 +173,7 @@ __video_device_state_alloc(struct video_device *vdev)
>>  	if (!state)
>>  		return ERR_PTR(-ENOMEM);
>>  
>> +	state->which = which;
>>  	state->vdev = vdev;
>>  
>>  	return state;
>> @@ -962,7 +964,8 @@ int __video_register_device(struct video_device *vdev,
>>  
>>  	/* state support */
>>  	if (test_bit(V4L2_FL_USES_STATE, &vdev->flags))
>> -		vdev->state = __video_device_state_alloc(vdev);
>> +		vdev->state = __video_device_state_alloc(vdev,
>> +							 VIDEO_DEVICE_STATE_ACTIVE);
>>  
>>  	/* Part 1: check device type */
>>  	switch (type) {
>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
>> index df3ba9d4674bd25626cfcddc2d0cb28c233e3cc3..522acc0eb8401305c6893232d96d826669ab90d5 100644
>> --- a/drivers/media/v4l2-core/v4l2-fh.c
>> +++ b/drivers/media/v4l2-core/v4l2-fh.c
>> @@ -38,6 +38,10 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>>  	INIT_LIST_HEAD(&fh->subscribed);
>>  	fh->sequence = -1;
>>  	mutex_init(&fh->subscribe_lock);
>> +	/* state support */
>> +	if (test_bit(V4L2_FL_USES_STATE, &fh->vdev->flags))
>> +		fh->state = __video_device_state_alloc(vdev,
>> +						       VIDEO_DEVICE_STATE_TRY);
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
>>  
>> @@ -84,6 +88,8 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>>  {
>>  	if (fh->vdev == NULL)
>>  		return;
>> +	if (test_bit(V4L2_FL_USES_STATE, &fh->vdev->flags))
>> +		kfree(fh->state);
>>  	v4l_disable_media_source(fh->vdev);
>>  	v4l2_event_unsubscribe_all(fh);
>>  	mutex_destroy(&fh->subscribe_lock);
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index 57e4691ef467aa2b0782dd4b8357bd0670643293..5ca04a1674e0bf7016537e6fb461d790fc0a958f 100644
>> --- a/include/media/v4l2-dev.h
>> +++ b/include/media/v4l2-dev.h
>> @@ -220,15 +220,28 @@ struct v4l2_file_operations {
>>  	int (*release) (struct file *);
>>  };
>>  
>> +/**
>> + * enum video_device_state_whence - Video device state type
>> + *
>> + * @VIDEO_DEVICE_STATE_TRY: from VIDIOC_TRY_xxx, for negotiation only
>> + * @VIDEO_DEVICE_STATE_ACTIVE: from VIDIOC_S_xxx, applied to the device
>> + */
>> +enum video_device_state_whence {
>> +	VIDEO_DEVICE_STATE_TRY = 0,
>> +	VIDEO_DEVICE_STATE_ACTIVE = 1,
>> +};
>> +
>>  /**
>>   * struct video_device_state - Used for storing video device state information.
>>   *
>>   * @fmt: Format of the capture stream
>>   * @vdev: Pointer to video device
>> + * @which: State type (from enum video_device_state_whence)
>>   */
>>  struct video_device_state {
>>  	struct v4l2_format fmt;
>>  	struct video_device *vdev;
>> +	enum video_device_state_whence which;
>>  };
>>  
>>  /*
>> @@ -568,13 +581,15 @@ static inline int video_is_registered(struct video_device *vdev)
>>  /** __video_device_state_alloc - allocate video device state structure
>>   *
>>   * @vdev: pointer to struct video_device
>> + * @which: type of video device state (from enum video_device_state_whence)
>>   *
>>   * .. note::
>>   *
>>   *	This function is meant to be used only inside the V4L2 core.
>>   */
>>  struct video_device_state *
>> -__video_device_state_alloc(struct video_device *vdev);
>> +__video_device_state_alloc(struct video_device *vdev,
>> +			   enum video_device_state_whence which);
>>  
>>  /** __video_device_state_free - free video device state structure
>>   *
>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
>> index aad4b3689d7ea191978f24ce24d24cd2e73636b6..55455704a98d0785d0a3418b8a43d7363b7c8aa2 100644
>> --- a/include/media/v4l2-fh.h
>> +++ b/include/media/v4l2-fh.h
>> @@ -28,6 +28,7 @@ struct v4l2_ctrl_handler;
>>   * @vdev: pointer to &struct video_device
>>   * @ctrl_handler: pointer to &struct v4l2_ctrl_handler
>>   * @prio: priority of the file handler, as defined by &enum v4l2_priority
>> + * @state: try state used for format negotiation on the video device
>>   *
>>   * @wait: event' s wait queue
>>   * @subscribe_lock: serialise changes to the subscribed list; guarantee that
>> @@ -44,6 +45,7 @@ struct v4l2_fh {
>>  	struct video_device	*vdev;
>>  	struct v4l2_ctrl_handler *ctrl_handler;
>>  	enum v4l2_priority	prio;
>> +	struct video_device_state *state;
>>  
>>  	/* Events */
>>  	wait_queue_head_t	wait;
>>
> 
>