| Message ID | 20240430103956.60190-7-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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:42PM +0200, Jacopo Mondi wrote: > Use the newly introduced routing table to configure on which MIPI > CSI-2 Virtual Channel to send the image stream on. The stream ID in the routing API isn't meant to be mapped directly to a virtual channel number. Sakari, your opinion would be appreciated. > Perform Virtual Channel selection at s_stream() time instead of > forcing it to 0 during the chip reset. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/adv748x/adv748x-core.c | 8 ++------ > drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++-- > drivers/media/i2c/adv748x/adv748x.h | 1 - > 3 files changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > index 3abc73ea8ccb..c9d917135709 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -530,14 +530,10 @@ static int adv748x_reset(struct adv748x_state *state) > io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); > > /* Conditionally enable TXa and TXb. */ > - if (is_tx_enabled(&state->txa)) { > + if (is_tx_enabled(&state->txa)) > regval |= ADV748X_IO_10_CSI4_EN; > - adv748x_csi2_set_virtual_channel(&state->txa, 0); > - } > - if (is_tx_enabled(&state->txb)) { > + if (is_tx_enabled(&state->txb)) > regval |= ADV748X_IO_10_CSI1_EN; > - adv748x_csi2_set_virtual_channel(&state->txb, 0); > - } > io_write(state, ADV748X_IO_10, regval); > > /* Use vid_std and v_freq as freerun resolution for CP */ > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > index 7fa72340e66e..a7bfed393ff0 100644 > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > @@ -14,7 +14,8 @@ > > #include "adv748x.h" > > -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc) > +static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, > + unsigned int vc) > { > return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT); > } > @@ -175,13 +176,30 @@ static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { > static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, int enable) > { > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > + struct v4l2_subdev_state *state; > struct v4l2_subdev *src; > + int ret; > > src = adv748x_get_remote_sd(&tx->pads[ADV748X_CSI2_SINK]); > if (!src) > return -EPIPE; > > - return v4l2_subdev_call(src, video, s_stream, enable); > + state = v4l2_subdev_lock_and_get_active_state(sd); > + > + if (enable) { > + /* A single route is available. */ > + struct v4l2_subdev_route *route = &state->routing.routes[0]; > + > + ret = adv748x_csi2_set_virtual_channel(tx, route->source_stream); > + if (ret) > + goto unlock; > + } > + > + ret = v4l2_subdev_call(src, video, s_stream, enable); > +unlock: > + v4l2_subdev_unlock_state(state); > + > + return ret; > } > > static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > index be24bc57767c..95d04468af9d 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -434,7 +434,6 @@ int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input); > > int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx); > void adv748x_csi2_cleanup(struct adv748x_csi2 *tx); > -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc); > int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate); > > int adv748x_hdmi_init(struct adv748x_hdmi *hdmi);
On Thu, May 02, 2024 at 08:51:16PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Apr 30, 2024 at 12:39:42PM +0200, Jacopo Mondi wrote: > > Use the newly introduced routing table to configure on which MIPI > > CSI-2 Virtual Channel to send the image stream on. > > The stream ID in the routing API isn't meant to be mapped directly to a > virtual channel number. Additionally, why do you need to make the virtual channel configurable, instead of allocating them dynamically ? > Sakari, your opinion would be appreciated. > > > Perform Virtual Channel selection at s_stream() time instead of > > forcing it to 0 during the chip reset. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/i2c/adv748x/adv748x-core.c | 8 ++------ > > drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++-- > > drivers/media/i2c/adv748x/adv748x.h | 1 - > > 3 files changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > > index 3abc73ea8ccb..c9d917135709 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -530,14 +530,10 @@ static int adv748x_reset(struct adv748x_state *state) > > io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); > > > > /* Conditionally enable TXa and TXb. */ > > - if (is_tx_enabled(&state->txa)) { > > + if (is_tx_enabled(&state->txa)) > > regval |= ADV748X_IO_10_CSI4_EN; > > - adv748x_csi2_set_virtual_channel(&state->txa, 0); > > - } > > - if (is_tx_enabled(&state->txb)) { > > + if (is_tx_enabled(&state->txb)) > > regval |= ADV748X_IO_10_CSI1_EN; > > - adv748x_csi2_set_virtual_channel(&state->txb, 0); > > - } > > io_write(state, ADV748X_IO_10, regval); > > > > /* Use vid_std and v_freq as freerun resolution for CP */ > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > index 7fa72340e66e..a7bfed393ff0 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > @@ -14,7 +14,8 @@ > > > > #include "adv748x.h" > > > > -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc) > > +static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, > > + unsigned int vc) > > { > > return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT); > > } > > @@ -175,13 +176,30 @@ static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { > > static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > { > > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > + struct v4l2_subdev_state *state; > > struct v4l2_subdev *src; > > + int ret; > > > > src = adv748x_get_remote_sd(&tx->pads[ADV748X_CSI2_SINK]); > > if (!src) > > return -EPIPE; > > > > - return v4l2_subdev_call(src, video, s_stream, enable); > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > + > > + if (enable) { > > + /* A single route is available. */ > > + struct v4l2_subdev_route *route = &state->routing.routes[0]; > > + > > + ret = adv748x_csi2_set_virtual_channel(tx, route->source_stream); > > + if (ret) > > + goto unlock; > > + } > > + > > + ret = v4l2_subdev_call(src, video, s_stream, enable); > > +unlock: > > + v4l2_subdev_unlock_state(state); > > + > > + return ret; > > } > > > > static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > index be24bc57767c..95d04468af9d 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -434,7 +434,6 @@ int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input); > > > > int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx); > > void adv748x_csi2_cleanup(struct adv748x_csi2 *tx); > > -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc); > > int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate); > > > > int adv748x_hdmi_init(struct adv748x_hdmi *hdmi); > > -- > Regards, > > Laurent Pinchart
On Thu, May 02, 2024 at 08:52:34PM GMT, Laurent Pinchart wrote: > On Thu, May 02, 2024 at 08:51:16PM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Tue, Apr 30, 2024 at 12:39:42PM +0200, Jacopo Mondi wrote: > > > Use the newly introduced routing table to configure on which MIPI > > > CSI-2 Virtual Channel to send the image stream on. > > > > The stream ID in the routing API isn't meant to be mapped directly to a > > virtual channel number. > Isn't that up to the device to decide what a stream number represent ? > Additionally, why do you need to make the virtual channel configurable, > instead of allocating them dynamically ? What do you mean with "allocating them dinamically" ? Anyway, I guess the main question is: - How to control VC selection if the stream number is not the right way to do that - Has VC selection any value at all > > > Sakari, your opinion would be appreciated. > > > > > Perform Virtual Channel selection at s_stream() time instead of > > > forcing it to 0 during the chip reset. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > drivers/media/i2c/adv748x/adv748x-core.c | 8 ++------ > > > drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++-- > > > drivers/media/i2c/adv748x/adv748x.h | 1 - > > > 3 files changed, 22 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > > > index 3abc73ea8ccb..c9d917135709 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > > @@ -530,14 +530,10 @@ static int adv748x_reset(struct adv748x_state *state) > > > io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); > > > > > > /* Conditionally enable TXa and TXb. */ > > > - if (is_tx_enabled(&state->txa)) { > > > + if (is_tx_enabled(&state->txa)) > > > regval |= ADV748X_IO_10_CSI4_EN; > > > - adv748x_csi2_set_virtual_channel(&state->txa, 0); > > > - } > > > - if (is_tx_enabled(&state->txb)) { > > > + if (is_tx_enabled(&state->txb)) > > > regval |= ADV748X_IO_10_CSI1_EN; > > > - adv748x_csi2_set_virtual_channel(&state->txb, 0); > > > - } > > > io_write(state, ADV748X_IO_10, regval); > > > > > > /* Use vid_std and v_freq as freerun resolution for CP */ > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > index 7fa72340e66e..a7bfed393ff0 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > @@ -14,7 +14,8 @@ > > > > > > #include "adv748x.h" > > > > > > -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc) > > > +static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, > > > + unsigned int vc) > > > { > > > return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT); > > > } > > > @@ -175,13 +176,30 @@ static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { > > > static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > > { > > > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > > + struct v4l2_subdev_state *state; > > > struct v4l2_subdev *src; > > > + int ret; > > > > > > src = adv748x_get_remote_sd(&tx->pads[ADV748X_CSI2_SINK]); > > > if (!src) > > > return -EPIPE; > > > > > > - return v4l2_subdev_call(src, video, s_stream, enable); > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > + > > > + if (enable) { > > > + /* A single route is available. */ > > > + struct v4l2_subdev_route *route = &state->routing.routes[0]; > > > + > > > + ret = adv748x_csi2_set_virtual_channel(tx, route->source_stream); > > > + if (ret) > > > + goto unlock; > > > + } > > > + > > > + ret = v4l2_subdev_call(src, video, s_stream, enable); > > > +unlock: > > > + v4l2_subdev_unlock_state(state); > > > + > > > + return ret; > > > } > > > > > > static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { > > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > > index be24bc57767c..95d04468af9d 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x.h > > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > > @@ -434,7 +434,6 @@ int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input); > > > > > > int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx); > > > void adv748x_csi2_cleanup(struct adv748x_csi2 *tx); > > > -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc); > > > int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate); > > > > > > int adv748x_hdmi_init(struct adv748x_hdmi *hdmi); > > > > -- > > Regards, > > > > Laurent Pinchart > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c index 3abc73ea8ccb..c9d917135709 100644 --- a/drivers/media/i2c/adv748x/adv748x-core.c +++ b/drivers/media/i2c/adv748x/adv748x-core.c @@ -530,14 +530,10 @@ static int adv748x_reset(struct adv748x_state *state) io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); /* Conditionally enable TXa and TXb. */ - if (is_tx_enabled(&state->txa)) { + if (is_tx_enabled(&state->txa)) regval |= ADV748X_IO_10_CSI4_EN; - adv748x_csi2_set_virtual_channel(&state->txa, 0); - } - if (is_tx_enabled(&state->txb)) { + if (is_tx_enabled(&state->txb)) regval |= ADV748X_IO_10_CSI1_EN; - adv748x_csi2_set_virtual_channel(&state->txb, 0); - } io_write(state, ADV748X_IO_10, regval); /* Use vid_std and v_freq as freerun resolution for CP */ diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c index 7fa72340e66e..a7bfed393ff0 100644 --- a/drivers/media/i2c/adv748x/adv748x-csi2.c +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c @@ -14,7 +14,8 @@ #include "adv748x.h" -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc) +static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, + unsigned int vc) { return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT); } @@ -175,13 +176,30 @@ static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { static int adv748x_csi2_s_stream(struct v4l2_subdev *sd, int enable) { struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); + struct v4l2_subdev_state *state; struct v4l2_subdev *src; + int ret; src = adv748x_get_remote_sd(&tx->pads[ADV748X_CSI2_SINK]); if (!src) return -EPIPE; - return v4l2_subdev_call(src, video, s_stream, enable); + state = v4l2_subdev_lock_and_get_active_state(sd); + + if (enable) { + /* A single route is available. */ + struct v4l2_subdev_route *route = &state->routing.routes[0]; + + ret = adv748x_csi2_set_virtual_channel(tx, route->source_stream); + if (ret) + goto unlock; + } + + ret = v4l2_subdev_call(src, video, s_stream, enable); +unlock: + v4l2_subdev_unlock_state(state); + + return ret; } static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = { diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h index be24bc57767c..95d04468af9d 100644 --- a/drivers/media/i2c/adv748x/adv748x.h +++ b/drivers/media/i2c/adv748x/adv748x.h @@ -434,7 +434,6 @@ int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input); int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx); void adv748x_csi2_cleanup(struct adv748x_csi2 *tx); -int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc); int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate); int adv748x_hdmi_init(struct adv748x_hdmi *hdmi);
Use the newly introduced routing table to configure on which MIPI CSI-2 Virtual Channel to send the image stream on. Perform Virtual Channel selection at s_stream() time instead of forcing it to 0 during the chip reset. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- drivers/media/i2c/adv748x/adv748x-core.c | 8 ++------ drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++++++++++++++++++++-- drivers/media/i2c/adv748x/adv748x.h | 1 - 3 files changed, 22 insertions(+), 9 deletions(-)