Re: [PATCH v7 7/8] media: raspberrypi: Add support for PiSP BE

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Sun, May 26, 2024 at 08:59:05PM +0000, Sakari Ailus wrote:
> Hi Jacppo,
> 
> Thanks for the update.
> 
> A few comments on the driver itself...
> 
> On Fri, May 24, 2024 at 04:00:22PM +0200, Jacopo Mondi wrote:
> > From: Naushir Patuck <naush@raspberrypi.com>
> > 
> > Add support for the Raspberry Pi PiSP Back End.
> > 
> > The driver has been upported from the Raspberry Pi kernel at revision
> > f74893f8a0c2 ("drivers: media: pisp_be: Update seqeuence numbers of the
> > buffers").
> > 
> > The ISP documentation is available at:
> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > 
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.org>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  MAINTAINERS                                   |    1 +
> >  drivers/media/platform/Kconfig                |    1 +
> >  drivers/media/platform/Makefile               |    1 +
> >  drivers/media/platform/raspberrypi/Kconfig    |    5 +
> >  drivers/media/platform/raspberrypi/Makefile   |    3 +
> >  .../platform/raspberrypi/pisp_be/Kconfig      |   12 +
> >  .../platform/raspberrypi/pisp_be/Makefile     |    6 +
> >  .../platform/raspberrypi/pisp_be/pisp_be.c    | 1848 +++++++++++++++++
> >  .../raspberrypi/pisp_be/pisp_be_formats.h     |  519 +++++
> >  9 files changed, 2396 insertions(+)
> >  create mode 100644 drivers/media/platform/raspberrypi/Kconfig
> >  create mode 100644 drivers/media/platform/raspberrypi/Makefile
> >  create mode 100644 drivers/media/platform/raspberrypi/pisp_be/Kconfig
> >  create mode 100644 drivers/media/platform/raspberrypi/pisp_be/Makefile
> >  create mode 100644 drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> >  create mode 100644 drivers/media/platform/raspberrypi/pisp_be/pisp_be_formats.h

[snip]

> > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> > new file mode 100644
> > index 000000000000..c4d13462eb81
> > --- /dev/null
> > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c
> > @@ -0,0 +1,1848 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PiSP Back End driver.
> > + * Copyright (c) 2021-2024 Raspberry Pi Limited.
> > + *
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/lockdep.h>
> > +#include <linux/media/raspberrypi/pisp_be_config.h>
> 
> Where is the header included from? If it's just this driver, then I'd put
> it in the driver's directory.
> 
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-vmalloc.h>
> > +
> > +#include "pisp_be_formats.h"
> > +
> > +/* Maximum number of config buffers possible */
> > +#define PISP_BE_NUM_CONFIG_BUFFERS VB2_MAX_FRAME
> > +
> > +/*
> > + * We want to support 2 independent instances allowing 2 simultaneous users
> > + * of the ISP-BE (of course they share hardware, platform resources and mutex).
> > + * Each such instance comprises a group of device nodes representing input
> > + * and output queues, and a media controller device node to describe them.
> > + */
> > +#define PISPBE_NUM_NODE_GROUPS 2
> 
> While MC and V4L2 don't have a good support for contexts currently, just
> duplicating the device nodes is a really poor solution. We should do better
> than that. If we merge this, where is the limit in the number of contexts?
> Is it 4? 8? Or when we run out of minor numbers?
> 
> One API-based solution could be moving the IOCTL interface to MC device
> node only. This wouldn't be a small change so I'm not proposing doing that
> now.

I think we could also use the request API. It is a bit more cumbersome
to use from a userspace point of view, but this driver is meant to be
used from libcamera, so we can isolate applications from the extra
burden.

We will need to add support for formats in the request API (or rather
for requests in the format ioctls).

From a kernel point of view, the helpers used by the codec drivers may
not be suitable for ISP drivers, but I don't think it would be very
difficult to implement other helpers is needed, isolating the ISP driver
from the complexity of the request API.

This doesn't preclude developing a better userspace API with ioctls on
the MC device node only at a later point. If the above-mentioned kernel
helpers are done right, transitioning to a new userspace API will have
minimal impact on drivers.

> The two short term alternatives I can think of are:
> 
> - Merge the driver with one set of device nodes. Once the better APIs are
>   available, move to use those.

That could be a suitable short term option. It would allow merging the
userspace code in libcamera, which I would really like to do sooner than
later.

> - Merge the driver to the staging tree. I'm not very eager to go this route
>   as the drivers simply end up being abandoned in the staging tree. Work to
>   get the driver out of staging should continue.

I don't like this option. Regardless of whether this particular driver
would end up bit-rotting in drivers/staging/ or not (I do agree most
drivers do, we should discuss the IPU3 ImgU driver at some point), I
think the code quality is suitable for drivers/media/.

> Perhaps the upside here is that this isn't the only device that would
> benefit from better context support in MC/V4L2 so multiple parties have
> incentives to have this matter addressed.

[snip]

-- 
Regards,

Laurent Pinchart