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