| Message ID | 20240430103956.60190-2-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/19] media: adv748x: Add support for active state | expand |
Hi Jacopo, Thank you for the patch. On Tue, Apr 30, 2024 at 12:39:37PM +0200, Jacopo Mondi wrote: > Initialize and use the subdev active state to store the subdevice > format. > > This simplifies the implementation of the get_fmt and set_fmt pad > operations. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/adv748x/adv748x-csi2.c | 69 ++++-------------------- > drivers/media/i2c/adv748x/adv748x.h | 1 - > 2 files changed, 11 insertions(+), 59 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > index 5b265b722394..435b0909bbef 100644 > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > @@ -139,78 +139,26 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { > * But we must support setting the pad formats for format propagation. > */ > > -static struct v4l2_mbus_framefmt * > -adv748x_csi2_get_pad_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - unsigned int pad, u32 which) > -{ > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > - > - if (which == V4L2_SUBDEV_FORMAT_TRY) > - return v4l2_subdev_state_get_format(sd_state, pad); > - > - return &tx->format; > -} > - > -static int adv748x_csi2_get_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *sdformat) > -{ > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > - struct adv748x_state *state = tx->state; > - struct v4l2_mbus_framefmt *mbusformat; > - > - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > - sdformat->which); > - if (!mbusformat) > - return -EINVAL; > - > - mutex_lock(&state->mutex); > - > - sdformat->format = *mbusformat; > - > - mutex_unlock(&state->mutex); > - > - return 0; > -} > - > static int adv748x_csi2_set_format(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_format *sdformat) > { > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > - struct adv748x_state *state = tx->state; > struct v4l2_mbus_framefmt *mbusformat; > - int ret = 0; > - > - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > - sdformat->which); > - if (!mbusformat) > - return -EINVAL; > > - mutex_lock(&state->mutex); > + mbusformat = v4l2_subdev_state_get_format(sd_state, sdformat->pad); > > + /* Format on the source pad is always copied from the sink one. */ > if (sdformat->pad == ADV748X_CSI2_SOURCE) { > const struct v4l2_mbus_framefmt *sink_fmt; > > - sink_fmt = adv748x_csi2_get_pad_format(sd, sd_state, > - ADV748X_CSI2_SINK, > - sdformat->which); > - > - if (!sink_fmt) { > - ret = -EINVAL; > - goto unlock; > - } > - > + sink_fmt = v4l2_subdev_state_get_format(sd_state, > + ADV748X_CSI2_SINK); > sdformat->format = *sink_fmt; That's not the right way to do it. You should propagate the format from sink to source when pad == ADV748X_CSI2_SINK, and return adv748x_csi2_get_format() when pad == ADV748X_CSI2_SOURCE. Otherwise setting the format on the sink pad will not update the state of the source pad, and a get format call on the source pad will return an incorrect format. > } > > *mbusformat = sdformat->format; > > -unlock: > - mutex_unlock(&state->mutex); > - > - return ret; > + return 0; > } > > static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > @@ -228,7 +176,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad > } > > static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = { > - .get_fmt = adv748x_csi2_get_format, > + .get_fmt = v4l2_subdev_get_fmt, > .set_fmt = adv748x_csi2_set_format, > .get_mbus_config = adv748x_csi2_get_mbus_config, > }; > @@ -320,6 +268,11 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > if (ret) > goto err_cleanup_subdev; > > + tx->sd.state_lock = tx->ctrl_hdl.lock; Maybe that's addressed in subsequent patches, but do we need a device-wide lock ? The code you replace above uses the adv748x_state.mutex lock, which covers all subdevs. I don't think this patch introduces race conditions, so this could possibly be handled on top. > + ret = v4l2_subdev_init_finalize(&tx->sd); > + if (ret) > + goto err_free_ctrl; > + > ret = v4l2_async_register_subdev(&tx->sd); > if (ret) > goto err_free_ctrl; > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > index d2b5e722e997..9bc0121d0eff 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -75,7 +75,6 @@ enum adv748x_csi2_pads { > > struct adv748x_csi2 { > struct adv748x_state *state; > - struct v4l2_mbus_framefmt format; > unsigned int page; > unsigned int port; > unsigned int num_lanes;
Hi Laurent On Thu, May 02, 2024 at 08:34:30PM GMT, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Apr 30, 2024 at 12:39:37PM +0200, Jacopo Mondi wrote: > > Initialize and use the subdev active state to store the subdevice > > format. > > > > This simplifies the implementation of the get_fmt and set_fmt pad > > operations. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/i2c/adv748x/adv748x-csi2.c | 69 ++++-------------------- > > drivers/media/i2c/adv748x/adv748x.h | 1 - > > 2 files changed, 11 insertions(+), 59 deletions(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > index 5b265b722394..435b0909bbef 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > @@ -139,78 +139,26 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { > > * But we must support setting the pad formats for format propagation. > > */ > > > > -static struct v4l2_mbus_framefmt * > > -adv748x_csi2_get_pad_format(struct v4l2_subdev *sd, > > - struct v4l2_subdev_state *sd_state, > > - unsigned int pad, u32 which) > > -{ > > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > - > > - if (which == V4L2_SUBDEV_FORMAT_TRY) > > - return v4l2_subdev_state_get_format(sd_state, pad); > > - > > - return &tx->format; > > -} > > - > > -static int adv748x_csi2_get_format(struct v4l2_subdev *sd, > > - struct v4l2_subdev_state *sd_state, > > - struct v4l2_subdev_format *sdformat) > > -{ > > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > - struct adv748x_state *state = tx->state; > > - struct v4l2_mbus_framefmt *mbusformat; > > - > > - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > > - sdformat->which); > > - if (!mbusformat) > > - return -EINVAL; > > - > > - mutex_lock(&state->mutex); > > - > > - sdformat->format = *mbusformat; > > - > > - mutex_unlock(&state->mutex); > > - > > - return 0; > > -} > > - > > static int adv748x_csi2_set_format(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_format *sdformat) > > { > > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > - struct adv748x_state *state = tx->state; > > struct v4l2_mbus_framefmt *mbusformat; > > - int ret = 0; > > - > > - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > > - sdformat->which); > > - if (!mbusformat) > > - return -EINVAL; > > > > - mutex_lock(&state->mutex); > > + mbusformat = v4l2_subdev_state_get_format(sd_state, sdformat->pad); > > > > + /* Format on the source pad is always copied from the sink one. */ > > if (sdformat->pad == ADV748X_CSI2_SOURCE) { > > const struct v4l2_mbus_framefmt *sink_fmt; > > > > - sink_fmt = adv748x_csi2_get_pad_format(sd, sd_state, > > - ADV748X_CSI2_SINK, > > - sdformat->which); > > - > > - if (!sink_fmt) { > > - ret = -EINVAL; > > - goto unlock; > > - } > > - > > + sink_fmt = v4l2_subdev_state_get_format(sd_state, > > + ADV748X_CSI2_SINK); > > sdformat->format = *sink_fmt; > > That's not the right way to do it. You should propagate the format from > sink to source when pad == ADV748X_CSI2_SINK, and return > adv748x_csi2_get_format() when pad == ADV748X_CSI2_SOURCE. Otherwise I think it's done later and this patch doesn't change the currently implemented behaviour, doesn't it ? Anyway, I got that you would prefer to squash the first patches in a single one, so this will be solved > setting the format on the sink pad will not update the state of the > source pad, and a get format call on the source pad will return an > incorrect format. > > > } > > > > *mbusformat = sdformat->format; > > > > -unlock: > > - mutex_unlock(&state->mutex); > > - > > - return ret; > > + return 0; > > } > > > > static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > > @@ -228,7 +176,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad > > } > > > > static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = { > > - .get_fmt = adv748x_csi2_get_format, > > + .get_fmt = v4l2_subdev_get_fmt, > > .set_fmt = adv748x_csi2_set_format, > > .get_mbus_config = adv748x_csi2_get_mbus_config, > > }; > > @@ -320,6 +268,11 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > if (ret) > > goto err_cleanup_subdev; > > > > + tx->sd.state_lock = tx->ctrl_hdl.lock; > > Maybe that's addressed in subsequent patches, but do we need a > device-wide lock ? The code you replace above uses the device-wide as global to the CSI-2, the HDMI and the AFE subdevices ? > adv748x_state.mutex lock, which covers all subdevs. I don't think this > patch introduces race conditions, so this could possibly be handled on > top. > > > + ret = v4l2_subdev_init_finalize(&tx->sd); > > + if (ret) > > + goto err_free_ctrl; > > + > > ret = v4l2_async_register_subdev(&tx->sd); > > if (ret) > > goto err_free_ctrl; > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > index d2b5e722e997..9bc0121d0eff 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -75,7 +75,6 @@ enum adv748x_csi2_pads { > > > > struct adv748x_csi2 { > > struct adv748x_state *state; > > - struct v4l2_mbus_framefmt format; > > unsigned int page; > > unsigned int port; > > unsigned int num_lanes; > > -- > Regards, > > Laurent Pinchart
On Fri, May 03, 2024 at 09:55:07AM +0200, Jacopo Mondi wrote: > Hi Laurent > > On Thu, May 02, 2024 at 08:34:30PM GMT, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Tue, Apr 30, 2024 at 12:39:37PM +0200, Jacopo Mondi wrote: > > > Initialize and use the subdev active state to store the subdevice > > > format. > > > > > > This simplifies the implementation of the get_fmt and set_fmt pad > > > operations. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > drivers/media/i2c/adv748x/adv748x-csi2.c | 69 ++++-------------------- > > > drivers/media/i2c/adv748x/adv748x.h | 1 - > > > 2 files changed, 11 insertions(+), 59 deletions(-) > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > index 5b265b722394..435b0909bbef 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > @@ -139,78 +139,26 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { > > > * But we must support setting the pad formats for format propagation. > > > */ > > > > > > -static struct v4l2_mbus_framefmt * > > > -adv748x_csi2_get_pad_format(struct v4l2_subdev *sd, > > > - struct v4l2_subdev_state *sd_state, > > > - unsigned int pad, u32 which) > > > -{ > > > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > > - > > > - if (which == V4L2_SUBDEV_FORMAT_TRY) > > > - return v4l2_subdev_state_get_format(sd_state, pad); > > > - > > > - return &tx->format; > > > -} > > > - > > > -static int adv748x_csi2_get_format(struct v4l2_subdev *sd, > > > - struct v4l2_subdev_state *sd_state, > > > - struct v4l2_subdev_format *sdformat) > > > -{ > > > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > > - struct adv748x_state *state = tx->state; > > > - struct v4l2_mbus_framefmt *mbusformat; > > > - > > > - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > > > - sdformat->which); > > > - if (!mbusformat) > > > - return -EINVAL; > > > - > > > - mutex_lock(&state->mutex); > > > - > > > - sdformat->format = *mbusformat; > > > - > > > - mutex_unlock(&state->mutex); > > > - > > > - return 0; > > > -} > > > - > > > static int adv748x_csi2_set_format(struct v4l2_subdev *sd, > > > struct v4l2_subdev_state *sd_state, > > > struct v4l2_subdev_format *sdformat) > > > { > > > - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > > - struct adv748x_state *state = tx->state; > > > struct v4l2_mbus_framefmt *mbusformat; > > > - int ret = 0; > > > - > > > - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > > > - sdformat->which); > > > - if (!mbusformat) > > > - return -EINVAL; > > > > > > - mutex_lock(&state->mutex); > > > + mbusformat = v4l2_subdev_state_get_format(sd_state, sdformat->pad); > > > > > > + /* Format on the source pad is always copied from the sink one. */ > > > if (sdformat->pad == ADV748X_CSI2_SOURCE) { > > > const struct v4l2_mbus_framefmt *sink_fmt; > > > > > > - sink_fmt = adv748x_csi2_get_pad_format(sd, sd_state, > > > - ADV748X_CSI2_SINK, > > > - sdformat->which); > > > - > > > - if (!sink_fmt) { > > > - ret = -EINVAL; > > > - goto unlock; > > > - } > > > - > > > + sink_fmt = v4l2_subdev_state_get_format(sd_state, > > > + ADV748X_CSI2_SINK); > > > sdformat->format = *sink_fmt; > > > > That's not the right way to do it. You should propagate the format from > > sink to source when pad == ADV748X_CSI2_SINK, and return > > adv748x_csi2_get_format() when pad == ADV748X_CSI2_SOURCE. Otherwise > > I think it's done later I didn't know when I reviewed this patch :-) > and this patch doesn't change the currently implemented behaviour, > doesn't it ? I think it does. > Anyway, I got that you would prefer to squash the first patches in a > single one, so this will be solved Agreed. > > setting the format on the sink pad will not update the state of the > > source pad, and a get format call on the source pad will return an > > incorrect format. > > > > > } > > > > > > *mbusformat = sdformat->format; > > > > > > -unlock: > > > - mutex_unlock(&state->mutex); > > > - > > > - return ret; > > > + return 0; > > > } > > > > > > static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > > > @@ -228,7 +176,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad > > > } > > > > > > static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = { > > > - .get_fmt = adv748x_csi2_get_format, > > > + .get_fmt = v4l2_subdev_get_fmt, > > > .set_fmt = adv748x_csi2_set_format, > > > .get_mbus_config = adv748x_csi2_get_mbus_config, > > > }; > > > @@ -320,6 +268,11 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > > if (ret) > > > goto err_cleanup_subdev; > > > > > > + tx->sd.state_lock = tx->ctrl_hdl.lock; > > > > Maybe that's addressed in subsequent patches, but do we need a > > device-wide lock ? The code you replace above uses the > > device-wide as global to the CSI-2, the HDMI and the AFE subdevices ? Yes. Multi-subdev drivers like CCS do so, because controls of one subdev influence operations on other subdevs. I haven't checked if it's actually required for adv748x. > > adv748x_state.mutex lock, which covers all subdevs. I don't think this > > patch introduces race conditions, so this could possibly be handled on > > top. > > > > > + ret = v4l2_subdev_init_finalize(&tx->sd); > > > + if (ret) > > > + goto err_free_ctrl; > > > + > > > ret = v4l2_async_register_subdev(&tx->sd); > > > if (ret) > > > goto err_free_ctrl; > > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > > index d2b5e722e997..9bc0121d0eff 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x.h > > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > > @@ -75,7 +75,6 @@ enum adv748x_csi2_pads { > > > > > > struct adv748x_csi2 { > > > struct adv748x_state *state; > > > - struct v4l2_mbus_framefmt format; > > > unsigned int page; > > > unsigned int port; > > > unsigned int num_lanes;
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c index 5b265b722394..435b0909bbef 100644 --- a/drivers/media/i2c/adv748x/adv748x-csi2.c +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c @@ -139,78 +139,26 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { * But we must support setting the pad formats for format propagation. */ -static struct v4l2_mbus_framefmt * -adv748x_csi2_get_pad_format(struct v4l2_subdev *sd, - struct v4l2_subdev_state *sd_state, - unsigned int pad, u32 which) -{ - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); - - if (which == V4L2_SUBDEV_FORMAT_TRY) - return v4l2_subdev_state_get_format(sd_state, pad); - - return &tx->format; -} - -static int adv748x_csi2_get_format(struct v4l2_subdev *sd, - struct v4l2_subdev_state *sd_state, - struct v4l2_subdev_format *sdformat) -{ - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); - struct adv748x_state *state = tx->state; - struct v4l2_mbus_framefmt *mbusformat; - - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, - sdformat->which); - if (!mbusformat) - return -EINVAL; - - mutex_lock(&state->mutex); - - sdformat->format = *mbusformat; - - mutex_unlock(&state->mutex); - - return 0; -} - static int adv748x_csi2_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_format *sdformat) { - struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); - struct adv748x_state *state = tx->state; struct v4l2_mbus_framefmt *mbusformat; - int ret = 0; - - mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, - sdformat->which); - if (!mbusformat) - return -EINVAL; - mutex_lock(&state->mutex); + mbusformat = v4l2_subdev_state_get_format(sd_state, sdformat->pad); + /* Format on the source pad is always copied from the sink one. */ if (sdformat->pad == ADV748X_CSI2_SOURCE) { const struct v4l2_mbus_framefmt *sink_fmt; - sink_fmt = adv748x_csi2_get_pad_format(sd, sd_state, - ADV748X_CSI2_SINK, - sdformat->which); - - if (!sink_fmt) { - ret = -EINVAL; - goto unlock; - } - + sink_fmt = v4l2_subdev_state_get_format(sd_state, + ADV748X_CSI2_SINK); sdformat->format = *sink_fmt; } *mbusformat = sdformat->format; -unlock: - mutex_unlock(&state->mutex); - - return ret; + return 0; } static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, @@ -228,7 +176,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad } static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = { - .get_fmt = adv748x_csi2_get_format, + .get_fmt = v4l2_subdev_get_fmt, .set_fmt = adv748x_csi2_set_format, .get_mbus_config = adv748x_csi2_get_mbus_config, }; @@ -320,6 +268,11 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) if (ret) goto err_cleanup_subdev; + tx->sd.state_lock = tx->ctrl_hdl.lock; + ret = v4l2_subdev_init_finalize(&tx->sd); + if (ret) + goto err_free_ctrl; + ret = v4l2_async_register_subdev(&tx->sd); if (ret) goto err_free_ctrl; diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h index d2b5e722e997..9bc0121d0eff 100644 --- a/drivers/media/i2c/adv748x/adv748x.h +++ b/drivers/media/i2c/adv748x/adv748x.h @@ -75,7 +75,6 @@ enum adv748x_csi2_pads { struct adv748x_csi2 { struct adv748x_state *state; - struct v4l2_mbus_framefmt format; unsigned int page; unsigned int port; unsigned int num_lanes;
Initialize and use the subdev active state to store the subdevice format. This simplifies the implementation of the get_fmt and set_fmt pad operations. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- drivers/media/i2c/adv748x/adv748x-csi2.c | 69 ++++-------------------- drivers/media/i2c/adv748x/adv748x.h | 1 - 2 files changed, 11 insertions(+), 59 deletions(-)