| 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
| ||
|
Message-ID: <CA+V-a8t3y9kU+D4E_n-d8ZJm2foSn3dCirL+HDnHhfSNoODv-g@mail.gmail.com>
Date: Tue, 27 Sep 2022 20:23:00 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Jacopo Mondi <jacopo@...ndi.org>,
Niklas Söderlund <niklas.soderlund@...natech.se>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Geert Uytterhoeven <geert+renesas@...der.be>,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH v2 4/4] media: platform: Add Renesas RZ/G2L CRU driver
Hi Laurent,
On Mon, Sep 26, 2022 at 7:11 PM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Mon, Sep 26, 2022 at 06:27:40PM +0100, Lad, Prabhakar wrote:
> > On Mon, Sep 26, 2022 at 5:29 PM Laurent Pinchart wrote:
> > > On Mon, Sep 26, 2022 at 05:24:47PM +0100, Lad, Prabhakar wrote:
> > > > On Mon, Sep 26, 2022 at 9:59 AM Laurent Pinchart wrote:
> > > > > On Fri, Sep 23, 2022 at 08:02:12PM +0100, Lad, Prabhakar wrote:
> > > > > > On Thu, Sep 22, 2022 at 4:29 PM Laurent Pinchart wrote:
> > > > > > > On Tue, Sep 06, 2022 at 12:04:06AM +0100, Lad Prabhakar wrote:
> > > > > > > > Add v4l driver for Renesas RZ/G2L Camera data Receiving Unit.
> > > > > > > >
> > > > > > > > Based on a patch in the BSP by Hien Huynh
> > > > > > > > <hien.huynh.px@...esas.com>
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > > > > > ---
> > > > > > > > v1 -> v2
> > > > > > > > * No change
> > > > > > > >
> > > > > > > > RFC v2 -> v1
> > > > > > > > * Moved the driver to renesas folder
> > > > > > > > * Fixed review comments pointed by Jacopo
> > > > > > > >
> > > > > > > > RFC v1 -> RFC v2
> > > > > > > > * Dropped group
> > > > > > > > * Dropped CSI subdev and implemented as new driver
> > > > > > > > * Dropped "mc_" from function names
> > > > > > > > * Moved the driver to renesas folder
> > > > > > > > ---
> > > > > > > > .../media/platform/renesas/rzg2l-cru/Kconfig | 17 +
> > > > > > > > .../media/platform/renesas/rzg2l-cru/Makefile | 3 +
> > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 395 ++++++++++
> > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 152 ++++
> > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-dma.c | 734 ++++++++++++++++++
> > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-v4l2.c | 368 +++++++++
> > > > > > >
> > > > > > > I'd merge those two files together, they both handle the video node.
> > > > > > > There's a comment below that recommends adding a subdev, that should
> > > > > > > then go to a separate file.
> > > > > >
> > > > > > OK, I'll merge these files into rzg2l-video.c.
> > > > > >
> > > > > > > > 6 files changed, 1669 insertions(+)
> > > > > > > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > > > > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-dma.c
> > > > > > > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-v4l2.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/Kconfig b/drivers/media/platform/renesas/rzg2l-cru/Kconfig
> > > > > > > > index 57c40bb499df..08ff0e96b3f5 100644
> > > > > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/Kconfig
> > > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/Kconfig
> > > > > > > > @@ -15,3 +15,20 @@ config VIDEO_RZG2L_CSI2
> > > > > > > >
> > > > > > > > To compile this driver as a module, choose M here: the
> > > > > > > > module will be called rzg2l-csi2.
> > > > > > > > +
> > > > > > > > +config VIDEO_RZG2L_CRU
> > > > > > > > + tristate "RZ/G2L Camera Receiving Unit (CRU) Driver"
> > > > > > > > + depends on ARCH_RENESAS || COMPILE_TEST
> > > > > > > > + depends on V4L_PLATFORM_DRIVERS
> > > > > > > > + depends on VIDEO_DEV && OF
> > > > > > > > + select MEDIA_CONTROLLER
> > > > > > > > + select V4L2_FWNODE
> > > > > > > > + select VIDEOBUF2_DMA_CONTIG
> > > > > > > > + select VIDEO_RZG2L_CSI2
> > > > > > >
> > > > > > > Is this required, can't the CRU be used with a parallel sensor without
> > > > > > > the CSI-2 receiver ?
> > > > > >
> > > > > > Yes the CRU can be used with parallel sensors, I'll drop the above select.
> > > > > >
> > > > > > > > + select VIDEO_V4L2_SUBDEV_API
> > > > > > > > + help
> > > > > > > > + Support for Renesas RZ/G2L (and alike SoC's) Camera Receiving
> > > > > > > > + Unit (CRU) driver.
> > > > > > > > +
> > > > > > > > + To compile this driver as a module, choose M here: the
> > > > > > > > + module will be called rzg2l-cru.
> > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/Makefile b/drivers/media/platform/renesas/rzg2l-cru/Makefile
> > > > > > > > index 91ea97a944e6..7628809e953f 100644
> > > > > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/Makefile
> > > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/Makefile
> > > > > > > > @@ -1,3 +1,6 @@
> > > > > > > > # SPDX-License-Identifier: GPL-2.0
> > > > > > > >
> > > > > > > > obj-$(CONFIG_VIDEO_RZG2L_CSI2) += rzg2l-csi2.o
> > > > > > > > +
> > > > > > > > +rzg2l-cru-objs = rzg2l-core.o rzg2l-dma.o rzg2l-v4l2.o
> > > > > > > > +obj-$(CONFIG_VIDEO_RZG2L_CRU) += rzg2l-cru.o
> > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..b5d4110b1913
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > @@ -0,0 +1,395 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > > +/*
> > > > > > > > + * Driver for Renesas RZ/G2L CRU
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2022 Renesas Electronics Corp.
> > > > > > > > + *
> > > > > > > > + * Based on Renesas R-Car VIN
> > > > > > > > + * Copyright (C) 2011-2013 Renesas Solutions Corp.
> > > > > > > > + * Copyright (C) 2013 Cogent Embedded, Inc., <source@...entembedded.com>
> > > > > > > > + * Copyright (C) 2008 Magnus Damm
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/clk.h>
> > > > > > > > +#include <linux/module.h>
> > > > > > > > +#include <linux/mod_devicetable.h>
> > > > > > > > +#include <linux/of.h>
> > > > > > > > +#include <linux/of_device.h>
> > > > > > > > +#include <linux/of_graph.h>
> > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > > +
> > > > > > > > +#include <media/v4l2-fwnode.h>
> > > > > > > > +#include <media/v4l2-mc.h>
> > > > > > > > +
> > > > > > > > +#include "rzg2l-cru.h"
> > > > > > > > +
> > > > > > > > +#define v4l2_dev_to_cru(d) container_of(d, struct rzg2l_cru_dev, v4l2_dev)
> > > > > > >
> > > > > > > As this macro is only used to get the rzg2l_cru_dev pointer from the
> > > > > > > v4l2_async_notifier pointer, you can replace it with
> > > > > > >
> > > > > > > #define notifier_to_cru(n) container_of(n, struct rzg2l_cru_dev, notifier)
> > > > > > >
> > > > > > > I would also turn it into a static inline function for additional
> > > > > > > compile-time type safety.
> > > > > >
> > > > > > OK, I will do it as mentioned above.
> > > > > >
> > > > > > > > +
> > > > > > > > +static int rzg2l_cru_csi2_link_notify(struct media_link *link, u32 flags,
> > > > > > > > + unsigned int notification)
> > > > > > > > +{
> > > > > > > > + struct media_entity *entity;
> > > > > > > > + struct rzg2l_cru_dev *cru;
> > > > > > > > + struct media_pad *csi_pad;
> > > > > > > > + struct v4l2_subdev *sd;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + ret = v4l2_pipeline_link_notify(link, flags, notification);
> > > > > > > > + if (ret)
> > > > > > > > + return ret;
> > > > > > > > +
> > > > > > > > + /* Only care about link enablement for CRU nodes. */
> > > > > > > > + if (!(flags & MEDIA_LNK_FL_ENABLED))
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > > > + cru = container_of(link->graph_obj.mdev, struct rzg2l_cru_dev, mdev);
> > > > > > > > + /*
> > > > > > > > + * Don't allow link changes if any entity in the graph is
> > > > > > > > + * streaming, modifying the CHSEL register fields can disrupt
> > > > > > > > + * running streams.
> > > > > > > > + */
> > > > > > > > + media_device_for_each_entity(entity, &cru->mdev)
> > > > > > > > + if (media_entity_is_streaming(entity))
> > > > > > > > + return -EBUSY;
> > > > > > > > +
> > > > > > > > + mutex_lock(&cru->mdev_lock);
> > > > > > > > +
> > > > > > > > + csi_pad = media_pad_remote_pad_first(&cru->vdev.entity.pads[0]);
> > > > > > > > + if (csi_pad) {
> > > > > > > > + ret = -EMLINK;
> > > > > > > > + goto out;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + sd = media_entity_to_v4l2_subdev(link->source->entity);
> > > > > > > > + if (cru->csi.subdev == sd) {
> > > > > > > > + cru->csi.channel = link->source->index - 1;
> > > > > > > > + cru->is_csi = true;
> > > > > > > > + } else {
> > > > > > > > + ret = -ENODEV;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > +out:
> > > > > > > > + mutex_unlock(&cru->mdev_lock);
> > > > > > > > +
> > > > > > > > + return ret;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct media_device_ops rzg2l_cru_media_ops = {
> > > > > > > > + .link_notify = rzg2l_cru_csi2_link_notify,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/* -----------------------------------------------------------------------------
> > > > > > > > + * Group async notifier
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +static int rzg2l_cru_group_notify_complete(struct v4l2_async_notifier *notifier)
> > > > > > > > +{
> > > > > > > > + struct rzg2l_cru_dev *cru = v4l2_dev_to_cru(notifier->v4l2_dev);
> > > > > > > > + unsigned int i;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + ret = media_device_register(&cru->mdev);
> > > > > > > > + if (ret)
> > > > > > > > + return ret;
> > > > > > >
> > > > > > > I'd move the v4l2_device_register() call here, as it's the V4L2
> > > > > > > counterpart of the media device, and handling them together would be
> > > > > > > best.
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > > > +
> > > > > > > > + ret = v4l2_device_register_subdev_nodes(&cru->v4l2_dev);
> > > > > > > > + if (ret) {
> > > > > > > > + dev_err(cru->dev, "Failed to register subdev nodes\n");
> > > > > > > > + return ret;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (!video_is_registered(&cru->vdev)) {
> > > > > > >
> > > > > > > Can this happen ?
> > > > > >
> > > > > > No, I'll drop this check.
> > > > > >
> > > > > > > > + ret = rzg2l_cru_v4l2_register(cru);
> > > > > > > > + if (ret)
> > > > > > > > + return ret;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + /* Create all media device links between CRU and CSI-2's. */
> > > > > > > > + /*
> > > > > > > > + * TODO: RZ/G2L supports 4 VC0, as support for virtual channels
> > > > > > > > + * should be implemented by streams API which is under development
> > > > > > > > + * so for now just link it to VC0
> > > > > > > > + */
> > > > > > >
> > > > > > > The streams API won't require more links, so I'd drop the comment and
> > > > > > > the loop and create a single link.
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > > > + for (i = 1; i <= 1; i++) {
> > > > > > > > + struct media_entity *source, *sink;
> > > > > > > > +
> > > > > > > > + source = &cru->csi.subdev->entity;
> > > > > > > > + sink = &cru->vdev.entity;
> > > > > > >
> > > > > > > Hmmm... I'd recommend adding a subdev to model the image processing
> > > > > > > pipeline of the CRU, between the CSI-2 receiver and the video node. That
> > > > > > > will help when you'll add support for parallel sensors, and it will also
> > > > > > > be needed by the streams API to select which virtual channel to capture.
> > > > > >/
> > > > > > just model as a dummy subdev for now (MEDIA_ENT_F_VID_MUX)?
> > > > >
> > > > > I think MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER would be more
> > > > > appropriate.
> > > >
> > > > OK I will use MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER. As this will be
> > > > just like a switch should I be implementing the get_fmt/set_fmt
> > > > callbacks?
> > >
> > > Yes, subdev operations need to be implemented, especially given that the
> > > CRU implements color space conversion, so the input and output formats
> > > of the subdev can be different.
> >
> > OK, the reason I asked as, for the sink pad the format of IP subdev
> > will be the same as remote source pad (i.e. either from CSI/parallel
> > subdev) and for the source pad this will be the same as format on the
> > video node (ie CRU output).
> >
> > get_fmt -> we get the subdev pad fmt of the remote source and return it.
> > set_fmt -> we just pass through, as the fmt is set on the video dev node.
> >
> > Does the above sound good?
>
> No, subdevs must not query each other to implement those operations.
> Subdevs are configured by userspace separately from each other, so a
> subdev set_fmt handler must accept any format that is valid for the
> subdev, regardless of how the connected subdevs or video nodes are
> configured. get_fmt just returns the format that has been set on the
> subdev. Validation of the pipeline, to check that connected pads have a
> compatible format, is done when starting the stream.
>
Thanks for reminding me of this ;) . It's the essence of the MC framework.
Cheers,
Prabhakar
Powered by blists - more mailing lists