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

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

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?

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