| Message ID | 20211130141536.891878-4-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | v4l: subdev internal routing and streams | expand |
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. I would still push for state-aware subdev drivers to BUG() on !state and tell them to fix the caller. Is this too harsh ? > patch. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > 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. > + > + 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() ? > switch (cmd) { > case VIDIOC_SUBDEV_QUERYCAP: { > struct v4l2_subdev_capability *cap = arg; > @@ -484,7 +534,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(format->reserved, 0, sizeof(format->reserved)); > memset(format->format.reserved, 0, sizeof(format->format.reserved)); > - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format); > + return v4l2_subdev_call(sd, pad, get_fmt, state, format); > } > > case VIDIOC_SUBDEV_S_FMT: { > @@ -495,7 +545,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(format->reserved, 0, sizeof(format->reserved)); > memset(format->format.reserved, 0, sizeof(format->format.reserved)); > - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format); > + return v4l2_subdev_call(sd, pad, set_fmt, state, format); > } > > case VIDIOC_SUBDEV_G_CROP: { > @@ -509,7 +559,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > sel.target = V4L2_SEL_TGT_CROP; > > rval = v4l2_subdev_call( > - sd, pad, get_selection, subdev_fh->state, &sel); > + sd, pad, get_selection, state, &sel); > > crop->rect = sel.r; > > @@ -531,7 +581,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > sel.r = crop->rect; > > rval = v4l2_subdev_call( > - sd, pad, set_selection, subdev_fh->state, &sel); > + sd, pad, set_selection, state, &sel); > > crop->rect = sel.r; > > @@ -542,7 +592,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > struct v4l2_subdev_mbus_code_enum *code = arg; > > memset(code->reserved, 0, sizeof(code->reserved)); > - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state, > + return v4l2_subdev_call(sd, pad, enum_mbus_code, state, > code); > } > > @@ -550,7 +600,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > struct v4l2_subdev_frame_size_enum *fse = arg; > > memset(fse->reserved, 0, sizeof(fse->reserved)); > - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state, > + return v4l2_subdev_call(sd, pad, enum_frame_size, state, > fse); > } > > @@ -575,7 +625,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > struct v4l2_subdev_frame_interval_enum *fie = arg; > > memset(fie->reserved, 0, sizeof(fie->reserved)); > - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state, > + return v4l2_subdev_call(sd, pad, enum_frame_interval, state, > fie); > } > > @@ -584,7 +634,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(sel->reserved, 0, sizeof(sel->reserved)); > return v4l2_subdev_call( > - sd, pad, get_selection, subdev_fh->state, sel); > + sd, pad, get_selection, state, sel); > } > > case VIDIOC_SUBDEV_S_SELECTION: { > @@ -595,7 +645,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(sel->reserved, 0, sizeof(sel->reserved)); > return v4l2_subdev_call( > - sd, pad, set_selection, subdev_fh->state, sel); > + sd, pad, set_selection, state, sel); > } > > case VIDIOC_G_EDID: { > @@ -829,10 +879,13 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, > if (is_media_entity_v4l2_subdev(pad->entity)) { > struct v4l2_subdev *sd = > media_entity_to_v4l2_subdev(pad->entity); > + struct v4l2_subdev_state *state; > + > + state = v4l2_subdev_get_active_state(sd); > > fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; > fmt->pad = pad->index; > - return v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt); > + return v4l2_subdev_call(sd, pad, get_fmt, state, fmt); Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > } > > WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, > -- > 2.25.1 >
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@ideasonboard.com> >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> 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
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; + } + + 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); + switch (cmd) { case VIDIOC_SUBDEV_QUERYCAP: { struct v4l2_subdev_capability *cap = arg; @@ -484,7 +534,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) memset(format->reserved, 0, sizeof(format->reserved)); memset(format->format.reserved, 0, sizeof(format->format.reserved)); - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format); + return v4l2_subdev_call(sd, pad, get_fmt, state, format); } case VIDIOC_SUBDEV_S_FMT: { @@ -495,7 +545,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) memset(format->reserved, 0, sizeof(format->reserved)); memset(format->format.reserved, 0, sizeof(format->format.reserved)); - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format); + return v4l2_subdev_call(sd, pad, set_fmt, state, format); } case VIDIOC_SUBDEV_G_CROP: { @@ -509,7 +559,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) sel.target = V4L2_SEL_TGT_CROP; rval = v4l2_subdev_call( - sd, pad, get_selection, subdev_fh->state, &sel); + sd, pad, get_selection, state, &sel); crop->rect = sel.r; @@ -531,7 +581,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) sel.r = crop->rect; rval = v4l2_subdev_call( - sd, pad, set_selection, subdev_fh->state, &sel); + sd, pad, set_selection, state, &sel); crop->rect = sel.r; @@ -542,7 +592,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) struct v4l2_subdev_mbus_code_enum *code = arg; memset(code->reserved, 0, sizeof(code->reserved)); - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state, + return v4l2_subdev_call(sd, pad, enum_mbus_code, state, code); } @@ -550,7 +600,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) struct v4l2_subdev_frame_size_enum *fse = arg; memset(fse->reserved, 0, sizeof(fse->reserved)); - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state, + return v4l2_subdev_call(sd, pad, enum_frame_size, state, fse); } @@ -575,7 +625,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) struct v4l2_subdev_frame_interval_enum *fie = arg; memset(fie->reserved, 0, sizeof(fie->reserved)); - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state, + return v4l2_subdev_call(sd, pad, enum_frame_interval, state, fie); } @@ -584,7 +634,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) memset(sel->reserved, 0, sizeof(sel->reserved)); return v4l2_subdev_call( - sd, pad, get_selection, subdev_fh->state, sel); + sd, pad, get_selection, state, sel); } case VIDIOC_SUBDEV_S_SELECTION: { @@ -595,7 +645,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) memset(sel->reserved, 0, sizeof(sel->reserved)); return v4l2_subdev_call( - sd, pad, set_selection, subdev_fh->state, sel); + sd, pad, set_selection, state, sel); } case VIDIOC_G_EDID: { @@ -829,10 +879,13 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, if (is_media_entity_v4l2_subdev(pad->entity)) { struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(pad->entity); + struct v4l2_subdev_state *state; + + state = v4l2_subdev_get_active_state(sd); fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; fmt->pad = pad->index; - return v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt); + return v4l2_subdev_call(sd, pad, get_fmt, state, fmt); } WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L,