Hi Jacopo,
Thank you for the patch.
On Sun, Oct 17, 2021 at 08:24:46PM +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.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 50 ++++++++++++---------
> 1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 451a6da26e03..b60845b1e563 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -722,34 +722,42 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *format)
> {
> - struct rcar_csi2 *priv = sd_to_csi2(sd);
> - struct v4l2_mbus_framefmt *framefmt;
> + struct v4l2_mbus_framefmt *fmt;
> + struct v4l2_subdev_state *state;
> + int ret = 0;
>
> if (!rcsi2_code_to_fmt(format->format.code))
> format->format.code = rcar_csi2_formats[0].code;
>
> - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> - priv->mf = format->format;
> - } else {
> - framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> - *framefmt = format->format;
> - }
> + /*
> + * Format is propagated from sink streams to source streams, so
> + * disallow setting format on the source pads.
> + */
> + if (format->pad > RCAR_CSI2_SINK)
> + return -EINVAL;
You should return v4l2_subdev_get_fmt() instead.
>
> - return 0;
> -}
> + state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
With v10 of the streams series I think you can use
v4l2_subdev_lock_state().
> + fmt = v4l2_state_get_stream_format(state, format->pad,
> + format->stream);
> + if (!fmt) {
> + ret = -EINVAL;
> + goto out;
> + }
> + *fmt = format->format;
I would get the pointers to both formats before updating any of them, to
avoid a partial update in case of error:
sink_fmt = v4l2_state_get_stream_format(state, format->pad,
format->stream);
source_fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
format->stream);
if (!sink_fmt || !source_fmt) {
ret = -EINVAL;
goto out;
}
*sink_fmt = format->format;
*source_fmt = format->format;
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *format)
> -{
> - struct rcar_csi2 *priv = sd_to_csi2(sd);
> + /* 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;
>
> - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> - format->format = priv->mf;
> - else
> - format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> +out:
> + v4l2_subdev_unlock_state(state);
>
> - return 0;
> + return ret;
> }
>
> static int _rcsi2_set_routing(struct v4l2_subdev *sd,
> @@ -829,7 +837,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> .init_cfg = rcsi2_init_cfg,
> .set_fmt = rcsi2_set_pad_format,
> - .get_fmt = rcsi2_get_pad_format,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_routing = rcsi2_set_routing,
> };
>
--
Regards,
Laurent Pinchart
