Hi Laurent, Niklas
On Fri, May 03, 2024 at 12:56:47AM GMT, Laurent Pinchart wrote:
> 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 leftover from where I had to call rcsi2_get_active_lanes() from
rcsi2_calc_mbps() and had to move the former above the latter.
I forgt to re-sort the two function, sorry about that
>
> 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
