On Fri, Aug 17, 2018 at 12:09:40PM +0200, Hans Verkuil wrote:
> On 17/08/18 12:02, Tomasz Figa wrote:
> > On Thu, Aug 16, 2018 at 8:15 PM Mauro Carvalho Chehab
> > <mchehab+samsung@xxxxxxxxxx> wrote:
> >>
> >> Em Thu, 16 Aug 2018 12:25:25 +0200
> >> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >>
> >>> Laurent raised a few API issues/questions in his review of the documentation.
> >>>
> >>> I've consolidated those in this RFC. I would like to know what others think
> >>> and if I should make changes.
> >
> > Thanks Hans for a nice summary and Mauro for initial input. :)
> >
> >>>
> >>> 1) Should you be allowed to set controls directly if they are also used in
> >>> requests? Right now this is allowed, although we warn in the spec that
> >>> this can lead to undefined behavior.
> >>>
> >>> In my experience being able to do this is very useful while testing,
> >>> and restricting this is not all that easy to implement. I also think it is
> >>> not our job. It is not as if something will break when you do this.
> >>>
> >>> If there really is a good reason why you can't mix this for a specific
> >>> control, then the driver can check this and return -EBUSY.
> >>
> >> IMHO, there's not much sense on preventing it. Just having a warning
> >> at the spec is enough.
> >>
> >
> > I tend to agree with Mauro on this.
> >
> > Besides testing, there are some legit use cases where a carefully
> > programmed user space may want to choose between setting controls
> > directly and via a request, depending on circumstances. For example,
> > one may want to set focus position alone (potentially a big step,
> > taking time), before even attempting to capture any frames and then,
> > when the capture starts, move the position gradually (in small steps,
> > not taking too much time) with subsequent requests, to obtain a set of
> > frames with different focus position.
> >
> >> +.. caution::
> >> +
> >> + Setting the same control through a request and also directly can lead to
> >> + undefined behavior!
> >>
> >> It is already warned with a caution. Anyone that decides to ignore a
> >> warning like that will deserve his faith if things stop work.
> >>
> >>>
> >>> 2) If request_fd in QBUF or the control ioctls is not a request fd, then we
> >>> now return ENOENT. Laurent suggests using EBADR ('Invalid request descriptor')
> >>> instead. This seems like a good idea to me. Should I change this?
> >>
> >> I don't have a strong opinion, but EBADR value seems to be arch-dependent:
> >>
> >> arch/alpha/include/uapi/asm/errno.h:#define EBADR 98 /* Invalid request descriptor */
> >> arch/mips/include/uapi/asm/errno.h:#define EBADR 51 /* Invalid request descriptor */
> >> arch/parisc/include/uapi/asm/errno.h:#define EBADR 161 /* Invalid request descriptor */
> >> arch/sparc/include/uapi/asm/errno.h:#define EBADR 103 /* Invalid request descriptor */
> >> include/uapi/asm-generic/errno.h:#define EBADR 53 /* Invalid request descriptor */
> >>
> >> Also, just because its name says "invalid request", it doesn't mean that it
> >> is the right error code. In this specific case, we're talking about a file
> >> descriptor. Invalid file descriptors is something that the FS subsystem
> >> has already a defined set of return codes. We should stick with whatever
> >> FS uses when a file descriptor is invalid.
> >>
> >> Where the VFS code returns EBADR? Does it make sense for our use cases?
> >>
> >
> > DMA-buf framework seems to return -EINVAL if a non-DMA-buf FD is
> > passed to dma_buf_get():
> > https://elixir.bootlin.com/linux/v4.18.1/source/drivers/dma-buf/dma-buf.c#L497
> >
> >>>
> >>> 3) Calling VIDIOC_G_EXT_CTRLS for a request that has not been queued yet will
> >>> return either the value of the control you set earlier in the request, or
> >>> the current HW control value if it was never set in the request.
> >>>
> >>> I believe it makes sense to return what was set in the request previously
> >>> (if you set it, you should be able to get it), but it is an idea to return
> >>> ENOENT when calling this for controls that are NOT in the request.
> >>>
> >>> I'm inclined to implement that. Opinions?
> >>
> >> Return the request "cached" value, IMO, doesn't make sense. If the
> >> application needs such cache, it can implement itself.
> >
> > Can we think about any specific use cases for a user space that first
> > sets a control value to a request and then needs to ask the kernel to
> > get the value back? After all, it was the user space which set the
> > value, so I'm not sure if there is any need for the kernel to be an
> > intermediary here.
> >
> >>
> >> Return an error code if the request has not yet completed makes
> >> sense. Not sure what would be the best error code here... if the
> >> request is queued already (but not processed), EBUSY seems to be the
> >> better choice, but, if it was not queued yet, I'm not sure. I guess
> >> ENOENT would work.
> >
> > IMHO, as far as we assign unique error codes for different conditions
> > and document them well, we should be okay with any not absurdly
> > mismatched code. After all, most of those codes are defined for file
> > system operations and don't really map directly to anything else.
> >
> > FYI, VIDIOC_G_(EXT_)CTRL returns EINVAL if an unsupported control is
> > queried, so if we decided to keep the "cache" functionality after all,
> > perhaps we should stay consistent with it?
> > Reference: https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-ext-ctrls.html#return-value
> >
> > My suggestion would be:
> > - EINVAL: the control was not in the request, (if we keep the cache
> > functionality)
> > - EPERM: the value is not ready, (we selected this code for Decoder
> > Interface to mean that CAPTURE format is not ready, which is similar;
> > perhaps that could be consistent?)
> >
> > Note that EINVAL would only apply to writable controls, while EPERM
> > only to volatile controls, since the latter can only change due to
> > request completion (non-volatile controls can only change as an effect
> > of user space action).
> >
>
> I'm inclined to just always return EPERM when calling G_EXT_CTRLS for
> a request. We can always relax this in the future.
>
> So when a request is not yet queued G_EXT_CTRLS returns EPERM, when
> queued but not completed it returns EBUSY and once completed it will
> work as it does today.
It may not be trivial to figure out the state of the request when a control
is being accessed. Besides, it could conceivably change during the IOCTL call.
How about just using EPERM (or EBUSY) in all cases?
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
