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

[Laurent Pinchart]

Hi Prabhakar,

On Mon, Sep 09, 2024 at 10:57:59AM +0100, Lad, Prabhakar wrote:
> On Sun, Sep 8, 2024 at 11:39 PM Laurent Pinchart wrote:
> > 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.

Why does it 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'.

Ah, that's why it returns -EINVAL :-)

You're right, the pad number can't be propagated as-is.

> 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/

-- 
Regards,

Laurent Pinchart