Hi Jacopo,
Thank you for the patch.
On Sun, Oct 17, 2021 at 08:24:41PM +0200, Jacopo Mondi wrote:
> Move format handling to the v4l2_subdev state and store it per
> (pad, stream) combination.
>
> Now that the image format is stored in the subdev state, it can be
> accessed through v4l2_subdev_get_fmt() instead of open-coding it.
Would it be possible to move this to 02/13 in the series ? Storing the
formats in the state doesn't depend on multiplexed streams support, we
could thus merge it early. The current 01/13 would need to be split in
two, with one part to allocate the active state and implement
.init_cfg() without muxed streams support, and another part to add muxed
streams support on top.
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> drivers/media/i2c/max9286.c | 85 ++++++++++++-------------------------
> 1 file changed, 27 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 3485478f08a5..e51771d99437 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -175,8 +175,6 @@ struct max9286_priv {
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixelrate;
>
> - struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> -
> /* Protects controls and fmt structures */
> struct mutex mutex;
>
> @@ -829,28 +827,18 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static struct v4l2_mbus_framefmt *
> -max9286_get_pad_format(struct max9286_priv *priv,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad, u32 which)
> -{
> - switch (which) {
> - case V4L2_SUBDEV_FORMAT_TRY:
> - return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> - case V4L2_SUBDEV_FORMAT_ACTIVE:
> - return &priv->fmt[pad];
> - default:
> - return NULL;
> - }
> -}
> -
> static int max9286_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *format)
> {
> - struct max9286_priv *priv = sd_to_max9286(sd);
> - struct v4l2_mbus_framefmt *cfg_fmt;
> + struct v4l2_mbus_framefmt *fmt;
> + struct v4l2_subdev_state *state;
> + int ret = 0;
>
> + /*
> + * Refuse to set format on the multiplexed source pad.
> + * Format is propagated from sinks streams to source streams.
> + */
> if (format->pad == MAX9286_SRC_PAD)
> return -EINVAL;
>
> @@ -866,44 +854,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> break;
> }
>
> - cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> - format->which);
> - if (!cfg_fmt)
> - return -EINVAL;
> -
> - mutex_lock(&priv->mutex);
> - *cfg_fmt = format->format;
> - mutex_unlock(&priv->mutex);
> -
> - return 0;
> -}
> -
> -static int max9286_get_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *format)
> -{
> - struct max9286_priv *priv = sd_to_max9286(sd);
> - struct v4l2_mbus_framefmt *cfg_fmt;
> - unsigned int pad = format->pad;
> -
> - /*
> - * Multiplexed Stream Support: Support link validation by returning the
> - * format of the first bound link. All links must have the same format,
> - * as we do not support mixing and matching of cameras connected to the
> - * max9286.
> - */
> - if (pad == MAX9286_SRC_PAD)
> - pad = __ffs(priv->bound_sources);
> + state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
> + fmt = v4l2_state_get_stream_format(state, format->pad,
> + format->stream);
> + if (!fmt) {
> + ret = -EINVAL;
> + goto out;
> + }
> + *fmt = format->format;
>
> - cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> - if (!cfg_fmt)
> - return -EINVAL;
> + /* Propagate format to the other end of the route. */
> + fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> + format->stream);
> + if (!fmt) {
> + ret = -EINVAL;
> + goto out;
> + }
> + *fmt = format->format;
>
> - mutex_lock(&priv->mutex);
> - format->format = *cfg_fmt;
> - mutex_unlock(&priv->mutex);
> +out:
> + v4l2_subdev_unlock_state(state);
>
> - return 0;
> + return ret;
> }
>
> static int max9286_routing_verify(struct max9286_priv *priv,
> @@ -1052,7 +1024,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
> static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> .init_cfg = max9286_init_cfg,
> .enum_mbus_code = max9286_enum_mbus_code,
> - .get_fmt = max9286_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = max9286_set_fmt,
> .set_routing = max9286_set_routing,
> };
> @@ -1092,9 +1064,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>
> /* Configure V4L2 for the MAX9286 itself */
>
> - for (i = 0; i < MAX9286_N_SINKS; i++)
> - priv->fmt[i] = max9286_default_format;
> -
> v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> V4L2_SUBDEV_FL_MULTIPLEXED;
> --
> 2.33.0
>
--
Regards,
Laurent Pinchart
