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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240908223905.GG15491@pendragon.ideasonboard.com>
Date: Mon, 9 Sep 2024 01:39:05 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org,
	Biju Das <biju.das.jz@...renesas.com>,
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve
 virtual channel information

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@...renesas.com>
> > >
> > > 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@...renesas.com>
> > > ---
> > >  .../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.

-- 
Regards,

Laurent Pinchart