Hi Ricardo,
On 25/06/2021 15:55, Ricardo Ribalda wrote:
> Hi Hans
>
> On Fri, 25 Jun 2021 at 13:07, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
>> On 25/06/2021 12:29, Ricardo Ribalda wrote:
>>> Hi Hans
>>>
>>> Did you have some hardware that did not work fine without this patch?
>>> Am I remembering correctly?
>>
>> Yes, that's correct. It's one of my webcams, but I can't remember which one
>> it is. You probably want me to test this v10?
>>
>> Regards,
>
> That would be awesome. Thanks!
You can add my:
Tested-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
to this series.
I do get these warnings (depends on the webcam model, though, some do, some don't):
Streaming ioctls:
test read/write: OK (Not Supported)
test blocking wait: OK
warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0
test MMAP (no poll): OK
warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0
test MMAP (select): OK
warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0
test MMAP (epoll): OK
warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0
test USERPTR (no poll): OK
warn: v4l2-test-buffers.cpp(438): got sequence number 1, expected 0
test USERPTR (select): OK
test DMABUF: Cannot test, specify --expbuf-device
It's something to do with the Field ID, but I'm not sure if this is really correctly
reporting a dropped frame, or if it is a false report and the sequence counter was
wrongly incremented.
This is a separate issue, though, and doesn't block this series.
Regards,
Hans
>
>>
>> Hans
>>
>>>
>>> Thanks!
>>>
>>> On Fri, 18 Jun 2021 at 14:29, Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
>>>>
>>>> If a control is inactive return -EACCES to let the userspace know that
>>>> the value will not be applied automatically when the control is active
>>>> again.
>>>>
>>>> Also make sure that query_v4l2_ctrl doesn't return an error.
>>>>
>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>>>> ---
>>>> drivers/media/usb/uvc/uvc_ctrl.c | 73 +++++++++++++++++++++-----------
>>>> 1 file changed, 49 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>>> index da44d5c0b9ad..4f80c06d3c43 100644
>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>>> @@ -1104,13 +1104,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
>>>> return "Unknown Control";
>>>> }
>>>>
>>>> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
>>>> + struct uvc_control *ctrl,
>>>> + struct uvc_control_mapping *mapping)
>>>> +{
>>>> + struct uvc_control_mapping *master_map = NULL;
>>>> + struct uvc_control *master_ctrl = NULL;
>>>> + s32 val;
>>>> + int ret;
>>>> +
>>>> + if (!mapping->master_id)
>>>> + return false;
>>>> +
>>>> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
>>>> + &master_ctrl, 0);
>>>> +
>>>> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
>>>> + return false;
>>>> +
>>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
>>>> + if (ret < 0 || val == mapping->master_manual)
>>>> + return false;
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>> struct uvc_control *ctrl,
>>>> struct uvc_control_mapping *mapping,
>>>> struct v4l2_queryctrl *v4l2_ctrl)
>>>> {
>>>> - struct uvc_control_mapping *master_map = NULL;
>>>> - struct uvc_control *master_ctrl = NULL;
>>>> const struct uvc_menu_info *menu;
>>>> unsigned int i;
>>>>
>>>> @@ -1126,18 +1149,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
>>>> v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>>
>>>> - if (mapping->master_id)
>>>> - __uvc_find_control(ctrl->entity, mapping->master_id,
>>>> - &master_map, &master_ctrl, 0);
>>>> - if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
>>>> - s32 val;
>>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
>>>> - if (ret < 0)
>>>> - return ret;
>>>> -
>>>> - if (val != mapping->master_manual)
>>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>>> - }
>>>> + if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
>>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>>>
>>>> if (!ctrl->cached) {
>>>> int ret = uvc_ctrl_populate_cache(chain, ctrl);
>>>> @@ -1660,25 +1673,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>>>> return 0;
>>>> }
>>>>
>>>> -static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
>>>> - struct v4l2_ext_controls *ctrls,
>>>> - struct uvc_control *uvc_control)
>>>> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
>>>> + struct uvc_entity *entity,
>>>> + struct v4l2_ext_controls *ctrls,
>>>> + struct uvc_control *err_control,
>>>> + int ret)
>>>> {
>>>> struct uvc_control_mapping *mapping;
>>>> struct uvc_control *ctrl_found;
>>>> unsigned int i;
>>>>
>>>> - if (!entity)
>>>> - return ctrls->count;
>>>> + if (!entity) {
>>>> + ctrls->error_idx = ctrls->count;
>>>> + return ret;
>>>> + }
>>>>
>>>> for (i = 0; i < ctrls->count; i++) {
>>>> __uvc_find_control(entity, ctrls->controls[i].id, &mapping,
>>>> &ctrl_found, 0);
>>>> - if (uvc_control == ctrl_found)
>>>> - return i;
>>>> + if (err_control == ctrl_found)
>>>> + break;
>>>> }
>>>> + ctrls->error_idx = i;
>>>> +
>>>> + /* We could not find the control that failed. */
>>>> + if (i == ctrls->count)
>>>> + return ret;
>>>> +
>>>> + if (uvc_ctrl_is_inactive(chain, err_control, mapping))
>>>> + return -EACCES;
>>>>
>>>> - return ctrls->count;
>>>> + return ret;
>>>> }
>>>>
>>>> int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>>>> @@ -1701,8 +1726,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>>>> uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
>>>> done:
>>>> if (ret < 0 && ctrls)
>>>> - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls,
>>>> - err_ctrl);
>>>> + ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
>>>> + ret);
>>>> mutex_unlock(&chain->ctrl_mutex);
>>>> return ret;
>>>> }
>>>> --
>>>> 2.32.0.288.g62a8d224e6-goog
>>>>
>>>
>>>
>>
>
>
