Hi Laurent, Sakari On Mon, May 27, 2024 at 06:39:24PM GMT, Laurent Pinchart wrote: > 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 can yes :) > > I don't expect we'll need the number of groups in the upstream driver. > > No, not the number, you're right, it would be silly to keep the macro and define it to 1. I'm all for removing the for-loops that create and destroy groups and for removing from the group structure the fields that are now global (namely the v4l2_device, the subdev and the media_device), but I would like to keep the 'group' wrapper in-place, even if we'll end up having a single group in mainline. > > 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 ? > I suspect we'll have to re-introduce contexts somehow, that's the reason why I would like to keep the group structure in place. I can experiment with removing the global fields from the group and check how much refactoring does it take to do so. > > > 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
