On Tue, Aug 21, 2018 at 7:00 PM Sakari Ailus <sakari.ai...@iki.fi> wrote:
>
> 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+sams...@kernel.org> wrote:
> > >>
> > >> Em Thu, 16 Aug 2018 12:25:25 +0200
> > >> Hans Verkuil <hverk...@xs4all.nl> 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?

+1 for the other reasons I mentioned in my another reply too.

Best regards,
Tomasz

Reply via email to