Hi,
On 14/12/2021 09:13, Jacopo Mondi wrote:
Hi Tomi
On Tue, Nov 30, 2021 at 04:15:01PM +0200, Tomi Valkeinen wrote:
At the moment when a subdev op is called, the TRY subdev state
(subdev_fh->state) is passed as a parameter even for the ACTIVE case, or
alternatively a NULL can be passed for ACTIVE case. This used to make
sense, as the ACTIVE state was handled internally by the subdev drivers.
We now have a state for the ACTIVE case in a standard place, and can
pass that also to the drivers. This patch changes the subdev ioctls to
either pass the TRY or ACTIVE state to the subdev.
Unfortunately many drivers call ops from other subdevs, and implicitly
pass NULL as the state, so this is just a partial solution. A coccinelle
spatch could perhaps be created which fixes the drivers' subdev calls.
For all current upstream drivers this doesn't matter, as they do not
expect to get a valid state for ACTIVE case. But future drivers which
support multiplexed streaming and routing will depend on getting a state
for both active and try cases.
For new drivers we can mandate that the pipelines where the drivers are
used need to pass the state properly, or preferably, not call such
subdev ops at all.
However, if an existing subdev driver is changed to support multiplexed
streams, the driver has to consider cases where its ops will be called
with NULL state. The problem can easily be solved by using the
v4l2_subdev_validate_and_lock_state() helper, introduced in a follow up
Now called v4l2_subdev_lock_and_return_state() and introduced in a
previous patch.
Ah, right.
I would still push for state-aware subdev drivers to BUG() on !state
and tell them to fix the caller. Is this too harsh ?
I think so. We can't break currently working drivers.
patch.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
---
drivers/media/v4l2-core/v4l2-subdev.c | 73 +++++++++++++++++++++++----
1 file changed, 63 insertions(+), 10 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index de160140d63b..3289875d9ec1 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -353,6 +353,53 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+
+static struct v4l2_subdev_state *
+subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
+ unsigned int cmd, void *arg)
+{
+ u32 which;
+
+ switch (cmd) {
+ default:
+ return NULL;
+
+ case VIDIOC_SUBDEV_G_FMT:
+ case VIDIOC_SUBDEV_S_FMT: {
+ which = ((struct v4l2_subdev_format *)arg)->which;
+ break;
+ }
+ case VIDIOC_SUBDEV_G_CROP:
+ case VIDIOC_SUBDEV_S_CROP: {
+ which = ((struct v4l2_subdev_crop *)arg)->which;
+ break;
+ }
+ case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {
+ which = ((struct v4l2_subdev_mbus_code_enum *)arg)->which;
+ break;
+ }
+ case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: {
+ which = ((struct v4l2_subdev_frame_size_enum *)arg)->which;
+ break;
+ }
No need for braces if I'm not mistaken, and also some blocks are
followed by an empty line some or not.
Ok.
+
+ case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: {
+ which = ((struct v4l2_subdev_frame_interval_enum *)arg)->which;
+ break;
+ }
+
+ case VIDIOC_SUBDEV_G_SELECTION:
+ case VIDIOC_SUBDEV_S_SELECTION: {
+ which = ((struct v4l2_subdev_selection *)arg)->which;
+ break;
+ }
+ }
+
+ return which == V4L2_SUBDEV_FORMAT_TRY ?
+ subdev_fh->state :
+ v4l2_subdev_get_active_state(sd);
+}
+
static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
{
struct video_device *vdev = video_devdata(file);
@@ -360,8 +407,11 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
struct v4l2_fh *vfh = file->private_data;
struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
+ struct v4l2_subdev_state *state;
int rval;
+ state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
+
There's a possibility NULL is returned if a new subdev_fh is added and
the above not updated. Should we BUG_ON() ?
NULL is fine, as not all ioctls need a state.
Tomi
