[RFC] Request API questions

[RFC] Request API questions

[Hans Verkuil]

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.

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.

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?

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?

4) When queueing a buffer to a request with VIDIOC_QBUF you set V4L2_BUF_FLAG_REQUEST_FD
   to indicate a valid request_fd. For other queue ioctls that take a struct v4l2_buffer
   this flag and the request_fd field are just ignored. Should we return EINVAL
   instead if the flag is set for those ioctls?

   The argument for just ignoring it is that older kernels that do not know about
   this flag will ignore it as well. There is no check against unknown flags.

Regards,

	Hans

Re: [RFC] Request API questions

[Mauro Carvalho Chehab]

Em Thu, 16 Aug 2018 12:25:25 +0200
Hans Verkuil <hverkuil@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.
> 
> 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.

+.. 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?

> 
> 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.

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.

> 
> 4) When queueing a buffer to a request with VIDIOC_QBUF you set V4L2_BUF_FLAG_REQUEST_FD
>    to indicate a valid request_fd. For other queue ioctls that take a struct v4l2_buffer
>    this flag and the request_fd field are just ignored. Should we return EINVAL
>    instead if the flag is set for those ioctls?
> 
>    The argument for just ignoring it is that older kernels that do not know about
>    this flag will ignore it as well. There is no check against unknown flags.

As I answered before, I don't see any need to add extra code for checking invalid
flags.

It might make sense to ask users to clean the flag if not QBUF, just at the
eventual remote case we might want to use it on other ioctls.

Thanks,
Mauro

Re: [RFC] Request API questions

[Tomasz Figa]

On Thu, Aug 16, 2018 at 8:15 PM Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> Em Thu, 16 Aug 2018 12:25:25 +0200
> Hans Verkuil <hverkuil@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).

>
> >
> > 4) When queueing a buffer to a request with VIDIOC_QBUF you set V4L2_BUF_FLAG_REQUEST_FD
> >    to indicate a valid request_fd. For other queue ioctls that take a struct v4l2_buffer
> >    this flag and the request_fd field are just ignored. Should we return EINVAL
> >    instead if the flag is set for those ioctls?
> >
> >    The argument for just ignoring it is that older kernels that do not know about
> >    this flag will ignore it as well. There is no check against unknown flags.
>
> As I answered before, I don't see any need to add extra code for checking invalid
> flags.
>
> It might make sense to ask users to clean the flag if not QBUF, just at the
> eventual remote case we might want to use it on other ioctls.

Agreed with Mauro on this.

Best regards,
Tomasz

Re: [RFC] Request API questions

[Hans Verkuil]

On 17/08/18 12:02, Tomasz Figa wrote:
> On Thu, Aug 16, 2018 at 8:15 PM Mauro Carvalho Chehab
> <mchehab+samsung@kernel.org> wrote:
>>
>> Em Thu, 16 Aug 2018 12:25:25 +0200
>> Hans Verkuil <hverkuil@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.

Regards,

	Hans

>>
>>>
>>> 4) When queueing a buffer to a request with VIDIOC_QBUF you set V4L2_BUF_FLAG_REQUEST_FD
>>>    to indicate a valid request_fd. For other queue ioctls that take a struct v4l2_buffer
>>>    this flag and the request_fd field are just ignored. Should we return EINVAL
>>>    instead if the flag is set for those ioctls?
>>>
>>>    The argument for just ignoring it is that older kernels that do not know about
>>>    this flag will ignore it as well. There is no check against unknown flags.
>>
>> As I answered before, I don't see any need to add extra code for checking invalid
>> flags.
>>
>> It might make sense to ask users to clean the flag if not QBUF, just at the
>> eventual remote case we might want to use it on other ioctls.
> 
> Agreed with Mauro on this.
> 
> Best regards,
> Tomasz
> 

Re: [RFC] Request API questions

[Mauro Carvalho Chehab]

Em Fri, 17 Aug 2018 12:09:40 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 17/08/18 12:02, Tomasz Figa wrote:
> > On Thu, Aug 16, 2018 at 8:15 PM Mauro Carvalho Chehab
> > <mchehab+samsung@kernel.org> wrote:  
> >>
> >> Em Thu, 16 Aug 2018 12:25:25 +0200
> >> Hans Verkuil <hverkuil@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.  
> > 

...

> > 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.

Works for me.

Thanks,
Mauro

Re: [RFC] Request API questions

[Tomasz Figa]

Hi Hans,

On Fri, Aug 17, 2018 at 7:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 17/08/18 12:02, Tomasz Figa wrote:
> > On Thu, Aug 16, 2018 at 8:15 PM Mauro Carvalho Chehab
> > <mchehab+samsung@kernel.org> wrote:
> >>
> >> Em Thu, 16 Aug 2018 12:25:25 +0200
> >> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>
[snip]
> >>> 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.

Not sure I'm following. What do we differentiate between with EPERM
and EBUSY? In both cases the value is not available yet and for codecs
we decided to have that signaled by EPERM.

On top of that, I still think we should be able to tell the case of
querying for a control that can never show up in the request, even
after it completes, specifically for any non-volatile control. That
could be done by returning a different code and -EINVAL would match
the control API behavior without requests.

The general logic would be like the pseudo code below:

G_EXT_CTRLS()
{
    if ( control is not volatile )
        return -EINVAL;

    if ( request is not complete )
        return -EPERM;

    return control from the request;
}

Best regards,
Tomasz

Re: [RFC] Request API questions

[Hans Verkuil]

