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().
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;
--
2.51.0
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; >
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; >> > >
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. > > Regards, > > Hans > > > > > Regards, > > > > Hans > > Thanks, Jai [snip]
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 for normal devices you would automatically expect the same thing when you see a try state. You should reconsider this approach. Regards, Hans
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 ? > 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 ? 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. Thanks j > > Regards, > > Hans
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
© 2016 - 2025 Red Hat, Inc.