| Message ID | 20240430103956.60190-18-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [01/19] media: adv748x: Add support for active state | expand |
Hi Jacpop, Thanks for your work. On 2024-04-30 12:39:53 +0200, Jacopo Mondi wrote: > Store the format in the subdevice state. Disallow setting format > on the source pads, as formats are set on the sink pad streams and > propagated to the source streams. > > Now that the driver doesn't store the active format in the > driver-specific structure, also remove the mutex and use the lock > associated with the state. Can't this whole patch be broken out to an independent patch and upstreamed already independent from the streams work? > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/platform/renesas/rcar-csi2.c | 54 +++++++--------------- > 1 file changed, 16 insertions(+), 38 deletions(-) > > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c > index ffb73272543b..ed818a6fa655 100644 > --- a/drivers/media/platform/renesas/rcar-csi2.c > +++ b/drivers/media/platform/renesas/rcar-csi2.c > @@ -621,8 +621,6 @@ struct rcar_csi2 { > > int channel_vc[4]; > > - struct mutex lock; /* Protects mf and stream_count. */ > - struct v4l2_mbus_framefmt mf; > int stream_count; > > bool cphy; > @@ -1263,43 +1261,28 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable) > } > > static int rcsi2_set_pad_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > - struct rcar_csi2 *priv = sd_to_csi2(sd); > - struct v4l2_mbus_framefmt *framefmt; > + struct v4l2_mbus_framefmt *fmt; > > - mutex_lock(&priv->lock); > + /* > + * 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; > > 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_state_get_format(sd_state, 0); > - *framefmt = format->format; > - } > > - mutex_unlock(&priv->lock); > + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); > + *fmt = format->format; > > - return 0; > -} > - > -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); > - > - mutex_lock(&priv->lock); > - > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > - format->format = priv->mf; > - else > - format->format = *v4l2_subdev_state_get_format(sd_state, 0); > - > - mutex_unlock(&priv->lock); > + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, > + format->stream); > + *fmt = format->format; > > return 0; > } > @@ -1310,7 +1293,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { > > static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { > .set_fmt = rcsi2_set_pad_format, > - .get_fmt = rcsi2_get_pad_format, > + .get_fmt = v4l2_subdev_get_fmt, > }; > > static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { > @@ -2031,20 +2014,19 @@ static int rcsi2_probe(struct platform_device *pdev) > > priv->dev = &pdev->dev; > > - mutex_init(&priv->lock); > priv->stream_count = 0; > > ret = rcsi2_probe_resources(priv, pdev); > if (ret) { > dev_err(priv->dev, "Failed to get resources\n"); > - goto error_mutex; > + return ret; > } > > platform_set_drvdata(pdev, priv); > > ret = rcsi2_parse_dt(priv); > if (ret) > - goto error_mutex; > + return ret; > > priv->subdev.owner = THIS_MODULE; > priv->subdev.dev = &pdev->dev; > @@ -2094,8 +2076,6 @@ static int rcsi2_probe(struct platform_device *pdev) > error_async: > v4l2_async_nf_unregister(&priv->notifier); > v4l2_async_nf_cleanup(&priv->notifier); > -error_mutex: > - mutex_destroy(&priv->lock); > > return ret; > } > @@ -2110,8 +2090,6 @@ static void rcsi2_remove(struct platform_device *pdev) > v4l2_subdev_cleanup(&priv->subdev); > > pm_runtime_disable(&pdev->dev); > - > - mutex_destroy(&priv->lock); > } > > static struct platform_driver rcar_csi2_pdrv = { > -- > 2.44.0 >
On Thu, May 02, 2024 at 04:32:32PM +0200, Niklas Söderlund wrote: > Hi Jacpop, > > Thanks for your work. > > On 2024-04-30 12:39:53 +0200, Jacopo Mondi wrote: > > Store the format in the subdevice state. Disallow setting format > > on the source pads, as formats are set on the sink pad streams and > > propagated to the source streams. > > > > Now that the driver doesn't store the active format in the > > driver-specific structure, also remove the mutex and use the lock > > associated with the state. > > Can't this whole patch be broken out to an independent patch and > upstreamed already independent from the streams work? I think it's a good idea. We will need to move part of 15/19 here (adding .init_state() and calling v4l2_subdev_init_finalize()). > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/platform/renesas/rcar-csi2.c | 54 +++++++--------------- > > 1 file changed, 16 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c > > index ffb73272543b..ed818a6fa655 100644 > > --- a/drivers/media/platform/renesas/rcar-csi2.c > > +++ b/drivers/media/platform/renesas/rcar-csi2.c > > @@ -621,8 +621,6 @@ struct rcar_csi2 { > > > > int channel_vc[4]; > > > > - struct mutex lock; /* Protects mf and stream_count. */ > > - struct v4l2_mbus_framefmt mf; > > int stream_count; > > > > bool cphy; > > @@ -1263,43 +1261,28 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable) > > } > > > > static int rcsi2_set_pad_format(struct v4l2_subdev *sd, > > - struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format) > > { > > - struct rcar_csi2 *priv = sd_to_csi2(sd); > > - struct v4l2_mbus_framefmt *framefmt; > > + struct v4l2_mbus_framefmt *fmt; > > > > - mutex_lock(&priv->lock); > > + /* > > + * 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 v4l2_subdev_get_fmt(sd, state, format); > > > > 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_state_get_format(sd_state, 0); > > - *framefmt = format->format; > > - } > > > > - mutex_unlock(&priv->lock); > > + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); > > + *fmt = format->format; > > > > - return 0; > > -} > > - > > -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); > > - > > - mutex_lock(&priv->lock); > > - > > - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > - format->format = priv->mf; > > - else > > - format->format = *v4l2_subdev_state_get_format(sd_state, 0); > > - > > - mutex_unlock(&priv->lock); > > + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, > > + format->stream); > > + *fmt = format->format; > > > > return 0; > > } > > @@ -1310,7 +1293,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { > > > > static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { > > .set_fmt = rcsi2_set_pad_format, > > - .get_fmt = rcsi2_get_pad_format, > > + .get_fmt = v4l2_subdev_get_fmt, > > }; > > > > static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { > > @@ -2031,20 +2014,19 @@ static int rcsi2_probe(struct platform_device *pdev) > > > > priv->dev = &pdev->dev; > > > > - mutex_init(&priv->lock); > > priv->stream_count = 0; > > > > ret = rcsi2_probe_resources(priv, pdev); > > if (ret) { > > dev_err(priv->dev, "Failed to get resources\n"); > > - goto error_mutex; > > + return ret; > > } > > > > platform_set_drvdata(pdev, priv); > > > > ret = rcsi2_parse_dt(priv); > > if (ret) > > - goto error_mutex; > > + return ret; > > > > priv->subdev.owner = THIS_MODULE; > > priv->subdev.dev = &pdev->dev; > > @@ -2094,8 +2076,6 @@ static int rcsi2_probe(struct platform_device *pdev) > > error_async: > > v4l2_async_nf_unregister(&priv->notifier); > > v4l2_async_nf_cleanup(&priv->notifier); > > -error_mutex: > > - mutex_destroy(&priv->lock); > > > > return ret; > > } > > @@ -2110,8 +2090,6 @@ static void rcsi2_remove(struct platform_device *pdev) > > v4l2_subdev_cleanup(&priv->subdev); > > > > pm_runtime_disable(&pdev->dev); > > - > > - mutex_destroy(&priv->lock); > > } > > > > static struct platform_driver rcar_csi2_pdrv = {
diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c index ffb73272543b..ed818a6fa655 100644 --- a/drivers/media/platform/renesas/rcar-csi2.c +++ b/drivers/media/platform/renesas/rcar-csi2.c @@ -621,8 +621,6 @@ struct rcar_csi2 { int channel_vc[4]; - struct mutex lock; /* Protects mf and stream_count. */ - struct v4l2_mbus_framefmt mf; int stream_count; bool cphy; @@ -1263,43 +1261,28 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable) } static int rcsi2_set_pad_format(struct v4l2_subdev *sd, - struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_state *state, struct v4l2_subdev_format *format) { - struct rcar_csi2 *priv = sd_to_csi2(sd); - struct v4l2_mbus_framefmt *framefmt; + struct v4l2_mbus_framefmt *fmt; - mutex_lock(&priv->lock); + /* + * 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; 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_state_get_format(sd_state, 0); - *framefmt = format->format; - } - mutex_unlock(&priv->lock); + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); + *fmt = format->format; - return 0; -} - -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); - - mutex_lock(&priv->lock); - - if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) - format->format = priv->mf; - else - format->format = *v4l2_subdev_state_get_format(sd_state, 0); - - mutex_unlock(&priv->lock); + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, + format->stream); + *fmt = format->format; return 0; } @@ -1310,7 +1293,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { .set_fmt = rcsi2_set_pad_format, - .get_fmt = rcsi2_get_pad_format, + .get_fmt = v4l2_subdev_get_fmt, }; static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { @@ -2031,20 +2014,19 @@ static int rcsi2_probe(struct platform_device *pdev) priv->dev = &pdev->dev; - mutex_init(&priv->lock); priv->stream_count = 0; ret = rcsi2_probe_resources(priv, pdev); if (ret) { dev_err(priv->dev, "Failed to get resources\n"); - goto error_mutex; + return ret; } platform_set_drvdata(pdev, priv); ret = rcsi2_parse_dt(priv); if (ret) - goto error_mutex; + return ret; priv->subdev.owner = THIS_MODULE; priv->subdev.dev = &pdev->dev; @@ -2094,8 +2076,6 @@ static int rcsi2_probe(struct platform_device *pdev) error_async: v4l2_async_nf_unregister(&priv->notifier); v4l2_async_nf_cleanup(&priv->notifier); -error_mutex: - mutex_destroy(&priv->lock); return ret; } @@ -2110,8 +2090,6 @@ static void rcsi2_remove(struct platform_device *pdev) v4l2_subdev_cleanup(&priv->subdev); pm_runtime_disable(&pdev->dev); - - mutex_destroy(&priv->lock); } static struct platform_driver rcar_csi2_pdrv = {
Store the format in the subdevice state. Disallow setting format on the source pads, as formats are set on the sink pad streams and propagated to the source streams. Now that the driver doesn't store the active format in the driver-specific structure, also remove the mutex and use the lock associated with the state. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- drivers/media/platform/renesas/rcar-csi2.c | 54 +++++++--------------- 1 file changed, 16 insertions(+), 38 deletions(-)