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