Hi Tomi
On Mon, Feb 27, 2023 at 10:35:23AM +0200, Tomi Valkeinen wrote:
> Hi Jacopo,
>
> On 24/02/2023 17:48, Jacopo Mondi wrote:
> > Hi Tomi
> >
> > On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> > > Add routing and stream_config support to CAL driver.
> > >
> > > Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> > > separate streams at the same time.
> > >
> > > Add 8 video device nodes, each representing a single dma-engine, and set
> > > the number of source pads on camerarx to 8. Each video node can be
> > > connected to any of the source pads on either of the camerarx instances
> > > using media links. Camerarx internal routing is used to route the
> > > incoming CSI-2 streams to one of the 8 source pads.
> > >
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
> > > drivers/media/platform/ti/cal/cal-video.c | 146 +++++++++---
> > > drivers/media/platform/ti/cal/cal.c | 65 ++++--
> > > drivers/media/platform/ti/cal/cal.h | 4 +-
> > > 4 files changed, 342 insertions(+), 106 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > index faafbd0e9240..49ae29065cd1 100644
> > > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
> > > {
> > > struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
> > > u32 num_lanes = mipi_csi2->num_data_lanes;
> > > - const struct cal_format_info *fmtinfo;
> > > struct v4l2_subdev_state *state;
> > > - struct v4l2_mbus_framefmt *fmt;
> > > u32 bpp;
> > > s64 freq;
> > >
> > > - state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > > + /*
> > > + * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > > + * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > > + *
> > > + * With multistream input there is no single pixel rate, and thus we
> > > + * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > > + * causes v4l2_get_link_freq() to return an error if it falls back to
> > > + * V4L2_CID_PIXEL_RATE.
> > > + */
> >
> > To recap a bit of our offline discussion:
> > - max9286 GMSL deserializer (as a comparison for a multiplexed
> > transmitter) use PIXEL_RATE to report the cumulative pixel rate of
> > enabled transmitters. This is because the R-Car CSI-2 receiver on
> > which use PIXEL_RATE to compute the link freq [1]
> >
> > - according to [2]
> > pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> >
> > from which:
> > link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> >
> > This works as long the reported pixel rate includes visible and
> > blankings, something I'm not sure how many transmitters handle
> > correctly as PIXEL_RATE control is meant to report the visible pixel
> > sampling rate on the pixel array.
> >
> > I guess we should go towards mandating LINK_FREQ for transmitters.
> >
> > cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()
> >
> > [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
> > [2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers
> >
> > >
> > > - fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
> > > + state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > >
> > > - fmtinfo = cal_format_by_code(fmt->code);
> > > - if (!fmtinfo)
> > > + if (state->routing.num_routes == 0)
> > > return -EINVAL;
> >
> > This function is in the call path of .enable_streams which if I'm not
> > reading the code wrong is called with
> >
> > int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
> > {
> >
> > ...
> >
> > for_each_active_route(&state->routing, route)
> > source_mask |= BIT_ULL(route->source_stream);
> >
> > if (enable)
> > return v4l2_subdev_enable_streams(sd, pad_index, source_mask);
> >
> > ...
> > }
> >
> > int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> > u64 streams_mask)
> > {
> > struct device *dev = sd->entity.graph_obj.mdev->dev;
> > struct v4l2_subdev_state *state;
> > u64 found_streams = 0;
> > unsigned int i;
> > int ret;
> >
> > /* A few basic sanity checks first. */
> > if (pad >= sd->entity.num_pads)
> > return -EINVAL;
> >
> > if (!streams_mask)
> > return 0;
> >
> > ...
> > }
> >
> > So the question is: can we get to enable_streams without enabled
> > routes (sorry I should have tested before asking it but I don't have a
> > multiplexed setup easily accessible) ?
>
> No, I don't think we can. Why do you ask?
Because if that's the case, you can drop
if (state->routing.num_routes == 0)
return -EINVAL;
>
> > >
> > > - bpp = fmtinfo->bpp;
> > > + if (state->routing.num_routes > 1) {
> > > + bpp = 0;
> > > + } else {
> > > + const struct cal_format_info *fmtinfo;
> > > + struct v4l2_subdev_route *route = &state->routing.routes[0];
> >
> > Nit: if I'm not mistaken along the driver reverse-xmas-tree (I
> > know...) is respected. Can you do it here as well ?
>
> Sure.
>
> > > + struct v4l2_mbus_framefmt *fmt;
> > > +
> > > + fmt = v4l2_subdev_state_get_stream_format(
> > > + state, route->sink_pad, route->sink_stream);
> > > +
> > > + fmtinfo = cal_format_by_code(fmt->code);
> > > + if (!fmtinfo)
> > > + return -EINVAL;
> > > +
> > > + bpp = fmtinfo->bpp;
> > > + }
> > >
> > > freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes);
> > > if (freq < 0) {
> > > @@ -284,15 +304,28 @@ static void cal_camerarx_ppi_disable(struct cal_camerarx *phy)
> > > 0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
> > > }
> > >
> > > -static int cal_camerarx_start(struct cal_camerarx *phy)
> > > +static int cal_camerarx_start(struct cal_camerarx *phy, u32 pad, u32 stream)
> > > {
> > > + struct media_pad *remote_pad;
> > > s64 link_freq;
> > > u32 sscounter;
> > > u32 val;
> > > int ret;
> > >
> > > + remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
> > > +
> >
> > Would it hurt a comment here to explain that in case it's the first to
> > be enabled you need to start the RX and power up the transmitter,
> > otherwise it's enough to enable the additional stream ?
>
> I'll add a comment.
>
> > > if (phy->enable_count > 0) {
> > > phy->enable_count++;
> > > +
> > > + ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
> > > + BIT(stream));
> > > + if (ret) {
> > > + phy->enable_count--;
> >
> > You can avoid this by enable_count++ after the error check ?
>
> Yep.
>
> > > +
> > > + phy_err(phy, "enable streams failed in source: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -394,7 +427,9 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
> > > * Start the source to enable the CSI-2 HS clock. We can now wait for
> > > * CSI-2 PHY reset to complete.
> > > */
> > > - ret = v4l2_subdev_call(phy->source, video, s_stream, 1);
> > > +
> >
> > Intentional additional blank line ?
>
> No.
>
> > > + ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
> > > + BIT(stream));
> > > if (ret) {
> > > v4l2_subdev_call(phy->source, core, s_power, 0);
> > > cal_camerarx_disable_irqs(phy);
> > > @@ -425,12 +460,22 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
> > > return 0;
> > > }
> > >
> > > -static void cal_camerarx_stop(struct cal_camerarx *phy)
> > > +static void cal_camerarx_stop(struct cal_camerarx *phy, u32 pad, u32 stream)
> > > {
> > > + struct media_pad *remote_pad;
> > > int ret;
> > >
> > > - if (--phy->enable_count > 0)
> > > + remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
> > > +
> > > + if (--phy->enable_count > 0) {
> > > + ret = v4l2_subdev_disable_streams(phy->source,
> > > + remote_pad->index,
> > > + BIT(stream));
> > > + if (ret)
> > > + phy_err(phy, "stream off failed in subdev\n");
> > > +
> > > return;
> > > + }
> > >
> > > cal_camerarx_ppi_disable(phy);
> > >
> > > @@ -450,7 +495,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy)
> > > /* Disable the phy */
> > > cal_camerarx_disable(phy);
> > >
> > > - if (v4l2_subdev_call(phy->source, video, s_stream, 0))
> > > + ret = v4l2_subdev_disable_streams(phy->source, remote_pad->index,
> > > + BIT(stream));
> > > + if (ret)
> > > phy_err(phy, "stream off failed in subdev\n");
> > >
> > > ret = v4l2_subdev_call(phy->source, core, s_power, 0);
> > > @@ -626,30 +673,62 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
> > > return container_of(sd, struct cal_camerarx, subdev);
> > > }
> > >
> > > -static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
> > > +struct cal_camerarx *
> > > +cal_camerarx_get_phy_from_entity(struct media_entity *entity)
> > > +{
> > > + struct v4l2_subdev *sd;
> > > +
> > > + sd = media_entity_to_v4l2_subdev(entity);
> > > + if (!sd)
> > > + return NULL;
> > > +
> > > + return to_cal_camerarx(sd);
> > > +}
> > > +
> > > +static int cal_camerarx_sd_enable_streams(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *state,
> > > + u32 pad, u64 streams_mask)
> > > {
> > > struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > - struct v4l2_subdev_state *state;
> > > - int ret = 0;
> > > + u32 other_pad, other_stream;
> > > + int ret;
> > >
> > > - state = v4l2_subdev_lock_and_get_active_state(sd);
> > > + if (WARN_ON(streams_mask != 1))
> >
> > as streams_mask is unsigned and you can't get here if streams_mask ==
> > 0, I wonder if checking for > 1 isn't more explicit. A detail though.
>
> Well, we need streams_mask to be exactly 1. I think the current test says
> that. If we'd check for > 1, the reader's thoughts might be that 0 and 1 are
> ok, which is not the case.
Yeah, you're probably right, it's easier to read it in the way you
have it
>
> Then again, I think the whole check can be dropped. We're calling it from
> inside the CAL driver, always with 1, so there's little chance it could be
> != 1.
>
That would be even better
> > > + return -EINVAL;
> > >
> > > - if (enable)
> > > - ret = cal_camerarx_start(phy);
> > > - else
> > > - cal_camerarx_stop(phy);
> > > + ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
> > > + &other_pad, &other_stream);
> > > + if (ret)
> > > + return ret;
> > >
> > > - v4l2_subdev_unlock_state(state);
> > > + return cal_camerarx_start(phy, other_pad, other_stream);
> > > +}
> > >
> > > - return ret;
> > > +static int cal_camerarx_sd_disable_streams(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *state,
> > > + u32 pad, u64 streams_mask)
> > > +{
> > > + struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > + u32 other_pad, other_stream;
> > > + int ret;
> > > +
> > > + if (WARN_ON(streams_mask != 1))
> > > + return -EINVAL;
> > > +
> > > + ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
> > > + &other_pad, &other_stream);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + cal_camerarx_stop(phy, other_pad, other_stream);
> > > +
> > > + return 0;
> > > }
> > >
> > > static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_state *state,
> > > struct v4l2_subdev_mbus_code_enum *code)
> > > {
> > > - struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > -
> > > /* No transcoding, source and sink codes must match. */
> > > if (cal_rx_pad_is_source(code->pad)) {
> > > struct v4l2_mbus_framefmt *fmt;
> > > @@ -657,8 +736,12 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
> > > if (code->index > 0)
> > > return -EINVAL;
> > >
> > > - fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
> > > - CAL_CAMERARX_PAD_SINK);
> > > + fmt = v4l2_subdev_state_get_opposite_stream_format(state,
> > > + code->pad,
> > > + code->stream);
> > > + if (!fmt)
> > > + return -EINVAL;
> > > +
> > > code->code = fmt->code;
> > > } else {
> > > if (code->index >= cal_num_formats)
> > > @@ -683,8 +766,12 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
> > > if (cal_rx_pad_is_source(fse->pad)) {
> > > struct v4l2_mbus_framefmt *fmt;
> > >
> > > - fmt = v4l2_subdev_get_pad_format(sd, state,
> > > - CAL_CAMERARX_PAD_SINK);
> > > + fmt = v4l2_subdev_state_get_opposite_stream_format(state,
> > > + fse->pad,
> > > + fse->stream);
> > > + if (!fmt)
> > > + return -EINVAL;
> > > +
> > > if (fse->code != fmt->code)
> > > return -EINVAL;
> > >
> > > @@ -740,57 +827,96 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
> > >
> > > /* Store the format and propagate it to the source pad. */
> > >
> > > - fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
> > > + fmt = v4l2_subdev_state_get_stream_format(state, format->pad,
> > > + format->stream);
> > > + if (!fmt)
> > > + return -EINVAL;
> > > +
> > > *fmt = format->format;
> > >
> > > - fmt = v4l2_subdev_get_pad_format(sd, state,
> > > - CAL_CAMERARX_PAD_FIRST_SOURCE);
> > > + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> > > + format->stream);
> > > + if (!fmt)
> > > + return -EINVAL;
> > > +
> > > *fmt = format->format;
> > >
> > > return 0;
> > > }
> > >
> > > +static int _cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *state,
> > > + struct v4l2_subdev_krouting *routing)
> >
> > Functions starting with _ are a bit unusual :)
>
> Hmm, well... I don't have any ideas for a good name =).
> cal_camerarx_sd_set_routing_impl() ?
Or just cal_camerarx_set_routing() with the _sd_ version being the
operation callback ? Too similar as a name and the risk is to confuse
readers maybe ?
>
> > > +{
> > > + static const struct v4l2_mbus_framefmt format = {
> > > + .width = 640,
> > > + .height = 480,
> > > + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > + .field = V4L2_FIELD_NONE,
> > > + .colorspace = V4L2_COLORSPACE_SRGB,
> > > + .ycbcr_enc = V4L2_YCBCR_ENC_601,
> > > + .quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > > + .xfer_func = V4L2_XFER_FUNC_SRGB,
> > > + };
> > > + int ret;
> > > +
> > > + ret = v4l2_subdev_routing_validate(sd, routing, V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_state *state,
> > > + enum v4l2_subdev_format_whence which,
> > > + struct v4l2_subdev_krouting *routing)
> > > +{
> > > + return _cal_camerarx_sd_set_routing(sd, state, routing);
> > > +}
> > > +
> > > static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_state *state)
> > > {
> > > - struct v4l2_subdev_format format = {
> > > - .which = state ? V4L2_SUBDEV_FORMAT_TRY
> > > - : V4L2_SUBDEV_FORMAT_ACTIVE,
> > > - .pad = CAL_CAMERARX_PAD_SINK,
> > > - .format = {
> > > - .width = 640,
> > > - .height = 480,
> > > - .code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > - .field = V4L2_FIELD_NONE,
> > > - .colorspace = V4L2_COLORSPACE_SRGB,
> > > - .ycbcr_enc = V4L2_YCBCR_ENC_601,
> > > - .quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > > - .xfer_func = V4L2_XFER_FUNC_SRGB,
> > > - },
> > > + struct v4l2_subdev_route routes[] = { {
> > > + .sink_pad = 0,
> > > + .sink_stream = 0,
> > > + .source_pad = 1,
> > > + .source_stream = 0,
> > > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > > + } };
> > > +
> > > + struct v4l2_subdev_krouting routing = {
> > > + .num_routes = 1,
> > > + .routes = routes,
> > > };
> > >
> > > - return cal_camerarx_sd_set_fmt(sd, state, &format);
> > > + /* Initialize routing to single route to the fist source pad */
> > > + return _cal_camerarx_sd_set_routing(sd, state, &routing);
> > > }
> > >
> > > -static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {
> > > - .s_stream = cal_camerarx_sd_s_stream,
> > > -};
> > > -
> > > static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
> > > + .enable_streams = cal_camerarx_sd_enable_streams,
> > > + .disable_streams = cal_camerarx_sd_disable_streams,
> > > .init_cfg = cal_camerarx_sd_init_cfg,
> > > .enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
> > > .enum_frame_size = cal_camerarx_sd_enum_frame_size,
> > > .get_fmt = v4l2_subdev_get_fmt,
> > > .set_fmt = cal_camerarx_sd_set_fmt,
> > > + .set_routing = cal_camerarx_sd_set_routing,
> > > };
> > >
> > > static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = {
> > > - .video = &cal_camerarx_video_ops,
> > > .pad = &cal_camerarx_pad_ops,
> > > };
> > >
> > > static struct media_entity_operations cal_camerarx_media_ops = {
> > > .link_validate = v4l2_subdev_link_validate,
> > > + .has_pad_interdep = v4l2_subdev_has_pad_interdep,
> > > };
> > >
> > > /* ------------------------------------------------------------------
> > > @@ -842,11 +968,12 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> > > sd = &phy->subdev;
> > > v4l2_subdev_init(sd, &cal_camerarx_subdev_ops);
> > > sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> > > - sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
> > > snprintf(sd->name, sizeof(sd->name), "CAMERARX%u", instance);
> > > sd->dev = cal->dev;
> > >
> > > phy->pads[CAL_CAMERARX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > > +
> >
> > Intentional ?
> >
> > > for (i = CAL_CAMERARX_PAD_FIRST_SOURCE; i < CAL_CAMERARX_NUM_PADS; ++i)
> > > phy->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > > sd->entity.ops = &cal_camerarx_media_ops;
> > > @@ -879,7 +1006,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
> > > return;
> > >
> > > v4l2_device_unregister_subdev(&phy->subdev);
> > > +
> > > v4l2_subdev_cleanup(&phy->subdev);
> > > +
> >
> > ditto
>
> I can drop these.
As you wish, I was just making sure they were intentional
>
> > > media_entity_cleanup(&phy->subdev.entity);
> > > of_node_put(phy->source_ep_node);
> > > of_node_put(phy->source_node);
> > > diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> > > index d363e123d4bb..71578bfc97ba 100644
> > > --- a/drivers/media/platform/ti/cal/cal-video.c
> > > +++ b/drivers/media/platform/ti/cal/cal-video.c
> > > @@ -119,12 +119,13 @@ static int __subdev_get_format(struct cal_ctx *ctx,
> > > {
> > > struct v4l2_subdev_format sd_fmt;
> > > struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
> > > + struct v4l2_subdev *sd = ctx->phy->source;
> > > int ret;
> > >
> > > sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > sd_fmt.pad = 0;
> > >
> > > - ret = v4l2_subdev_call(ctx->phy->source, pad, get_fmt, NULL, &sd_fmt);
> > > + ret = v4l2_subdev_call_state_active(sd, pad, get_fmt, &sd_fmt);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -141,13 +142,14 @@ static int __subdev_set_format(struct cal_ctx *ctx,
> > > {
> > > struct v4l2_subdev_format sd_fmt;
> > > struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
> > > + struct v4l2_subdev *sd = ctx->phy->source;
> > > int ret;
> > >
> > > sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > sd_fmt.pad = 0;
> > > *mbus_fmt = *fmt;
> > >
> > > - ret = v4l2_subdev_call(ctx->phy->source, pad, set_fmt, NULL, &sd_fmt);
> > > + ret = v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -189,6 +191,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
> > > struct v4l2_format *f)
> > > {
> > > struct cal_ctx *ctx = video_drvdata(file);
> > > + struct v4l2_subdev *sd = ctx->phy->source;
> > > const struct cal_format_info *fmtinfo;
> > > struct v4l2_subdev_frame_size_enum fse;
> > > int found;
> > > @@ -213,8 +216,8 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
> > > for (fse.index = 0; ; fse.index++) {
> > > int ret;
> > >
> > > - ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
> > > - NULL, &fse);
> > > + ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size,
> > > + &fse);
> > > if (ret)
> > > break;
> > >
> > > @@ -250,6 +253,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
> > > struct v4l2_format *f)
> > > {
> > > struct cal_ctx *ctx = video_drvdata(file);
> > > + struct v4l2_subdev *sd = &ctx->phy->subdev;
> > > struct vb2_queue *q = &ctx->vb_vidq;
> > > struct v4l2_subdev_format sd_fmt = {
> > > .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > @@ -289,7 +293,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
> > > ctx->v_fmt.fmt.pix.field = sd_fmt.format.field;
> > > cal_calc_format_size(ctx, fmtinfo, &ctx->v_fmt);
> > >
> > > - v4l2_subdev_call(&ctx->phy->subdev, pad, set_fmt, NULL, &sd_fmt);
> > > + v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
> > >
> > > ctx->fmtinfo = fmtinfo;
> > > *f = ctx->v_fmt;
> > > @@ -301,6 +305,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
> > > struct v4l2_frmsizeenum *fsize)
> > > {
> > > struct cal_ctx *ctx = video_drvdata(file);
> > > + struct v4l2_subdev *sd = ctx->phy->source;
> > > const struct cal_format_info *fmtinfo;
> > > struct v4l2_subdev_frame_size_enum fse;
> > > int ret;
> > > @@ -318,8 +323,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
> > > fse.code = fmtinfo->code;
> > > fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > - ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, NULL,
> > > - &fse);
> > > + ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size, &fse);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -361,6 +365,7 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
> > > struct v4l2_frmivalenum *fival)
> > > {
> > > struct cal_ctx *ctx = video_drvdata(file);
> > > + struct v4l2_subdev *sd = ctx->phy->source;
> > > const struct cal_format_info *fmtinfo;
> > > struct v4l2_subdev_frame_interval_enum fie = {
> > > .index = fival->index,
> > > @@ -375,8 +380,8 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
> > > return -EINVAL;
> > >
> > > fie.code = fmtinfo->code;
> > > - ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_interval,
> > > - NULL, &fie);
> > > +
> > > + ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_interval, &fie);
> > > if (ret)
> > > return ret;
> > > fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > > @@ -694,8 +699,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
> > > {
> > > const struct cal_format_info *rx_fmtinfo;
> > > const struct v4l2_mbus_framefmt *format;
> > > - struct v4l2_subdev_state *state;
> > > struct media_pad *remote_pad;
> > > + struct v4l2_subdev_state *state;
> > > int ret = 0;
> > >
> > > remote_pad = media_pad_remote_pad_first(&ctx->pad);
> > > @@ -704,7 +709,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
> > >
> > > state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
> > >
> > > - format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
> > > + format = v4l2_subdev_state_get_stream_format(state, remote_pad->index,
> > > + 0);
> > > if (!format) {
> > > ret = -EINVAL;
> > > goto out;
> > > @@ -733,10 +739,52 @@ static int cal_video_check_format(struct cal_ctx *ctx)
> > > static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> > > {
> > > struct cal_ctx *ctx = vb2_get_drv_priv(vq);
> > > + struct media_pad *remote_pad;
> > > struct cal_buffer *buf;
> > > dma_addr_t addr;
> > > int ret;
> > >
> > > + remote_pad = media_pad_remote_pad_first(&ctx->pad);
> > > + if (!remote_pad) {
> > > + ctx_err(ctx, "Context not connected\n");
> > > + ret = -ENODEV;
> > > + goto error_release_buffers;
> > > + }
> > > +
> > > + if (cal_mc_api) {
> > > + struct v4l2_subdev_route *route = NULL;
> > > + struct v4l2_subdev_route *r;
> > > + struct v4l2_subdev_state *state;
> > > +
> > > + /* Find the PHY connected to this video device */
> > > +
> > > + ctx->phy = cal_camerarx_get_phy_from_entity(remote_pad->entity);
> > > +
> > > + state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
> > > +
> > > + /* Find the stream */
> > > +
> > > + for_each_active_route(&state->routing, r) {
> > > + if (r->source_pad != remote_pad->index)
> > > + continue;
> > > +
> > > + route = r;
> > > +
> > > + break;
> > > + }
> > > +
> > > + if (!route) {
> > > + v4l2_subdev_unlock_state(state);
> > > + ctx_err(ctx, "Failed to find route\n");
> > > + ret = -ENODEV;
> > > + goto error_release_buffers;
> > > + }
> >
> > If I got it right: we inspect the PHY routing table, pick the route that ends in
> > the pad connected to the this video device and store the sink-stream
> > id. The sink-stream id is used for... digging into the frame_desc
> > handling I have a slightly unrelated question if the PHY shouldn't
> > implement .get_frame_desc() instead of filtering what is returned from
> > the deser's implementation of .get_frame_desc() which is called by
> > cal_camerarx_get_remote_frame_desc(). If I'm not mitaken in that case
> > you could just call the PHY's .get_frame_desc() instead of inspecting its
> > routing table here. Does it make sense..
>
> Indeed, this looks a bit odd. This patch has evolved along the stream
> series, so it has gone though almost total rewrites a few times. And there
> seem to be some odd designs like this.
>
> I think adding .get_frame_desc() to the camerarx makes sense.
I presume it will help if camerarx will ever be used without CAL (if
that's even possible..)
Thanks!
>
> > > +
> > > + ctx->stream = route->sink_stream;
> > > +
> > > + v4l2_subdev_unlock_state(state);
> > > + }
> > > +
> > > ret = video_device_pipeline_alloc_start(&ctx->vdev);
> > > if (ret < 0) {
> > > ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
> > > @@ -775,7 +823,8 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> > > cal_ctx_set_dma_addr(ctx, addr);
> > > cal_ctx_start(ctx);
> > >
> > > - ret = v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 1);
> > > + ret = v4l2_subdev_enable_streams(&ctx->phy->subdev, remote_pad->index,
> > > + BIT(0));
> > > if (ret)
> > > goto error_stop;
> > >
> > > @@ -800,10 +849,14 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> > > static void cal_stop_streaming(struct vb2_queue *vq)
> > > {
> > > struct cal_ctx *ctx = vb2_get_drv_priv(vq);
> > > + struct media_pad *remote_pad;
> > >
> > > cal_ctx_stop(ctx);
> > >
> > > - v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 0);
> > > + remote_pad = media_pad_remote_pad_first(&ctx->pad);
> > > +
> > > + v4l2_subdev_disable_streams(&ctx->phy->subdev, remote_pad->index,
> > > + BIT(0));
> > >
> > > pm_runtime_put_sync(ctx->cal->dev);
> > >
> > > @@ -812,6 +865,9 @@ static void cal_stop_streaming(struct vb2_queue *vq)
> > > cal_release_buffers(ctx, VB2_BUF_STATE_ERROR);
> > >
> > > video_device_pipeline_stop(&ctx->vdev);
> > > +
> > > + if (cal_mc_api)
> > > + ctx->phy = NULL;
> > > }
> > >
> > > static const struct vb2_ops cal_video_qops = {
> > > @@ -845,6 +901,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
> > > const struct cal_format_info *fmtinfo;
> > > unsigned int i, j, k;
> > > int ret = 0;
> > > + struct v4l2_subdev *sd = ctx->phy->source;
> >
> > Move it up ? :P
>
> Sure.
>
> > The rest (for my undestanding) seems right. By dropping the !mc
> > support things would look much simpler :)
>
> Yes, I'd love to drop !mc =).
>
> Thanks!
>
> Tomi
>
