Hi Sakari,
Thanks for your comments.
On 2017-12-15 15:19:36 +0200, Sakari Ailus wrote:
> Hi Niklas,
>
> On Thu, Dec 14, 2017 at 08:08:26PM +0100, Niklas Söderlund wrote:
> > Use the frame description from the remote subdevice of the rcar-csi2's
> > sink pad to get the remote pad and stream pad needed to propagate the
> > .s_stream() operation.
> >
> > The CSI-2 virtual channel which should be acted upon can be determined
> > by looking at which of the rcar-csi2 source pad the .s_stream() was
> > called on. This is because the rcar-csi2 acts as a demultiplexer for the
> > CSI-2 link on the one sink pad and outputs each virtual channel on a
> > distinct and known source pad.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 58 ++++++++++++++++++++---------
> > 1 file changed, 41 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index e0f56cc3d25179a9..6b607b2e31e26063 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -614,20 +614,31 @@ static void rcar_csi2_stop(struct rcar_csi2 *priv)
> > rcar_csi2_reset(priv);
> > }
> >
> > -static int rcar_csi2_sd_info(struct rcar_csi2 *priv, struct v4l2_subdev **sd)
> > +static int rcar_csi2_get_source_info(struct rcar_csi2 *priv,
> > + struct v4l2_subdev **subdev,
>
> I wonder if using struct media_pad for this would be cleaner.
I can give it a try and see how it works out, thanks for the suggestion.
>
> > + unsigned int *pad,
> > + struct v4l2_mbus_frame_desc *fd)
> > {
> > - struct media_pad *pad;
> > + struct media_pad *remote_pad;
> >
> > - pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> > - if (!pad) {
> > - dev_err(priv->dev, "Could not find remote pad\n");
> > + /* Get source subdevice and pad */
> > + remote_pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> > + if (!remote_pad) {
> > + dev_err(priv->dev, "Could not find remote source pad\n");
> > return -ENODEV;
> > }
> > + *subdev = media_entity_to_v4l2_subdev(remote_pad->entity);
> > + *pad = remote_pad->index;
> >
> > - *sd = media_entity_to_v4l2_subdev(pad->entity);
> > - if (!*sd) {
> > - dev_err(priv->dev, "Could not find remote subdevice\n");
> > - return -ENODEV;
> > + /* Get frame descriptor */
> > + if (v4l2_subdev_call(*subdev, pad, get_frame_desc, *pad, fd)) {
> > + dev_err(priv->dev, "Could not read frame desc\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > + dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
> > + return -EINVAL;
>
> I think this should also work with drivers that do not support frame
> descriptors.
>
> Alternatively support could be added for all drivers. In practice this
> would mean having a few bus specific implementations of get_frame_desc op
> that would dig the information from the frame format.
>
> Perhaps the former option would make sense here, for now.
I will try to give it some thought when I rework this series. At the
moment I'm not sure what is the best idea :-)
>
> > }
> >
> > return 0;
> > @@ -637,9 +648,10 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> > unsigned int stream, int enable)
> > {
> > struct rcar_csi2 *priv = sd_to_csi2(sd);
> > + struct v4l2_mbus_frame_desc fd;
> > struct v4l2_subdev *nextsd;
> > - unsigned int i, count = 0;
> > - int ret, vc;
> > + unsigned int i, rpad, count = 0;
> > + int ret, vc, rstream = -1;
> >
> > /* Only allow stream control on source pads and valid vc */
> > vc = rcar_csi2_pad_to_vc(pad);
> > @@ -650,11 +662,23 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> > if (stream != 0)
> > return -EINVAL;
> >
> > - mutex_lock(&priv->lock);
> > -
> > - ret = rcar_csi2_sd_info(priv, &nextsd);
> > + /* Get information about multiplexed link */
> > + ret = rcar_csi2_get_source_info(priv, &nextsd, &rpad, &fd);
> > if (ret)
> > - goto out;
> > + return ret;
> > +
> > + /* Get stream on multiplexed link */
> > + for (i = 0; i < fd.num_entries; i++)
> > + if (fd.entry[i].bus.csi2.channel == vc)
> > + rstream = fd.entry[i].stream;
>
> Virtual channel does not equate to the stream. You'll need the data type,
> too.
>
> You should actually obtain this from the configured routes instead.
You are correct, will fix.
>
> How does this work btw. if you have several streams on the same virtual
> channel that only have different data types?
I have not been able to test this but I think the hardware should be
able to handle it. From other part of this driver:
tmp = VCDT_SEL_VC(entry->bus.csi2.channel) | VCDT_VCDTN_EN |
VCDT_SEL_DTN_ON |
VCDT_SEL_DT(entry->bus.csi2.data_type);
So it looks like selection is based on both virtual channel and data
type. Unfortunately I'm not aware of hardware setup I can use to verify
this for the R-Car CSI-2. Maybe I can create a data type mismatch in the
driver and verify that no stream is outputted, but I would not say that
would prove it would work with to streams with same VC but different
data types.
>
> > +
> > + if (rstream < 0) {
> > + dev_err(priv->dev, "Could not find stream for vc %u\n", vc);
> > + return -EINVAL;
> > + }
> > +
> > + /* Start or stop the requested stream */
> > + mutex_lock(&priv->lock);
> >
> > for (i = 0; i < 4; i++)
> > count += priv->stream_count[i];
> > @@ -673,14 +697,14 @@ static int rcar_csi2_s_stream(struct v4l2_subdev *sd, unsigned int pad,
> > }
> >
> > if (enable && priv->stream_count[vc] == 0) {
> > - ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> > + ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 1);
> > if (ret) {
> > rcar_csi2_stop(priv);
> > pm_runtime_put(priv->dev);
> > goto out;
> > }
> > } else if (!enable && priv->stream_count[vc] == 1) {
> > - ret = v4l2_subdev_call(nextsd, video, s_stream, 0);
> > + ret = v4l2_subdev_call(nextsd, pad, s_stream, rpad, rstream, 0);
> > }
> >
> > priv->stream_count[vc] += enable ? 1 : -1;
> > --
> > 2.15.1
> >
>
> --
> Sakari Ailus
> sakari.ailus@xxxxxxxxxxxxxxx
--
Regards,
Niklas Söderlund
