| Message ID | 20231023174408.803874-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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:02PM +0300, Sakari Ailus wrote: > 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 +++++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 9 ++++--- > 2 files changed, 45 insertions(+), 3 deletions(-) > > 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; Do we want to carry this forward from the existing pad config accessors, can't we return NULL instead ? Either way, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + 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; > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 6a02a565035c..0ba1cd8c3ac7 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1550,7 +1550,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > * This returns a pointer to &struct v4l2_mbus_framefmt for the given pad + > * stream in the subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the format for the corresponding pad is returned. > + * If the pad does not exist, NULL is returned. > */ > struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > @@ -1565,7 +1566,8 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > * This returns a pointer to crop rectangle for the given pad + stream in the > * subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the crop rectangle for the corresponding pad is > + * returned. If the pad does not exist, NULL is returned. > */ > struct v4l2_rect * > v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > @@ -1581,7 +1583,8 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > * This returns a pointer to compose rectangle for the given pad + stream in the > * subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the compose rectangle for the corresponding pad is > + * returned. If the pad does not exist, NULL is returned. > */ > struct v4l2_rect * > v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
Hi Laurent, On Tue, Oct 24, 2023 at 01:07:29AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Mon, Oct 23, 2023 at 08:44:02PM +0300, Sakari Ailus wrote: > > 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 +++++++++++++++++++++++++++ > > include/media/v4l2-subdev.h | 9 ++++--- > > 2 files changed, 45 insertions(+), 3 deletions(-) > > > > 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; > > Do we want to carry this forward from the existing pad config accessors, > can't we return NULL instead ? I also think this should be changed, but again, this should be done in a separate patch. I'd also postpone it after this set is merged. > > Either way, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks!
On 23/10/2023 20:44, Sakari Ailus wrote: > 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 +++++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 9 ++++--- > 2 files changed, 45 insertions(+), 3 deletions(-) > > 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 this happens, is it a driver error? WARN_ON()? Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi > + > + 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; > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 6a02a565035c..0ba1cd8c3ac7 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1550,7 +1550,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > * This returns a pointer to &struct v4l2_mbus_framefmt for the given pad + > * stream in the subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the format for the corresponding pad is returned. > + * If the pad does not exist, NULL is returned. > */ > struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > @@ -1565,7 +1566,8 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > * This returns a pointer to crop rectangle for the given pad + stream in the > * subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the crop rectangle for the corresponding pad is > + * returned. If the pad does not exist, NULL is returned. > */ > struct v4l2_rect * > v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > @@ -1581,7 +1583,8 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > * This returns a pointer to compose rectangle for the given pad + stream in the > * subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the compose rectangle for the corresponding pad is > + * returned. If the pad does not exist, NULL is returned. > */ > struct v4l2_rect * > v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
Hi Laurent, On Tue, Oct 24, 2023 at 05:45:27PM +0300, Tomi Valkeinen wrote: > On 23/10/2023 20:44, Sakari Ailus wrote: > > 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 +++++++++++++++++++++++++++ > > include/media/v4l2-subdev.h | 9 ++++--- > > 2 files changed, 45 insertions(+), 3 deletions(-) > > > > 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 this happens, is it a driver error? WARN_ON()? The caller has used stream-unaware state and still specified a stream. It's a driver bug. A WARN_ON() wouldn't be unreasonable in this case IMO. > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Thanks!
On 23/10/2023 19:44, Sakari Ailus wrote: > 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 +++++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 9 ++++--- > 2 files changed, 45 insertions(+), 3 deletions(-) > > 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; I saw Laurent's comment on why we don't return NULL here, and I think based on your reply to that this will need a comment why we don't return NULL (yet). It's weird code, and it needs an explanation. Ditto elsewhere, of course. > + > + 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; > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 6a02a565035c..0ba1cd8c3ac7 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1550,7 +1550,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > * This returns a pointer to &struct v4l2_mbus_framefmt for the given pad + > * stream in the subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the format for the corresponding pad is returned. > + * If the pad does not exist, NULL is returned. > */ > struct v4l2_mbus_framefmt * > v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > @@ -1565,7 +1566,8 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > * This returns a pointer to crop rectangle for the given pad + stream in the > * subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the crop rectangle for the corresponding pad is > + * returned. If the pad does not exist, NULL is returned. > */ > struct v4l2_rect * > v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > @@ -1581,7 +1583,8 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > * This returns a pointer to compose rectangle for the given pad + stream in the > * subdev state. > * > - * If the state does not contain the given pad + stream, NULL is returned. > + * For stream-unaware drivers the compose rectangle for the corresponding pad is > + * returned. If the pad does not exist, NULL is returned. > */ > struct v4l2_rect * > v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state, Regards, Hans
Hi Hans, On Wed, Oct 25, 2023 at 11:32:11AM +0200, Hans Verkuil wrote: > On 23/10/2023 19:44, Sakari Ailus wrote: > > 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 +++++++++++++++++++++++++++ > > include/media/v4l2-subdev.h | 9 ++++--- > > 2 files changed, 45 insertions(+), 3 deletions(-) > > > > 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; > > I saw Laurent's comment on why we don't return NULL here, and I think > based on your reply to that this will need a comment why we don't > return NULL (yet). It's weird code, and it needs an explanation. I can add a comment, sure. I'll also add a patch to just return NULL here in the set.
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; diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 6a02a565035c..0ba1cd8c3ac7 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1550,7 +1550,8 @@ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, * This returns a pointer to &struct v4l2_mbus_framefmt for the given pad + * stream in the subdev state. * - * If the state does not contain the given pad + stream, NULL is returned. + * For stream-unaware drivers the format for the corresponding pad is returned. + * If the pad does not exist, NULL is returned. */ struct v4l2_mbus_framefmt * v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, @@ -1565,7 +1566,8 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, * This returns a pointer to crop rectangle for the given pad + stream in the * subdev state. * - * If the state does not contain the given pad + stream, NULL is returned. + * For stream-unaware drivers the crop rectangle for the corresponding pad is + * returned. If the pad does not exist, NULL is returned. */ struct v4l2_rect * v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, @@ -1581,7 +1583,8 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, * This returns a pointer to compose rectangle for the given pad + stream in the * subdev state. * - * If the state does not contain the given pad + stream, NULL is returned. + * For stream-unaware drivers the compose rectangle for the corresponding pad is + * returned. If the pad does not exist, NULL is returned. */ struct v4l2_rect * v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
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 +++++++++++++++++++++++++++ include/media/v4l2-subdev.h | 9 ++++--- 2 files changed, 45 insertions(+), 3 deletions(-)