Hi Laurent, Thanks for detailed review. Please let me add my thoughts inline as well. On Fri, May 18, 2018 at 11:46 PM Laurent Pinchart < laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > Hi Hans, > Thank you for the patch. > On Thursday, 3 May 2018 17:53:12 EEST Hans Verkuil wrote: [snip] > > +Name > > +==== > > + > > +MEDIA_REQUEST_IOC_REINIT - Re-initialize a request > > + > > + > > +Synopsis > > +======== > > + > > +.. c:function:: int ioctl( int request_fd, MEDIA_REQUEST_IOC_REINIT ) > > + :name: MEDIA_REQUEST_IOC_REINIT > > + > > + > > +Arguments > > +========= > > + > > +``request_fd`` > > + File descriptor returned by :ref:`MEDIA_IOC_REQUEST_ALLOC`. > I was expecting an argument to this ioctl, with a reference to another request > (and I expected the same reference for the MEDIA_IOC_REQUEST_ALLOC ioctl). I > think this topic has been discussed several times in the past, but possibly > not in venues where everybody was present, so I'll detail the problem here. > The idea behind this version of the request API is to split request handling > in three parts: > 1. A request is allocated (MEDIA_IOC_REQUEST_ALLOC) or an existing request is > re-initialized (MEDIA_REQUEST_IOC_REINIT). At that point the request contains > no property (control, format, ...) to be modified. > 2. The request is filled with properties to be modified using existing V4L2 > ioctls (VIDIOC_S_EXT_CTRLS, VIDIOC_SUBDEV_S_FMT, VIDIOC_SUBDEV_S_SELECTION, > ..). The current implementation supports controls only, with support for > formats and selection rectangles to be added later. > 3. The request is queued with MEDIA_REQUEST_IOC_QUEUE. > As usual the devil is in the details, and the details of the second step in > particular. > The idea behind splitting request handling in those three steps is to reuse > existing V4L2 ioctls as much as possible, and to keep the existing semantics > of those ioctls. I believe this isn't possible with the proposed > MEDIA_IOC_REQUEST_ALLOC and MEDIA_REQUEST_IOC_REINIT ioctls, neither for > controls nor for formats or selection rectangles, although the problem is more > limited for controls. I will thus take formats and selection rectangles to > explain where the problem lies. > The S_FMT and S_SELECTION semantics on subdevs (and, for that matter, on video > nodes too) is that those ioctls don't fail when passed parameters invalid for > the device, but return a valid configuration as close as possible to the > requested one. This allows implementing negotiation of formats and selection > rectangles through an iterative process that involves back-and-forth calls > between user space and kernel space until the application finds a usable > configuration accepted by the driver, or until it decides to fail because such > a configuration doesn't exist. (As a reminder, such negotiation is needed > because it was deemed impossible to create a proper API that would expose all > device constraints in a static way.) > Formats and selection rectangles are propagated within subdevs from sink pads > to source pads by the driver. Sink formats and selection rectangles thus > influence the result of negotiation on source pads. For instance, on a scaler > subdev that can't perform format conversion, the format on the source pad will > be dictated by the format on the sink pad. In order to implement S_FMT on the > source pad the kernel driver thus needs context information to get the format > on the sink pad. > Without the request API this is easy. The context is either stored in the > driver-specific device structure (for active formats) or in the file handle > (for try formats). With the proposed request API, however, we have no such > context: the requests are created empty (or re-initialized to be empty). > Kernel drivers can access the active state of the device, or the state of > previously queued requests, but can't access the state of prepared but not yet > queued requests. > My proposal to solve this was to link requests by specifying the handle of a > previous request to the MEDIA_IOC_REQUEST_ALLOC and MEDIA_REQUEST_IOC_REINIT > ioctls. That way kernel drivers would know in which order requests will be > queued, and would be able to access the correct context information to > implement the existing semantics of the S_FMT and S_SELECTION ioctls (as well > as being able to implement the G_FMT and G_SELECTION ioctls). Note that the > problem isn't limited to format and selection rectangles, similar problems can > exist when multiple controls are related, such as auto and manual controls. > This would result in a more complex API and a more complex request lifetime > management, as well as a more complex semantics of the request API. While I > thought this was unavoidable, I came to reconsider my position on this topic, > and I believe that the current request API proposal is acceptable in that > regard, provided that we agree that the existing semantics of the S_FMT and > S_SELECTION ioctls won't need to be implemented for requests as it won't be > possible to do so. The reason that prompted me to change my mind is that I > believe the API would be simpler to define and simpler to use if we dropped > that semantics, and required applications to create requests that are fully > valid instead of trying to mangle the content of a request the same way we do > for format and selection rectangle negotiation. Obviously, negotiation is > still needed in order for applications to find a valid configuration, but we > have the V4L2_SUBDEV_FORMAT_TRY API for that that, in conjunction with the > S_FMT and S_SELECTION ioctls, can be used to negotiate a configuration. The > negotiated configuration could then be set in a request, and would be > guaranteed to be valid. I really like the idea of requiring the requests to be valid. I believe this is something similar to DRM atomic API and it avoids the state negotiation hell in the drivers and userspace, where the drivers would be adjusting settings back and forth and eventually arriving to some values not matching any of the ones originally given. [snip] > > Codec +v2). One such requirement is the ability for devices belonging to > > the same +pipeline to reconfigure and collaborate closely on a per-frame > > basis. Another is +efficient support of stateless codecs, which need > > per-frame controls to be set +asynchronously in order to be used > > efficiently. > Do you really mean asynchronously ? I thought the purpose of the request API > with codecs was to set controls synchronously with frames. Set controls synchronized with frames, but submit frames (with matching controls) to be processed asynchronously, as compared to no request API, when you would have to: - set controls, - queue buffer, - wait for decoding, -set controls, - queue buffer, and so on, all synchronously. [snip] > > synchronize +so the requested pipeline's topology is applied before the > > buffers are +processed. This is at the discretion of media controller > > drivers and is not a +requirement. > One of the main purposes of the request API is to support atomic > configuration. Saying that it's simply optional defeats that purpose. I agree > that atomicity can't be achieved in all cases, and that there will often be a > best-effort approach, but for hardware devices that support atomic > configuration I think applications need to be able to rely on the driver > implementing that feature. I would thus put a bit more pressure on drivers > than just saying it's fully optional. I think we should say that drivers should make their best effort to apply the state atomically. However, I'd want to avoid userspace (or users) blindly relying on the API as guarantying atomicity, since this really depends on hardware capability. > > +Buffers queued without an associated request after a request-bound buffer > > will +be processed using the state of the hardware at the time of the > > request +completion. > I'm not sure to follow that, could you explain in a bit more details ? Also, > even more than for controls (see below), I really want to know about use cases > for mixing buffers in and outside of requests for the same buffer queue before > accepting it. AFAIR this was defined just to avoid making the API have undefined behavior. I can't think of any real use case for mixing requests with non-request processing (at least within the same buffer queue)... [snip] > > All the same, controls set without a request are applied +immediately, > > regardless of whether a request is in use or not. > This will be tricky to implement, and will potentially generate side effects > difficult to handle, and difficult to debug. I understand that this is a > requirement for codecs, but I don't know the exact use case(s), so I can't > comment on whether this is the right approach. I don't think this is a requirement for codecs. Not for stateless ones for sure, because for them the requirement is to actually use the requests for setting controls. As above, I think this was defined to avoid making the API have undefined behavior, but I can also see some classes of controls which should be handled regardless of buffers flow, e.g. autofocus, where the userspace just wants the motor to start moving instantly after the user clicks the screen (tap-to-focus). > While I don't necessarily > reject this feature on principle, I first want to know more about the use > case(s) before accepting it. If we end up needing it, we'll have to better > document it, by clearly specifying what is allowed and what isn't (for > instance, we might decide to allow setting control asynchronously only when > they're not simultaneously modified through a queued request), as well as the > exact behaviour that applications can expect in the allowed cases. [snip] > > +If ``request_fd`` is specified and ``which`` is set to > > ``V4L2_CTRL_WHICH_REQUEST_VAL`` +during a call to :ref:`VIDIOC_G_EXT_CTRLS > > <VIDIOC_G_EXT_CTRLS>`, then the +returned values will be the values > > currently set for the request (or the +hardware value if none is set) > Do we really have a need for returning the hardware value ? That seems a bit > random to me, wouldn't it be better to return an error instead ? As per my autofocus example above and also things like various statistics from camera sensor that don't really depend on other state (e.g. absolute light intensity), we want to read the values instantly, for example to drive the userspace 3A loop. [snip] > > diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst > > b/Documentation/media/uapi/v4l/vidioc-qbuf.rst index > > 9e448a4aa3aa..c9f55d8340d1 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst > > +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst > > @@ -98,6 +98,13 @@ dequeued, until the :ref:`VIDIOC_STREAMOFF > > <VIDIOC_STREAMON>` or > > :ref:`VIDIOC_REQBUFS` ioctl is called, or until the > > > > device is closed. > > > > +The ``request_fd`` field can be used when queuing to specify the file > > +descriptor of a request, if requests are in use. Setting it means that the > > +buffer will not be passed to the driver until the request itself is queued. > > +Also, the driver will apply any setting associated with the request before > > +processing the buffer. > I think we should be more precise with the wording, here and above in the > documentation. From an API point of view, we need to guarantee that the > settings from the request are used to process the image capture in the buffer, > or the image output from the buffer. Making the API guarantee that would essentially eliminate usage of this API from any hardware that can't guarantee 100% atomicity of all the state within request, which I'd say would be actually most of existing hardware. > Drivers don't process buffers other than > queuing them to the hardware, so saying that the driver will apply settings > before processing the buffer is a bit vague in my opinion, and also possibly > not very accurate depending on the hardware. The question is whether we can guarantee anything more precise. > Saying that controls are applied right before the buffer is processed by the > driver is inaccurate : for example exposure time might need several frames to > take effect, and might thus need to be applied a few frames before the buffer > is queued to the hardware for capture. I believe this would be typically accounted for the userspace queuing such exposure setting several requests before. Hardware that would be able to handle this on its own, if such exists, would be treated as 0 frame delay of exposure setting. (We might need a way to report that for drivers, though. Right now we can just have configuration files for userspace.) > The other way around is possible too : > the hardware might support a buffer queue but have no queue for controls. > Controls would then need to be applied a few frames after the buffer is queued > to the hardware in order to take effect for the right buffer. > We're specifying an API that carries very strong semantics across multiple > components and multiple objects, so I think it's very important to be as > precise as possible. Or, do you just mean, s/before processing the buffer/before hardware starts processing the buffer/? Best regards, Tomasz
