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

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Jacopo,

On Mon, May 27, 2024 at 12:18:54PM +0200, Jacopo Mondi wrote:
> Hi Sakari, Laurent
> 
> On Mon, May 27, 2024 at 06:44:25AM GMT, Sakari Ailus wrote:
> > Hi Laurent,
> >
> > On Mon, May 27, 2024 at 04:19:11AM +0300, Laurent Pinchart wrote:
> > > > 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.
> >
> > This is indeed the third feasible option. I agree. The work on the
> > framework side might not be that much either.
> >
> 
> For the time being, I would like to move forward and merge the version
> of the driver with a single enabled context.
> 
> As you can see the driver multiples the contexts by creating two
> groups
> 
> The number of groups is defined by
> 
> /*
>  * 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
> 
> Can I simply set this to 1 or should the driver be reworked to remove
> the group concept completely (it will be quite some rework).
> 
> You can guess what my preference is, and considering we want to
> experiment with a different API the group part will possibily need to
> be reintroduced.

I believe you can also guess what my preference is. :-)

I don't expect we'll need the number of groups in the upstream driver.

Before deciding what to do here I'd like to arrive to a conclusion on how
this gets addressed so the desired functionality is available in the
upstream driver.

Maybe the Request API is indeed something to consider here.

I wonder what Laurent thinks.

> 
> Also if we simply set PISPBE_NUM_NODE_GROUPS=1, the downstream RPi
> kernel will solely need to have 1 patch that restores the value to 2
> to be able to use the mainline kernel driver instead of keeping their
> multi-context downstream version in use until multi-context is
> finalized in mainline.

-- 
Kind regards,

Sakari Ailus