Hi Laurent, On Tue, Dec 05, 2023 at 03:10:54PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Dec 05, 2023 at 03:00:01PM +0200, Sakari Ailus wrote: > > The sub-device state information access function were only available if > > CONFIG_MEDIA_CONTROLLER was defined whereas the functions themselves were > > used by non-MC-enabled drivers, too. Fix this by moving the definitions > > out of CONFIG_MEDIA_CONTROLLER dependent parts. Also make the MC dependent > > features conditional to CONFIG_MEDIA_CONTROLLER. > > > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Closes: https://lore.kernel.org/oe-kbuild-all/202312051913.e5iif8Qz-lkp@xxxxxxxxx/ > > Fixes: bc0e8d91feec ("media: v4l: subdev: Switch to stream-aware state functions") > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > --- > > Thanks to Laurent for review. > > > > since v1: > > > > - I thought the c file had already this fixed. Also fixed that now and > > changed the commit message accordingly. > > > > drivers/media/v4l2-core/v4l2-subdev.c | 216 ++++++++++++++------------ > > include/media/v4l2-subdev.h | 158 +++++++++---------- > > 2 files changed, 193 insertions(+), 181 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 4fbefe4cd714..30746a7df259 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -1533,108 +1533,6 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd) > > } > > EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup); > > > > -struct v4l2_mbus_framefmt * > > -__v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, > > - unsigned int pad, u32 stream) > > -{ > > - struct v4l2_subdev_stream_configs *stream_configs; > > - unsigned int i; > > - > > - if (WARN_ON_ONCE(!state)) > > - return NULL; > > - > > - if (state->pads) { > > - if (stream) > > - return NULL; > > - > > - if (pad >= state->sd->entity.num_pads) > > - return NULL; > > - > > - return &state->pads[pad].format; > > - } > > - > > - lockdep_assert_held(state->lock); > > - > > - stream_configs = &state->stream_configs; > > - > > - for (i = 0; i < stream_configs->num_configs; ++i) { > > - if (stream_configs->configs[i].pad == pad && > > - stream_configs->configs[i].stream == stream) > > - return &stream_configs->configs[i].fmt; > > - } > > - > > - return NULL; > > -} > > -EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_format); > > - > > -struct v4l2_rect * > > -__v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > > - u32 stream) > > -{ > > - struct v4l2_subdev_stream_configs *stream_configs; > > - unsigned int i; > > - > > - if (WARN_ON_ONCE(!state)) > > - return NULL; > > - > > - if (state->pads) { > > - if (stream) > > - return NULL; > > - > > - if (pad >= state->sd->entity.num_pads) > > - return NULL; > > - > > - return &state->pads[pad].crop; > > - } > > - > > - lockdep_assert_held(state->lock); > > - > > - stream_configs = &state->stream_configs; > > - > > - for (i = 0; i < stream_configs->num_configs; ++i) { > > - if (stream_configs->configs[i].pad == pad && > > - stream_configs->configs[i].stream == stream) > > - return &stream_configs->configs[i].crop; > > - } > > - > > - return NULL; > > -} > > -EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_crop); > > - > > -struct v4l2_rect * > > -__v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, > > - unsigned int pad, u32 stream) > > -{ > > - struct v4l2_subdev_stream_configs *stream_configs; > > - unsigned int i; > > - > > - if (WARN_ON_ONCE(!state)) > > - return NULL; > > - > > - if (state->pads) { > > - if (stream) > > - return NULL; > > - > > - if (pad >= state->sd->entity.num_pads) > > - return NULL; > > - > > - return &state->pads[pad].compose; > > - } > > - > > - lockdep_assert_held(state->lock); > > - > > - stream_configs = &state->stream_configs; > > - > > - for (i = 0; i < stream_configs->num_configs; ++i) { > > - if (stream_configs->configs[i].pad == pad && > > - stream_configs->configs[i].stream == stream) > > - return &stream_configs->configs[i].compose; > > - } > > - > > - return NULL; > > -} > > -EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_compose); > > - > > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > > > > static int > > @@ -2272,6 +2170,120 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper); > > > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > > > +struct v4l2_mbus_framefmt * > > +__v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, > > + unsigned int pad, u32 stream) > > +{ > > + struct v4l2_subdev_stream_configs *stream_configs; > > + unsigned int i; > > + > > + if (WARN_ON_ONCE(!state)) > > + return NULL; > > + > > + if (state->pads) { > > + if (stream) > > + return NULL; > > + > > + if (pad >= state->sd->entity.num_pads) > > This won't work, v4l2_subdev has no entity field when > CONFIG_MEDIA_CONTROLLER is not defined. Please compile-test further > versions of this patch. > > Now, for an attempt to review v3 before you post it: dropping the > > #if defined(CONFIG_MEDIA_CONTROLLER) > > around the entity field of v4l2_subdev won't work either, as the > saa6752hs driver doesn't initialize the entity, so num_fields will be at > best memset to 0, at worst random. This check will then always fail. I'll prepare v3, as discussed, to address the issues in the drivers instead. I expect to post patches for that later this week. -- Regards, Sakari Ailus
