lkml.org 
[lkml]   [2022]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@ideasonboard.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@renesas.com>
    > > > > > > > >
    > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@cogentembedded.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

    \
     
     \ /
      Last update: 2022-09-27 21:24    [W:8.993 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site