Hello, On Mon, May 27, 2024 at 12:43:37PM +0000, Sakari Ailus wrote: > On Mon, May 27, 2024 at 12:18:54PM +0200, Jacopo Mondi wrote: > > On Mon, May 27, 2024 at 06:44:25AM GMT, Sakari Ailus wrote: > > > 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. Using the request API, we will still have contexts. Instead of having one media device per context, the contexts will be dynamically allocated, one per file handle. Would it be enough, as a first step forward, to refactor the contexts and move the media device and other related pieces out, keeping the fields that we expect will stay per-context ? > > 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. -- Regards, Laurent Pinchart
