| Message ID | 20231023174408.803874-9-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Unify sub-device state access functions | expand |
Hi Sakari, Thank you for the patch. On Mon, Oct 23, 2023 at 08:44:08PM +0300, Sakari Ailus wrote: > Assert mutex is taken by drivers that are not stream-aware. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Do you expect the assertions to trigger ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index e35226587244..8d079ad3287e 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1680,6 +1680,8 @@ __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, > if (WARN_ON(!state)) > return NULL; > > + lockdep_assert_held(state->lock); > + > if (state->pads) { > if (stream) > return NULL; > @@ -1690,8 +1692,6 @@ __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, > return &state->pads[pad].try_fmt; > } > > - lockdep_assert_held(state->lock); > - > stream_configs = &state->stream_configs; > > for (i = 0; i < stream_configs->num_configs; ++i) { > @@ -1714,6 +1714,8 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > if (WARN_ON(!state)) > return NULL; > > + lockdep_assert_held(state->lock); > + > if (state->pads) { > if (stream) > return NULL; > @@ -1724,8 +1726,6 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > return &state->pads[pad].try_crop; > } > > - lockdep_assert_held(state->lock); > - > stream_configs = &state->stream_configs; > > for (i = 0; i < stream_configs->num_configs; ++i) { > @@ -1748,6 +1748,8 @@ __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, > if (WARN_ON(!state)) > return NULL; > > + lockdep_assert_held(state->lock); > + > if (state->pads) { > if (stream) > return NULL; > @@ -1758,8 +1760,6 @@ __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, > return &state->pads[pad].try_compose; > } > > - lockdep_assert_held(state->lock); > - > stream_configs = &state->stream_configs; > > for (i = 0; i < stream_configs->num_configs; ++i) {
Hi Laurent, On Tue, Oct 24, 2023 at 01:14:53AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Mon, Oct 23, 2023 at 08:44:08PM +0300, Sakari Ailus wrote: > > Assert mutex is taken by drivers that are not stream-aware. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Do you expect the assertions to trigger ? Good question. We should actually probably postpone this patch for now and test more before merging. Drivers that have not been converted to sub-device state also use these functions on subdev ops where the framework doesn't take the state lock such as s_stream. The drivers use their own locks to serialise access to the data but because the state lock hasn't been acquired, there's going to be a warning. The best option would probably be to also acquire the lock for s_stream but that's not a trivial change as multiple sub-devices may use the same lock so special handling will be required.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index e35226587244..8d079ad3287e 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1680,6 +1680,8 @@ __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, if (WARN_ON(!state)) return NULL; + lockdep_assert_held(state->lock); + if (state->pads) { if (stream) return NULL; @@ -1690,8 +1692,6 @@ __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, return &state->pads[pad].try_fmt; } - lockdep_assert_held(state->lock); - stream_configs = &state->stream_configs; for (i = 0; i < stream_configs->num_configs; ++i) { @@ -1714,6 +1714,8 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, if (WARN_ON(!state)) return NULL; + lockdep_assert_held(state->lock); + if (state->pads) { if (stream) return NULL; @@ -1724,8 +1726,6 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, return &state->pads[pad].try_crop; } - lockdep_assert_held(state->lock); - stream_configs = &state->stream_configs; for (i = 0; i < stream_configs->num_configs; ++i) { @@ -1748,6 +1748,8 @@ __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, if (WARN_ON(!state)) return NULL; + lockdep_assert_held(state->lock); + if (state->pads) { if (stream) return NULL; @@ -1758,8 +1760,6 @@ __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, return &state->pads[pad].try_compose; } - lockdep_assert_held(state->lock); - stream_configs = &state->stream_configs; for (i = 0; i < stream_configs->num_configs; ++i) {
Assert mutex is taken by drivers that are not stream-aware. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)