Re: [PATCH v2 04/34] media: bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Jacopo Mondi <jacopo@jmondi.org>]

Hello Sakari, Dave, Laurent,

On Tue, Sep 15, 2020 at 02:28:16PM +0100, Dave Stevenson wrote:
> Hi Sakari & Laurent
>
> On Tue, 15 Sep 2020 at 10:33, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Sakari,
> >
> > (With a question for Dave below)
> >
> > I'm replying to the two main points of your review. All the other
> > comments look fine at a glance, Jacopo is having a more detailed look
> > and will incorporate them in v3.
> >
> > On Tue, Sep 15, 2020 at 10:03:26AM +0300, Sakari Ailus wrote:
> > > Hi Laurent,
> > >
> > > Thanks for the patch, and my apologies for the late review. Please do cc me
> > > for v3.
> > >
> > > After a quick look I can already say this is the cleanest Unicam driver
> > > I've ever seen. But please also see my detailed comments below.
> > >
> > > On Mon, May 04, 2020 at 12:25:41PM +0300, Laurent Pinchart wrote:
> > > > From: Naushir Patuck <naush@raspberrypi.com>
> > > >
> > > > Add a driver for the Unicam camera receiver block on BCM283x processors.
> > > > Compared to the bcm2835-camera driver present in staging, this driver
> > > > handles the Unicam block only (CSI-2 receiver), and doesn't depend on
> > > > the VC4 firmware running on the VPU.
> > > >
> > > > The commit is made up of a series of changes cherry-picked from the
> > > > rpi-5.4.y branch of https://github.com/raspberrypi/linux/ with
> > > > additional enhancements, forward-ported to the mainline kernel.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Re-fetch mbus code from subdev on a g_fmt call
> > > > - Group all ioctl disabling together
> > > > - Fix reference counting in unicam_open
> > > > - Add support for VIDIOC_[S|G]_SELECTION
> > > > ---
> > > >  MAINTAINERS                                   |    7 +
> > > >  drivers/media/platform/Kconfig                |    1 +
> > > >  drivers/media/platform/Makefile               |    2 +
> > > >  drivers/media/platform/bcm2835/Kconfig        |   15 +
> > > >  drivers/media/platform/bcm2835/Makefile       |    3 +
> > > >  .../media/platform/bcm2835/bcm2835-unicam.c   | 2825 +++++++++++++++++
> > > >  .../media/platform/bcm2835/vc4-regs-unicam.h  |  253 ++
> > > >  7 files changed, 3106 insertions(+)
> > > >  create mode 100644 drivers/media/platform/bcm2835/Kconfig
> > > >  create mode 100644 drivers/media/platform/bcm2835/Makefile
> > > >  create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
> > > >  create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
> >
> > [snip]
> >
> > > > diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c b/drivers/media/platform/bcm2835/bcm2835-unicam.c
> > > > new file mode 100644
> > > > index 000000000000..2e9387cbc1e0
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
> > > > @@ -0,0 +1,2825 @@
> >
> > [snip]
> >
> > > > +static int unicam_enum_frameintervals(struct file *file, void *priv,
> > > > +                                 struct v4l2_frmivalenum *fival)
> > > > +{
> > > > +   struct unicam_node *node = video_drvdata(file);
> > > > +   struct unicam_device *dev = node->dev;
> > > > +   const struct unicam_fmt *fmt;
> > > > +   struct v4l2_subdev_frame_interval_enum fie = {
> > > > +           .index = fival->index,
> > > > +           .width = fival->width,
> > > > +           .height = fival->height,
> > > > +           .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > > +   };
> > > > +   int ret;
> > > > +
> > > > +   fmt = find_format_by_pix(dev, fival->pixel_format);
> > > > +   if (!fmt)
> > > > +           return -EINVAL;
> > > > +
> > > > +   fie.code = fmt->code;
> > > > +   ret = v4l2_subdev_call(dev->sensor, pad, enum_frame_interval,
> > > > +                          NULL, &fie);
> > >
> > > You're adding a new CSI-2 receiver driver but your driver appears to be
> > > video node centric and does not use MC / V4L2 subdev uAPIs for pipeline
> > > configuration.
> > >
> > > This is effectively needed if you want to be able to capture embedded data.
> > >
> > > I'd also recommend it since this way the driver will be compliant with all
> > > camera sensor drivers, not just those that expose a single sub-device.
> > > There are no good ways to change this once your driver is in upstream
> > > kernel.
> > >
> > > This is also why e.g. ipu3-cio2 driver is MC-centric.
> >
> > I've had lengthy discussions with Dave on this topic. While I agree with
> > you in principle, Dave had good arguments for keeping this
> > video-node-centric. We all agreed it wasn't a perfect solution, but it
> > could still be a pragmatic one.
> >
> > If I remember correctly the discussion was in private e-mails though.
> > Dave, I'm pretty sure you're tired of repeating the same thing, but
> > Sakari can't be expected to know all we've talked about. I can try to
> > summarize your points, but I may not do a very good job at defending
> > your point of view given that I wish you would be wrong :-) Would you
> > like to summarize your position, or should I give it a go ?
>
> One previous thread was on libcamera-devel -
> https://lists.libcamera.org/pipermail/libcamera-devel/2020-February/006789.html
>
> The main stumbling point is the lack of userspace support for MC in
> the normal applications like GStreamer and FFmpeg? Or even in simpler
> apps like fswebcam or qv4l2?
> AFACIT None of them can set the resolution via MC. Surely that's one
> of the most fundamental operations.
>
> The main devices I've got working are:
> - ov5647 (5MPix Bayer)
> - imx219 (8MPix Bayer)
> - imx477 (12MPix Bayer)
> - imx290/327 (1080p global shutter Bayer or mono)
> - ov9281 (1MPix global shutter Bayer or mono)
> - ov7251 (0.31MPix global shutter)
> - tc358743 (HDMI to CSI-2 bridge)
> - adv7282m (analogue video to CSI2 bridge)
>
> None need MC for any of their functionality, and I've yet to really
> see a driver that does (perhaps I've just missed them).
>
> tc358743 & adv7282 are slightly odd in that setting the timing or
> standard sets the capture resolution. Conveying that configuration
> path to users is bad enough, and it isn't brilliantly supported by
> apps.
>
> For the sensor modules from a user's perspective having to invoke
> media-ctl to set the resolution before starting GStreamer or FFmpeg
> sucks. Why are we forcing them down that route?
> If video-node-centric then a GStreamer pipeline along the lines of
> gst-launch-1.0 -e v4l2src ! "video/x-raw,width=W,height=H,format=Y10P"
> ! v4l2convert ! "video/x-raw,width=W,height=H,format=I420" ! <some
> sink>
> just works and can set everything up. Same with FFmpeg.
> There isn't an equivalent one-line pipeline in an MC-centric world
> that can set the resolution.
>
> libcamera starts to address that restriction, but isn't applicable for
> tc358743 or adv7282, and potentially only limited use for mono sensors
> (eg users want low latency to some machine vision library).
> So, unless I've missed something, if we adopt MC it makes libcamera
> practically mandatory for all Bayer sensors, and we force users of
> those other devices into an additional API with manual configuration
> as none of the apps support it.
>
> Unicam doesn't have any significant processing stages hidden within
> it, only unpacking raw formats to 16bpp which is handled via pixel
> formats. Otherwise it purely takes the data from the CSI2/CCP2 bus and
> writes it to SDRAM.
> MC is there for the complex pipelines, but we have a simple one!
>
>
> Can you be the sales-person for MC here and tell me what problem it
> actually solves for the user in my case? I've had this driver kicking
> around in our tree for a while, so to make the change to MC means I
> need to justify it to users, and provide them the tools to do the same
> as they currently can. At present I can't do that.
> A quick look at the docs says the MC API has been finalised since
> around 4.8 by the looks of it, so nearly 4 years. IMHO For none of the
> main userspace apps to handle it says a huge amount. Sorry.
>
>
> If there was a clear way of implementing sufficient MC support within
> GStreamer and FFmpeg and there was someone willing to take on the
> work, then I could see if I can get my bosses to pay to subcontract
> the work and get it upstreamed (we don't want to be maintaining
> patches to those apps). Of course that still leaves a million and one
> other apps out there which won't work.
>
> (Thinking out loud. If there is only one link in the graph that
> terminates in the appropriate /dev/video node, then try setting the
> resolution on that? Would that cover it? Does it cover it for the
> simple cases on other hardware? Would it be acceptable to GStreamer
> and FFmpeg? Have we just introduced the mapping table between MBUS
> formats and V4L2_PIX_FMT_xxx into both apps? All these questions!)
>
> > > > +   if (ret)
> > > > +           return ret;
> > > > +
> > > > +   fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > > > +   fival->discrete = fie.interval;
> > > > +
> > > > +   return 0;
> > > > +}
> >
> > [snip]
> >
> > > > +static int register_node(struct unicam_device *unicam, struct unicam_node *node,
> > > > +                    enum v4l2_buf_type type, int pad_id)
> > > > +{
> >
> > [snip]
> >
> > > > +   if (pad_id != METADATA_PAD || unicam->sensor_embedded_data) {
> > > > +           ret = media_create_pad_link(&unicam->sensor->entity, pad_id,
> > > > +                                       &node->video_dev.entity, 0,
> > > > +                                       MEDIA_LNK_FL_ENABLED |
> > > > +                                       MEDIA_LNK_FL_IMMUTABLE);
> > >
> > > This does create two links between the sensor and the CSI-2 receiver,
> > > doesn't it?
> > >
> > > The links in Media controller represent physical links, not logical flows
> > > of data. At the time the API was added, it wasn't thought there would be a
> > > need to separate the two.
> > >
> > > There is an effort to add the concept of data flow to V4L2, but it's been
> > > complicated and we haven't had patches for the CSI-2 receiver driver to
> > > support it. Perhaps Unicam could be the first one to do that?
> >
> > I agree that this is the right approach. The V4L2 multiplexed streams
> > support seems to be one of these cursed series, where bad things happen
> > to anyone who touches it. I was about to actively start working on it
> > again back in June for a different project, which then got frozen at the
> > last minute :-S
> >
> > Would you like to give it a try ? :-) I'd be more than happy to provide
> > you hardware as a present.
>
> If you want hardware then we can provide it and save Laurent's wallet.
> Email me an address and phone number (our couriers now insist on it)
> and I can get some sorted.
>
> > > Alternatively support for embedded data could be removed in the meantime.
>
> If that's what it takes, then OK, but using embedded data removes the
> guesswork from knowing the gain and exposures actually used, and so
> avoids oscillations. Some sensors don't support embedded data so you
> have to guess, but it's a shame if it is available and it can't be
> used.
>
> I'm between a rock and a hard place:
> - Libcamera wants platforms to be supported on mainline kernels (or at
> least submitted to the list)
> - But there isn't a framework available to let us do what is needed,
> and I don't know enough of the history and use cases to really work on
> it directly.
> How to satisfy everyone? :-/
>
> Alternatively I end up with a driver that has a flag to switch between
> MC-centric and video-device-centric modes (I would say a DT flag, but
> would it get past the DT maintainers as it isn't describing the
> hardware?)
> - If you want libcamera and embedded data then go MC and you have to
> jump through hoops to set up resolution.
> - If you don't want libcamera or embedded data then go video-centric
> and use all the tools you're used to.
> Possible, but there are going to be so many conditionals dotted around
> it would get ugly.
>
> Enough rambling from me. Thanks for your time in reviewing this lot -
> hopefully we can find a way forward.
>
>    Dave

I would be pleased to see this discussion continue, but in the
meantime, not to block v3, I'll move the driver to staging if no one
objects.

Thanks
  j

>
> > > The latest patchset is here I believe:
> > >
> > > <URL:https://patchwork.kernel.org/project/linux-media/list/?series=98277>
> > >
> > > > +           if (ret)
> > > > +                   unicam_err(unicam, "Unable to create pad link for %s\n",
> > > > +                              vdev->name);
> > > > +   }
> > > > +
> > > > +   return ret;
> > > > +}
> >
> > --
> > Regards,
> >
> > Laurent Pinchart