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