| Message ID | 20211017182449.64192-11-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: Add multiplexed support to R-Car CSI-2 and GMSL | expand |
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@jmondi.org> > --- > 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@ideasonboard.com> > > -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, > }; >
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@jmondi.org> > > --- > > 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@ideasonboard.com> 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, > > }; > >
On Sun, Jan 23, 2022 at 04:20:01PM +0200, Laurent Pinchart wrote: > 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@jmondi.org> > > > --- > > > 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@ideasonboard.com> > > Actually, the rest of the driver still uses priv->mf, so this patch will > cause a breakage. You can squash it with 11/13. Or, looking at 11/13, it may be best to replace all usages of priv->mf with the format from the state in this patch, and keep 11/13 separate. > > > > > > -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, > > > }; > > >
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; - return 0; -} + 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; -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, };
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@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-csi2.c | 50 ++++++++++++--------- 1 file changed, 29 insertions(+), 21 deletions(-)