Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Lad, Prabhakar]

Hi Laurent,

On Sun, Sep 8, 2024 at 11:39 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Sat, Sep 07, 2024 at 10:09:10PM +0100, Lad, Prabhakar wrote:
> > On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart wrote:
> > > On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > >
> > > > The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
> > > > virtual channel should be processed from the four available VCs. To
> > > > retrieve this information from the connected subdevice, the
> > > > .get_frame_desc() function is called.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > ---
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
> > > >  1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > index bbf4674f888d..6101a070e785 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> > > >       spin_unlock_irqrestore(&cru->qlock, flags);
> > > >  }
> > > >
> > > > +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> > > > +{
> > > > +     struct v4l2_mbus_frame_desc fd = { };
> > > > +     struct media_pad *pad;
> > > > +     int ret;
> > > > +
> > > > +     pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);
> > >
> > > It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding
> > > the pad number. That would require moving rzg2l_csi2_pads to the shared
> > > header. You can do that on top.
> >
> > With the below comment we dont need to move rzg2l_csi2_pads into the
> > shared header.
> >
> > > An now that I've said that, is it really the source pad you need here ?
> >
> > Ouch you are right.
> >
> > > > +     if (IS_ERR(pad))
> > > > +             return PTR_ERR(pad);
> > >
> > > Can this happen, or would the pipeline fail to validate ? I think you
> > > can set the MUST_CONNECT flag on the sink pad, then you'll have a
> > > guarantee something will be connected.
> >
> > After adding the MUST_CONNECT flag, I wouldn't need the  above
> > media_pad_remote_pad_unique()...
> >
> > > > +
> > > > +     ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> > > > +                            pad->index, &fd);
> >
> > ... and here I can use '0' instead
>
> Can you ? You need to call the operation on the pad of the connected
> entity that is connected to tbe sink pad of the IP entity. That would be
> the source pad of the CSI-2 RX in this case, but it can't be hardcoded
> as it could also bethe source pad of a parallel sensor (once support for
> that will be implemented). I think you therefore need to keep the
> media_pad_remote_pad_unique() call.
>
media pipeline for RZ/G2L is [0].

Calling media_pad_remote_pad_unique() with sink pad of IP entity will
return source pad of CSI-Rx, where the pad index will be '1', passing
pad index '1' to .get_frame_desc to CSI subdev and then OV5645 subdev
would return -EINVAL.

I need to update patch [1] instead of just forwarding the pad index to
remote subdev I'll need to do the same as done IP subdev ie in CSI
subdev call media_pad_remote_pad_unique() on sink pad of CSI which
will return OV5465 source pad, and this will have a pad index of '0'.
The CSI get_frame_desc() will look something like below:

static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
                     struct v4l2_mbus_frame_desc *fd)
{
    struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
    struct media_pad *remote_pad;

    if (!csi2->remote_source)
        return -ENODEV;

    remote_pad = media_pad_remote_pad_unique(&csi2->pads[RZG2L_CSI2_SINK]);
    if (IS_ERR(remote_pad)) {
        dev_err(csi2->dev, "can't get source pad of %s (%ld)\n",
            csi2->remote_source->name, PTR_ERR(remote_pad));
        return PTR_ERR(remote_pad);
    }
    return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc,
remote_pad->index, fd);
}

For the parallel input case I plan to implement something similar to
R-Car vin with bool flag 'is_csi' where we skip calling
'rzg2l_cru_get_virtual_channel'.

[0] https://postimg.cc/rz0vSMLC
[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240906173947.282402-2-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/

Cheers,
Prabhakar