On Sun, Jan 23, 2022 at 04:19:16PM +0200, Laurent Pinchart wrote:
> 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>
Actually, the rest of the driver still uses priv->mf, so this patch will
cause a breakage. You can squash it with 11/13.
> >
> > -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
