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

Commit Message

Lad, Prabhakar Sept. 6, 2024, 5:39 p.m. UTC

From: Lad Prabhakar <[e-mail]>

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 <[e-mail]>
---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Laurent Pinchart Sept. 6, 2024, 11:10 p.m. UTC | #1

Hi Prabhakar,

Thank you for the patch.

On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  .../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.

An now that I've said that, is it really the source pad you need here ?

> +	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.

> +
> +	ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> +			       pad->index, &fd);
> +	if (ret < 0 && ret != -ENOIOCTLCMD)

Printing an error message would help debugging.

> +		return ret;
> +	/* If remote subdev does not implement .get_frame_desc default to VC0. */
> +	if (ret == -ENOIOCTLCMD)
> +		return 0;
> +
> +	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)

An error message would help here too I think.

> +		return -EINVAL;
> +
> +	return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;

I think you should return an error if fd.num_entries is 0, that
shouldn't happen.

> +}
> +
>  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  {
>  	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
>  	unsigned long flags;
>  	int ret;
>  
> +	ret = rzg2l_cru_get_virtual_channel(cru);
> +	if (ret < 0)
> +		return ret;
> +	cru->csi.channel = ret;

How about passing the value to the function that needs it, instead of
storing it in cru->csi.channel ? You can do that on top and drop the
csi.channel field.

Reviewed-by: Laurent Pinchart <[e-mail]>

> +
>  	spin_lock_irqsave(&cru->qlock, flags);
>  
>  	/* Select a video input */

Lad, Prabhakar Sept. 7, 2024, 9:09 p.m. UTC | #2

Hi Laurent,

Thank you for the review.

On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart
<[e-mail]> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  .../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 or do you prefer RZG2L_CRU_IP_SINK
(I say because we are calling into remote subdev of IP which is CSI so
the RZG2L_CRU_IP_SINK wont make sense)?

> > +     if (ret < 0 && ret != -ENOIOCTLCMD)
>
> Printing an error message would help debugging.
>
OK, I will add.

> > +             return ret;
> > +     /* If remote subdev does not implement .get_frame_desc default to VC0. */
> > +     if (ret == -ENOIOCTLCMD)
> > +             return 0;
> > +
> > +     if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>
> An error message would help here too I think.
>
OK, I will add.

> > +             return -EINVAL;
> > +
> > +     return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;
>
> I think you should return an error if fd.num_entries is 0, that
> shouldn't happen.
>
OK, I will add.

> > +}
> > +
> >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> >  {
> >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> >       unsigned long flags;
> >       int ret;
> >
> > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > +     if (ret < 0)
> > +             return ret;
> > +     cru->csi.channel = ret;
>
> How about passing the value to the function that needs it, instead of
> storing it in cru->csi.channel ? You can do that on top and drop the
> csi.channel field.
>
OK, let me check if this can be done.

Cheers,
Prabhakar

Lad, Prabhakar Sept. 8, 2024, 8:23 p.m. UTC | #3

Hi Laurent,

On Sat, Sep 7, 2024 at 10:09 PM Lad, Prabhakar
<[e-mail]> wrote:
>
> Hi Laurent,
>
> Thank you for the review.
>
> On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart
> <[e-mail]> wrote:
<snip>
> > > +
> > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> > >  {
> > >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> > >       unsigned long flags;
> > >       int ret;
> > >
> > > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     cru->csi.channel = ret;
> >
> > How about passing the value to the function that needs it, instead of
> > storing it in cru->csi.channel ? You can do that on top and drop the
> > csi.channel field.
> >
> OK, let me check if this can be done.
>
The virtual channel number is programmed in rzg2l_cru_csi2_setup() [0]
call, below is the code flow of the call. This code flow is called by
spinlock held.

rzg2l_cru_start_image_processing()
    rzg2l_cru_initialize_image_conv()
        rzg2l_cru_csi2_setup()

When rzg2l_cru_get_virtual_channel() is called directly in
rzg2l_cru_csi2_setup() function we get "BUG: Invalid wait context"
(when PROVE_LOCKING is enabled) this is due to we do a mutex lock as
part of v4l2_subdev_lock_and_get_active_state() in get_frame_desc.

So probably I'll leave this as it is now.

[0] https://elixir.bootlin.com/linux/v6.10.8/source/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c#L311

Cheers,
Prabhakar

Laurent Pinchart Sept. 8, 2024, 10:39 p.m. UTC | #4

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 <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  .../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.

> or do you prefer RZG2L_CRU_IP_SINK
> (I say because we are calling into remote subdev of IP which is CSI so
> the RZG2L_CRU_IP_SINK wont make sense)?
> 
> > > +     if (ret < 0 && ret != -ENOIOCTLCMD)
> >
> > Printing an error message would help debugging.
> >
> OK, I will add.
> 
> > > +             return ret;
> > > +     /* If remote subdev does not implement .get_frame_desc default to VC0. */
> > > +     if (ret == -ENOIOCTLCMD)
> > > +             return 0;
> > > +
> > > +     if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> >
> > An error message would help here too I think.
> >
> OK, I will add.
> 
> > > +             return -EINVAL;
> > > +
> > > +     return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;
> >
> > I think you should return an error if fd.num_entries is 0, that
> > shouldn't happen.
> >
> OK, I will add.
> 
> > > +}
> > > +
> > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> > >  {
> > >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> > >       unsigned long flags;
> > >       int ret;
> > >
> > > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     cru->csi.channel = ret;
> >
> > How about passing the value to the function that needs it, instead of
> > storing it in cru->csi.channel ? You can do that on top and drop the
> > csi.channel field.
> >
> OK, let me check if this can be done.

Lad, Prabhakar Sept. 9, 2024, 9:57 a.m. UTC | #5

Hi Laurent,

On Sun, Sep 8, 2024 at 11:39 PM Laurent Pinchart
<[e-mail]> 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 <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >  .../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/[e-mail]/

Cheers,
Prabhakar

Laurent Pinchart Sept. 9, 2024, 10:07 a.m. UTC | #6

On Sun, Sep 08, 2024 at 09:23:52PM +0100, Lad, Prabhakar wrote:
> On Sat, Sep 7, 2024 at 10:09 PM Lad, Prabhakar wrote:
> > On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart wrote:
>
> <snip>
>
> > > > +
> > > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> > > >  {
> > > >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> > > >       unsigned long flags;
> > > >       int ret;
> > > >
> > > > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +     cru->csi.channel = ret;
> > >
> > > How about passing the value to the function that needs it, instead of
> > > storing it in cru->csi.channel ? You can do that on top and drop the
> > > csi.channel field.
> >
> > OK, let me check if this can be done.
>
> The virtual channel number is programmed in rzg2l_cru_csi2_setup() [0]
> call, below is the code flow of the call. This code flow is called by
> spinlock held.
> 
> rzg2l_cru_start_image_processing()
>     rzg2l_cru_initialize_image_conv()
>         rzg2l_cru_csi2_setup()
> 
> When rzg2l_cru_get_virtual_channel() is called directly in
> rzg2l_cru_csi2_setup() function we get "BUG: Invalid wait context"
> (when PROVE_LOCKING is enabled) this is due to we do a mutex lock as
> part of v4l2_subdev_lock_and_get_active_state() in get_frame_desc.

I didn't mean calling rzg2l_cru_get_virtual_channel() from
rzg2l_cru_csi2_setup(), but passing the virtual channel number from
rzg2l_cru_start_image_processing() to rzg2l_cru_initialize_image_conv()
and then to rzg2l_cru_csi2_setup().

> So probably I'll leave this as it is now.
> 
> [0] https://elixir.bootlin.com/linux/v6.10.8/source/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c#L311

Laurent Pinchart Sept. 9, 2024, 12:54 p.m. UTC | #7

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 <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  .../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/[e-mail]/

Patch

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]);
+	if (IS_ERR(pad))
+		return PTR_ERR(pad);
+
+	ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
+			       pad->index, &fd);
+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		return ret;
+	/* If remote subdev does not implement .get_frame_desc default to VC0. */
+	if (ret == -ENOIOCTLCMD)
+		return 0;
+
+	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
+		return -EINVAL;
+
+	return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;
+}
+
 int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 {
 	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
 	unsigned long flags;
 	int ret;
 
+	ret = rzg2l_cru_get_virtual_channel(cru);
+	if (ret < 0)
+		return ret;
+	cru->csi.channel = ret;
+
 	spin_lock_irqsave(&cru->qlock, flags);
 
 	/* Select a video input */