On Thu, May 02, 2024 at 04:30:54PM +0200, Niklas Söderlund wrote: > On 2024-04-30 12:39:52 +0200, Jacopo Mondi wrote: > > Rework the R-Car CSI-2 start routine to support multiplexed > > transmitters. > > > > Configure the CSI-2 receiver MIPI CSI-2 Data Type filtering by inspecting > > the remote subdev frame_desc instead of relying on the image format > > configured on the sink pad. > > > > Enable MIPI CSI-2 Data Type filtering by inspecting the remote subdevice > > frame descriptor to discern which Data Type is transmitted on which > > Virtual Channel. If multiple Data Types are transmitted on the same VC > > then Data Type filtering is disabled. > > > > Rework the per-lane bandwidth calculation to use the LINK_FREQ control > > if a transmitter sends multiple streams on the same data link. The > > current usage of the PIXEL_RATE control does not support multiplexed > > transmitters, as there's not a unique pixel rate among all the possible > > source streams. It sounds like this should be split in two patches, the LINK_FREQ change seems separate from the DT/VC filtering and routing handling. > > > > This change makes mandatory for any subdevice that operates with > > the R-Car CSI-2 receiver to implement the .get_frame_desc() operation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > > --- > > drivers/media/platform/renesas/rcar-csi2.c | 280 ++++++++++++++------- > > 1 file changed, 191 insertions(+), 89 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c > > index 82dc0b92b8d3..ffb73272543b 100644 > > --- a/drivers/media/platform/renesas/rcar-csi2.c > > +++ b/drivers/media/platform/renesas/rcar-csi2.c > > @@ -69,10 +69,7 @@ struct rcar_csi2; > > #define FLD_REG 0x1c > > #define FLD_FLD_NUM(n) (((n) & 0xff) << 16) > > #define FLD_DET_SEL(n) (((n) & 0x3) << 4) > > -#define FLD_FLD_EN4 BIT(3) > > -#define FLD_FLD_EN3 BIT(2) > > -#define FLD_FLD_EN2 BIT(1) > > -#define FLD_FLD_EN BIT(0) > > +#define FLD_FLD_EN(n) BIT((n) & 0xf) > > > > /* Automatic Standby Control */ > > #define ASTBY_REG 0x20 > > @@ -575,6 +572,16 @@ static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code) > > return NULL; > > } > > > > +static const struct rcar_csi2_format *rcsi2_datatype_to_fmt(unsigned int dt) > > +{ > > + for (unsigned int i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) { > > + if (rcar_csi2_formats[i].datatype == dt) > > + return &rcar_csi2_formats[i]; > > + } > > + > > + return NULL; > > +} > > + > > enum rcar_csi2_pads { > > RCAR_CSI2_SINK, > > RCAR_CSI2_SOURCE_VC0, > > @@ -587,7 +594,8 @@ enum rcar_csi2_pads { > > struct rcar_csi2_info { > > int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps); > > int (*phy_post_init)(struct rcar_csi2 *priv); > > - int (*start_receiver)(struct rcar_csi2 *priv); > > + int (*start_receiver)(struct rcar_csi2 *priv, > > + struct v4l2_subdev_state *state); > > void (*enter_standby)(struct rcar_csi2 *priv); > > const struct rcsi2_mbps_reg *hsfreqrange; > > unsigned int csi0clkfreqrange; > > @@ -676,6 +684,32 @@ static int rcsi2_exit_standby(struct rcar_csi2 *priv) > > return 0; > > } > > > > +static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv, > > + struct v4l2_mbus_frame_desc *fd) > > +{ > > + struct media_pad *pad; > > + int ret; > > + > > + if (!priv->remote) > > + return -ENOLINK; > > + > > + pad = media_pad_remote_pad_unique(&priv->pads[RCAR_CSI2_SINK]); > > + if (IS_ERR(pad)) > > + return PTR_ERR(pad); > > + > > + ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc, > > + pad->index, fd); > > + if (ret) > > + return ret; > > + > > + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > > + dev_err(priv->dev, "Frame desc does not describe a CSI-2 link"); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > static int rcsi2_wait_phy_start(struct rcar_csi2 *priv, > > unsigned int lanes) > > { > > @@ -726,41 +760,6 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps) > > return 0; > > } > > > > -static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, > > - unsigned int lanes) > > -{ > > - struct v4l2_subdev *source; > > - struct v4l2_ctrl *ctrl; > > - u64 mbps; > > - > > - if (!priv->remote) > > - return -ENODEV; > > - > > - source = priv->remote; > > - > > - /* Read the pixel rate control from remote. */ > > - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > - if (!ctrl) { > > - dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > - source->name); > > - return -EINVAL; > > - } > > - > > - /* > > - * Calculate the phypll in mbps. > > - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > - * bps = link_freq * 2 > > - */ > > - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > - do_div(mbps, lanes * 1000000); > > - > > - /* Adjust for C-PHY, divide by 2.8. */ > > - if (priv->cphy) > > - mbps = div_u64(mbps * 5, 14); > > - > > - return mbps; > > -} > > - > > static int rcsi2_get_active_lanes(struct rcar_csi2 *priv, > > unsigned int *lanes) > > { > > @@ -808,52 +807,146 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv, > > return 0; > > } > > > > -static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv) > > +static int rcsi2_calc_mbps(struct rcar_csi2 *priv, > > + struct v4l2_mbus_frame_desc *fd, unsigned int lanes) > > { > > Is there a reason you need to move the rcsi2_calc_mbps()? No problem if > so, else if you keep it where it is the diff is easier to read ;-) It's a good practice to move functions around without changing them in a separate patch first, it makes review easier. > > - const struct rcar_csi2_format *format; > > + struct v4l2_subdev *source; > > + unsigned int bpp; > > + s64 link_freq; > > + u64 mbps; > > + > > + if (!priv->remote) > > + return -ENODEV; > > + > > + source = priv->remote; > > + > > + /* > > + * 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. > > + */ > > + > > + if (fd->num_entries > 1) { > > + bpp = 0; > > + } else { > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[0]; > > + const struct rcar_csi2_format *format; > > + > > + format = rcsi2_datatype_to_fmt(entry->bus.csi2.dt); > > + if (WARN_ON(!format)) > > + return -EINVAL; > > + > > + bpp = format->bpp; > > + } > > + > > + /* > > + * Calculate the phypll in mbps. > > + * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > + * bps = link_freq * 2 > > + */ > > + link_freq = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); > > + if (link_freq < 0) { > > + dev_err(priv->dev, "Failed to get remote subdev link freq\n"); > > + return link_freq; > > + } > > I just want to make sure I understand our discussion about on IRC about > this. It's this call you aim to replace by getting the link frequency > from the frame descriptor once it's available there? If so I think it > would be good to make this series depend on that work already and list > it as a dependency. > > > + > > + mbps = 2 * link_freq; > > + do_div(mbps, 1000000); > > + > > + /* Adjust for C-PHY, divide by 2.8. */ > > + if (priv->cphy) > > + mbps = div_u64(mbps * 5, 14); > > + > > + return mbps; > > +} > > + > > +static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv, > > + struct v4l2_subdev_state *state) > > +{ > > + const struct v4l2_subdev_stream_configs *configs; > > u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0; > > + struct v4l2_mbus_frame_desc fd; > > unsigned int lanes; > > - unsigned int i; > > int mbps, ret; > > > > - dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > - priv->mf.width, priv->mf.height, > > - priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > - > > - /* Code is validated in set_fmt. */ > > - format = rcsi2_code_to_fmt(priv->mf.code); > > - if (!format) > > - return -EINVAL; > > + /* Get information about multiplexed link. */ > > + ret = rcsi2_get_remote_frame_desc(priv, &fd); > > + if (ret) > > + return ret; > > > > /* > > - * Enable all supported CSI-2 channels with virtual channel and > > - * data type matching. > > + * Configure and enable the R-Car CSI-2 channels. > > * > > - * NOTE: It's not possible to get individual datatype for each > > - * source virtual channel. Once this is possible in V4L2 > > - * it should be used here. > > + * The VC transmitted on the channel is configured by the [CSI-2->VIN] > > + * link_setup operation, while the data type to match comes from the > > + * remote subdevice. > > */ > > - for (i = 0; i < priv->info->num_channels; i++) { > > - u32 vcdt_part; > > + for (unsigned int i = 0; i < priv->info->num_channels; i++) { > > + struct v4l2_mbus_frame_desc_entry *entry = NULL; > > > > + /* CSI-2 channel disabled (not linked to any VIN). */ > > if (priv->channel_vc[i] < 0) > > continue; > > > > - vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN | > > - VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype); > > + u32 vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | > > + VCDT_VCDTN_EN | VCDT_SEL_DTN_ON; > > + > > + /* > > + * Search the entries that describe the data types on the > > + * MIPI CSI-2 Virtual Channel assigned to this CSI-2 channel. > > + */ > > + for (unsigned int e = 0; e < fd.num_entries; e++) { > > + if (fd.entry[e].bus.csi2.vc != priv->channel_vc[i]) > > + continue; > > + > > + /* > > + * If multiple data types are sent on the same MIPI > > + * CSI-2 Virtual Channel, disable Data Type matching. > > + */ > > + if (entry) { > > + vcdt_part &= ~VCDT_SEL_DTN_ON; > > + break; > > + } > > + > > + entry = &fd.entry[e]; > > + vcdt_part |= VCDT_SEL_DT(entry->bus.csi2.dt); > > + } > > + > > + if (!entry) > > + continue; > > > > /* Store in correct reg and offset. */ > > if (i < 2) > > vcdt |= vcdt_part << ((i % 2) * 16); > > else > > vcdt2 |= vcdt_part << ((i % 2) * 16); > > + > > + dev_dbg(priv->dev, "channel %u: VC = %d, datatype = 0x%x\n", > > + i, priv->channel_vc[i], entry->bus.csi2.dt); > > } > > > > - if (priv->mf.field == V4L2_FIELD_ALTERNATE) { > > - fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 > > - | FLD_FLD_EN; > > + /* > > + * Configure field handling inspecting the formats of the > > + * sink pad streams. > > + */ > > + configs = &state->stream_configs; > > + for (unsigned int i = 0; i < configs->num_configs; ++i) { > > + const struct v4l2_subdev_stream_config *config = configs->configs; > > + > > + if (config->pad != RCAR_CSI2_SINK) > > + continue; > > > > - if (priv->mf.height == 240) > > + if (config->fmt.field != V4L2_FIELD_ALTERNATE) > > + continue; > > + > > + fld |= FLD_DET_SEL(1) | FLD_FLD_EN(config->stream); The stream ID is set by the source, and could be anything. You can't assume it's a virtual channel number. > > + > > + /* PAL vs NTSC. */ > > + if (config->fmt.height == 240) > > fld |= FLD_FLD_NUM(0); > > else > > fld |= FLD_FLD_NUM(1); > > @@ -870,7 +963,7 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv) > > phycnt = PHYCNT_ENABLECLK; > > phycnt |= (1 << lanes) - 1; > > > > - mbps = rcsi2_calc_mbps(priv, format->bpp, lanes); > > + mbps = rcsi2_calc_mbps(priv, &fd, lanes); > > if (mbps < 0) > > return mbps; > > > > @@ -1049,23 +1142,24 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps) > > return 0; > > } > > > > -static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv) > > +static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv, > > + struct v4l2_subdev_state *state) > > { > > - const struct rcar_csi2_format *format; > > + struct v4l2_mbus_frame_desc fd; > > unsigned int lanes; > > int msps; > > int ret; > > > > /* Calculate parameters */ > > - format = rcsi2_code_to_fmt(priv->mf.code); > > - if (!format) > > - return -EINVAL; > > - > > ret = rcsi2_get_active_lanes(priv, &lanes); > > if (ret) > > return ret; > > > > - msps = rcsi2_calc_mbps(priv, format->bpp, lanes); > > + ret = rcsi2_get_remote_frame_desc(priv, &fd); > > + if (ret) > > + return ret; > > + > > + msps = rcsi2_calc_mbps(priv, &fd, lanes); > > if (msps < 0) > > return msps; > > > > @@ -1114,7 +1208,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv) > > return 0; > > } > > > > -static int rcsi2_start(struct rcar_csi2 *priv) > > +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state) > > { > > int ret; > > > > @@ -1122,7 +1216,7 @@ static int rcsi2_start(struct rcar_csi2 *priv) > > if (ret < 0) > > return ret; > > > > - ret = priv->info->start_receiver(priv); > > + ret = priv->info->start_receiver(priv, state); > > if (ret) { > > rcsi2_enter_standby(priv); > > return ret; > > @@ -1146,26 +1240,24 @@ static void rcsi2_stop(struct rcar_csi2 *priv) > > static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable) > > { > > struct rcar_csi2 *priv = sd_to_csi2(sd); > > + struct v4l2_subdev_state *state; > > int ret = 0; > > > > - mutex_lock(&priv->lock); > > + if (!priv->remote) > > + return -ENODEV; > > > > - if (!priv->remote) { > > - ret = -ENODEV; > > - goto out; > > - } > > + state = v4l2_subdev_lock_and_get_active_state(&priv->subdev); > > > > - if (enable && priv->stream_count == 0) { > > - ret = rcsi2_start(priv); > > - if (ret) > > - goto out; > > - } else if (!enable && priv->stream_count == 1) { > > + if (enable && priv->stream_count == 0) > > + ret = rcsi2_start(priv, state); > > + else if (!enable && priv->stream_count == 1) > > rcsi2_stop(priv); > > - } > > + if (ret) > > + goto out; > > > > priv->stream_count += enable ? 1 : -1; > > out: > > - mutex_unlock(&priv->lock); > > + v4l2_subdev_unlock_state(state); > > > > return ret; > > } > > @@ -1310,14 +1402,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data) > > > > static irqreturn_t rcsi2_irq_thread(int irq, void *data) > > { > > + struct v4l2_subdev_state *state; > > struct rcar_csi2 *priv = data; > > > > - mutex_lock(&priv->lock); > > + state = v4l2_subdev_lock_and_get_active_state(&priv->subdev); > > + > > rcsi2_stop(priv); > > usleep_range(1000, 2000); > > - if (rcsi2_start(priv)) > > + if (rcsi2_start(priv, state)) > > dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n"); > > - mutex_unlock(&priv->lock); > > + > > + v4l2_subdev_unlock_state(state); > > > > return IRQ_HANDLED; > > } > > @@ -1340,6 +1435,13 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, > > return pad; > > } > > > > + if (!v4l2_subdev_has_op(subdev, pad, get_frame_desc)) { > > + dev_err(priv->dev, > > + "Subdev %s bound failed: missing get_frame_desc()\n", > > + subdev->name); > > + return -EINVAL; > > + } > > + > > priv->remote = subdev; > > priv->remote_pad = pad; > > -- Regards, Laurent Pinchart