| Message ID | 20231023123308.782592-2-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/6] media: v4l: subdev: Also return pads array information on stream functions | expand |
Hi Laurent, On Mon, Oct 23, 2023 at 04:29:02PM +0300, Laurent Pinchart wrote: > > + return NULL; > > + > > + if (state->pads) { > > + if (stream) > > + return NULL; > > + > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > + pad = 0; > > + > > + return &state->pads[pad].try_fmt; > > + } > > + > > lockdep_assert_held(state->lock); > > Can we move towards proper locking for all callers ? This was never done in the existing pad ops. That's certainly a good idea, but it belongs to a new patch. I'll add one.
On Mon, Oct 23, 2023 at 08:09:53PM +0300, Laurent Pinchart wrote: > On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > > Hi Laurent, > > > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > > master branch, no mention of dependencies in the cover letter, and no > > > specified base. > > > > > > Please generate patch series with --base. > > > > That wouldn't help. > > Why not ? Because the patch the rest of the set depends on is missing. The base for the patches is indeed the media stage tree (or close to that). > > > But I've realised what the problem is. I forgot to include the first patch. > > There were six patches, but one added made that seven... > > > > I'll send v3, addressing comments found in v2. > > -- > Regards, > > Laurent Pinchart
On Mon, Oct 23, 2023 at 05:18:35PM +0000, Sakari Ailus wrote: > On Mon, Oct 23, 2023 at 08:09:53PM +0300, Laurent Pinchart wrote: > > On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > > > Hi Laurent, > > > > > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > > > master branch, no mention of dependencies in the cover letter, and no > > > > specified base. > > > > > > > > Please generate patch series with --base. > > > > > > That wouldn't help. > > > > Why not ? > > Because the patch the rest of the set depends on is missing. The base for > the patches is indeed the media stage tree (or close to that). The cover letter will then mention that. Try it out :-) > > > But I've realised what the problem is. I forgot to include the first patch. > > > There were six patches, but one added made that seven... > > > > > > I'll send v3, addressing comments found in v2.
On Mon, Oct 23, 2023 at 08:29:38PM +0300, Laurent Pinchart wrote: > On Mon, Oct 23, 2023 at 05:18:35PM +0000, Sakari Ailus wrote: > > On Mon, Oct 23, 2023 at 08:09:53PM +0300, Laurent Pinchart wrote: > > > On Mon, Oct 23, 2023 at 05:01:48PM +0000, Sakari Ailus wrote: > > > > Hi Laurent, > > > > > > > > On Mon, Oct 23, 2023 at 05:08:45PM +0300, Laurent Pinchart wrote: > > > > > > > + if (WARN_ON(pad >= state->sd->entity.num_pads)) > > > > > > > > > > There's no sd field in struct v4l2_subdev_state in the linux media > > > > > master branch, no mention of dependencies in the cover letter, and no > > > > > specified base. > > > > > > > > > > Please generate patch series with --base. > > > > > > > > That wouldn't help. > > > > > > Why not ? > > > > Because the patch the rest of the set depends on is missing. The base for > > the patches is indeed the media stage tree (or close to that). > > The cover letter will then mention that. Try it out :-) I can use it in v3 but that alone will not solve the problem. :-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index ee4fe8f33a41..955ee9a6c91f 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1684,6 +1684,19 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, struct v4l2_subdev_stream_configs *stream_configs; unsigned int i; + if (WARN_ON(!state)) + return NULL; + + if (state->pads) { + if (stream) + return NULL; + + if (WARN_ON(pad >= state->sd->entity.num_pads)) + pad = 0; + + return &state->pads[pad].try_fmt; + } + lockdep_assert_held(state->lock); stream_configs = &state->stream_configs; @@ -1705,6 +1718,19 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, struct v4l2_subdev_stream_configs *stream_configs; unsigned int i; + if (WARN_ON(!state)) + return NULL; + + if (state->pads) { + if (stream) + return NULL; + + if (WARN_ON(pad >= state->sd->entity.num_pads)) + pad = 0; + + return &state->pads[pad].try_crop; + } + lockdep_assert_held(state->lock); stream_configs = &state->stream_configs; @@ -1726,6 +1752,19 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state, struct v4l2_subdev_stream_configs *stream_configs; unsigned int i; + if (WARN_ON(!state)) + return NULL; + + if (state->pads) { + if (stream) + return NULL; + + if (WARN_ON(pad >= state->sd->entity.num_pads)) + pad = 0; + + return &state->pads[pad].try_compose; + } + lockdep_assert_held(state->lock); stream_configs = &state->stream_configs;
There are two sets of functions that return information from sub-device state, one for stream-unaware users and another for stream-aware users. Add support for stream-aware functions to return format, crop and compose information from pad-based array that are functionally equivalent to the old, stream-unaware ones. Also check state is non-NULL, in order to guard against old drivers potentially calling this with NULL state for active formats or selection rectangles. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)