Hi Hans, On Fri, Aug 17, 2018 at 7:09 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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: > >> [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