On 08/20/18 09:17, Tomasz Figa wrote:
> Hi Hans,
> 
> On Fri, Aug 17, 2018 at 7:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 17/08/18 12:02, Tomasz Figa wrote:
>>> On Thu, Aug 16, 2018 at 8:15 PM Mauro Carvalho Chehab
>>> <mchehab+samsung@kernel.org> wrote:
>>>>
>>>> Em Thu, 16 Aug 2018 12:25:25 +0200
>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>
> [snip]
>>>>> 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.
> 
> Not sure I'm following. What do we differentiate between with EPERM
> and EBUSY? In both cases the value is not available yet and for codecs
> we decided to have that signaled by EPERM.

EBUSY is only returned when you attempt to set a control that temporarily
cannot be written (usually because you are streaming and changing the
control value would break something).

Getting a control from a request that is not completed is in this proposal
just not permitted (at least for now, this might be relaxed in the future).

Thinking some more about this I believe that the correct error code to
return here is EACCES. This is currently returned if you try to get a
write-only control. Controls in a request are basically write-only
controls until the request completes, so this makes sense to me.


> On top of that, I still think we should be able to tell the case of
> querying for a control that can never show up in the request, even
> after it completes, specifically for any non-volatile control. That
> could be done by returning a different code and -EINVAL would match
> the control API behavior without requests.

Why can't you get non-volatile controls? I fail to see why volatile
or non-volatile makes any difference. It is perfectly fine to get
a non-volatile control from a completed request. I.e. if you set a
control like GAIN in a request, then you want to read back the
gain value used when the request was applied. There is no guarantee
that the driver didn't change the requested gain when it actually
applied it.

Regards,

	Hans

> 
> The general logic would be like the pseudo code below:
> 
> G_EXT_CTRLS()
> {
>     if ( control is not volatile )
>         return -EINVAL;
> 
>     if ( request is not complete )
>         return -EPERM;
> 
>     return control from the request;
> }
> 
> Best regards,
> Tomasz
> 

Re: [RFC] Request API questions

[Sakari Ailus]

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@kernel.org> wrote:
> >>
> >> Em Thu, 16 Aug 2018 12:25:25 +0200
> >> Hans Verkuil <hverkuil@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?

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [RFC] Request API questions

[Tomasz Figa]

On Tue, Aug 21, 2018 at 7:00 PM Sakari Ailus <sakari.ailus@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+samsung@kernel.org> wrote:
> > >>
> > >> Em Thu, 16 Aug 2018 12:25:25 +0200
> > >> Hans Verkuil <hverkuil@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

Re: [RFC] Request API questions

[Sakari Ailus]

Hi Hans,

On Thu, Aug 16, 2018 at 12:25:25PM +0200, Hans Verkuil wrote:
> 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.
> 
> 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.

It should be easy for drivers to disable controls that need to go through
requests whenever there are requests queued. There will be lots of corner
cases when you poking requests that are already validated with stuff that
was not considered when the request was validated.

Controls such as exposure time for cameras would be generally fine whereas
those such as rotation would be not.

That said, I'd guess we don't run into these issues with stateless codecs.

We still should be careful with this: allowing setting controls (or other
bits) that can also be a part of a request can prove troublesome for some
individual controls such as rotation. If we do allow setting them now and
disallow that later, that may break applications however dangerous setting
those controls may be.

> 
>    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.
> 
> 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 agree, this sounds like a good idea. The next question is though: should
this apply to requests in general, independently of the IOCTL? It would be
logical.

> 
> 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?

Is there any particular reason for that kind of a change? In general, the
device state is changed by the requests and what is not part of a request
should remain intact.

What would be the behaviour if the requests would result in changing a
value of the control that the user would read back?

> 
> 4) When queueing a buffer to a request with VIDIOC_QBUF you set V4L2_BUF_FLAG_REQUEST_FD
>    to indicate a valid request_fd. For other queue ioctls that take a struct v4l2_buffer
>    this flag and the request_fd field are just ignored. Should we return EINVAL
>    instead if the flag is set for those ioctls?

Good question. If there is no need to use the flag for other IOCTLs now,
will there be such a need in the future? Can we guarantee there will not
be? I think it'd be safer to return an error if there's no need to use
these IOCTLs with requests now.

> 
>    The argument for just ignoring it is that older kernels that do not know about
>    this flag will ignore it as well. There is no check against unknown flags.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [RFC] Request API questions

[Hans Verkuil]

Hi all,

Based on the discussion on the mailinglist I came to the following conclusions
which I will be implementing on top of the reqv18 patches:

On 08/16/18 12:25, Hans Verkuil wrote:
> 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.
> 
> 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.

We allow this. The warning in the spec is sufficient and there are legitimate
use-cases where you want to be able to do this.

> 
> 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?

Mauro wasn't too keen on it and EBADR appears to be arch dependent (different
values for different architectures).

I think using EBADR is a bit too risky.

However, I do agree with Laurent that ENOENT isn't the best error code. We have a
similar situation with vb2 and dmabuf if the fd for a dmabuf is invalid. In that
case vb2 returns -EINVAL, but it also issues a dprintk in that case so it is a
bit easier to discover the reason for EINVAL. I'll do the same.

> 
> 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?

The consensus is to prohibit this, at least for now. So attempts to get controls
from a request that is not in completed state will return -EACCES.

> 
> 4) When queueing a buffer to a request with VIDIOC_QBUF you set V4L2_BUF_FLAG_REQUEST_FD
>    to indicate a valid request_fd. For other queue ioctls that take a struct v4l2_buffer
>    this flag and the request_fd field are just ignored. Should we return EINVAL
>    instead if the flag is set for those ioctls?
> 
>    The argument for just ignoring it is that older kernels that do not know about
>    this flag will ignore it as well. There is no check against unknown flags.

We'll just leave this as-is. I'll add a note to the spec that userspace should clear this
flag for ioctls other than QBUF.

Regards,

	Hans