Re: [PATCH 1/5] uvcvideo: Lock controls mutex when querying menus
On Monday, November 22, 2010 15:57:38 Laurent Pinchart wrote:
> Hi Hans,
>
> On Monday 22 November 2010 10:21:41 Hans Verkuil wrote:
> > On Sunday, November 21, 2010 22:45:48 Laurent Pinchart wrote:
> > > On Sunday 21 November 2010 22:18:41 Hans Verkuil wrote:
> > > > On Sunday, November 21, 2010 21:32:49 Laurent Pinchart wrote:
> > > > > uvc_find_control() must be called with the controls mutex locked. Fix
> > > > > uvc_query_v4l2_menu() accordingly.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart
> > > > > ---
> > > > >
> > > > > drivers/media/video/uvc/uvc_ctrl.c | 48
> > > > > +++-
> > > > > drivers/media/video/uvc/uvc_v4l2.c
> > > > >
> > > > > | 36 +--
> > > > > | drivers/media/video/uvc/uvcvideo.h |
> > > > >
> > > > > 4 +-
> > > > >
> > > > > 3 files changed, 50 insertions(+), 38 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/video/uvc/uvc_ctrl.c
> > > > > b/drivers/media/video/uvc/uvc_ctrl.c index f169f77..59f8a9a 100644
> > > > > --- a/drivers/media/video/uvc/uvc_ctrl.c
> > > > > +++ b/drivers/media/video/uvc/uvc_ctrl.c
> > > > > @@ -785,7 +785,7 @@ static void __uvc_find_control(struct uvc_entity
> > > > > *entity, __u32 v4l2_id,
> > > > >
> > > > > }
> > > > >
> > > > > }
> > > > >
> > > > > -struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> > > > > +static struct uvc_control *uvc_find_control(struct uvc_video_chain
> > > > > *chain,
> > > > >
> > > > > __u32 v4l2_id, struct uvc_control_mapping **mapping)
> > > > >
> > > > > {
> > > > >
> > > > > struct uvc_control *ctrl = NULL;
> > > > >
> > > > > @@ -944,6 +944,52 @@ done:
> > > > > return ret;
> > > > >
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * Mapping V4L2 controls to UVC controls can be straighforward if
> > > > > done well. + * Most of the UVC controls exist in V4L2, and can be
> > > > > mapped directly. Some + * must be grouped (for instance the Red
> > > > > Balance, Blue Balance and Do White + * Balance V4L2 controls use the
> > > > > White Balance Component UVC control) or + * otherwise translated.
> > > > > The approach we take here is to use a translation + * table for the
> > > > > controls that can be mapped directly, and handle the others + *
> > > > > manually.
> > > > > + */
> > > > > +int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > > > > + struct v4l2_querymenu *query_menu)
> > > > > +{
> > > > > + struct uvc_menu_info *menu_info;
> > > > > + struct uvc_control_mapping *mapping;
> > > > > + struct uvc_control *ctrl;
> > > > > + u32 index = query_menu->index;
> > > > > + u32 id = query_menu->id;
> > > > > + int ret;
> > > > > +
> > > > > + memset(query_menu, 0, sizeof(*query_menu));
> > > > > + query_menu->id = id;
> > > > > + query_menu->index = index;
> > > > > +
> > > > > + ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> > > > > + if (ret < 0)
> > > > > + return -ERESTARTSYS;
> > > >
> > > > Just return 'ret' here (which is -EINTR).
> > >
> > > Hmmm... The uvcvideo driver uses -ERESTARTSYS extensively. What's the
> > > rationale behind using -EINTR instead ? Allowing users to interrupt the
> > > ioctl with ctrl-C ?
> > > If so, I wonder if it's worth it in this case, as the controls
> > > mutex will not be locked by another thread for an extensive period of
> > > time anyway.
> >
> > I don't think it is worth it here. It's perfectly fine to switch to
> > mutex_lock.
>
> I'm using mutex_lock_interruptible in several places. Is it ok if I keep this
> patch as-is for 2.6.37, and send a patch for 2.6.38 that replaces all
> mutex_lock_interruptible calls ?
Sure, no problem.
Regards,
Hans
>
> > > ERESTARTSYS is meant to be used to deliver signals to application right
> > > away and then restart the system call. With EINTR applications would see
> > > an error in response to a VIDIOC_QUERYMENU call if SIGALRM arrives at
> > > the same time. I don't think that's something we want.
> >
> > Good point. I never realized that. I will have to modify my patch series
> > for that. BTW, a quick scan in the kernel sources once again shows random
> > behavior regarding EINTR vs ERESTARTSYS.
>
>
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] uvcvideo: Lock controls mutex when querying menus
Hi Hans,
On Monday 22 November 2010 10:21:41 Hans Verkuil wrote:
> On Sunday, November 21, 2010 22:45:48 Laurent Pinchart wrote:
> > On Sunday 21 November 2010 22:18:41 Hans Verkuil wrote:
> > > On Sunday, November 21, 2010 21:32:49 Laurent Pinchart wrote:
> > > > uvc_find_control() must be called with the controls mutex locked. Fix
> > > > uvc_query_v4l2_menu() accordingly.
> > > >
> > > > Signed-off-by: Laurent Pinchart
> > > > ---
> > > >
> > > > drivers/media/video/uvc/uvc_ctrl.c | 48
> > > > +++-
> > > > drivers/media/video/uvc/uvc_v4l2.c
> > > >
> > > > | 36 +--
> > > > | drivers/media/video/uvc/uvcvideo.h |
> > > >
> > > > 4 +-
> > > >
> > > > 3 files changed, 50 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/drivers/media/video/uvc/uvc_ctrl.c
> > > > b/drivers/media/video/uvc/uvc_ctrl.c index f169f77..59f8a9a 100644
> > > > --- a/drivers/media/video/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/video/uvc/uvc_ctrl.c
> > > > @@ -785,7 +785,7 @@ static void __uvc_find_control(struct uvc_entity
> > > > *entity, __u32 v4l2_id,
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > -struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> > > > +static struct uvc_control *uvc_find_control(struct uvc_video_chain
> > > > *chain,
> > > >
> > > > __u32 v4l2_id, struct uvc_control_mapping **mapping)
> > > >
> > > > {
> > > >
> > > > struct uvc_control *ctrl = NULL;
> > > >
> > > > @@ -944,6 +944,52 @@ done:
> > > > return ret;
> > > >
> > > > }
> > > >
> > > > +/*
> > > > + * Mapping V4L2 controls to UVC controls can be straighforward if
> > > > done well. + * Most of the UVC controls exist in V4L2, and can be
> > > > mapped directly. Some + * must be grouped (for instance the Red
> > > > Balance, Blue Balance and Do White + * Balance V4L2 controls use the
> > > > White Balance Component UVC control) or + * otherwise translated.
> > > > The approach we take here is to use a translation + * table for the
> > > > controls that can be mapped directly, and handle the others + *
> > > > manually.
> > > > + */
> > > > +int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > > > + struct v4l2_querymenu *query_menu)
> > > > +{
> > > > + struct uvc_menu_info *menu_info;
> > > > + struct uvc_control_mapping *mapping;
> > > > + struct uvc_control *ctrl;
> > > > + u32 index = query_menu->index;
> > > > + u32 id = query_menu->id;
> > > > + int ret;
> > > > +
> > > > + memset(query_menu, 0, sizeof(*query_menu));
> > > > + query_menu->id = id;
> > > > + query_menu->index = index;
> > > > +
> > > > + ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> > > > + if (ret < 0)
> > > > + return -ERESTARTSYS;
> > >
> > > Just return 'ret' here (which is -EINTR).
> >
> > Hmmm... The uvcvideo driver uses -ERESTARTSYS extensively. What's the
> > rationale behind using -EINTR instead ? Allowing users to interrupt the
> > ioctl with ctrl-C ?
> > If so, I wonder if it's worth it in this case, as the controls
> > mutex will not be locked by another thread for an extensive period of
> > time anyway.
>
> I don't think it is worth it here. It's perfectly fine to switch to
> mutex_lock.
I'm using mutex_lock_interruptible in several places. Is it ok if I keep this
patch as-is for 2.6.37, and send a patch for 2.6.38 that replaces all
mutex_lock_interruptible calls ?
> > ERESTARTSYS is meant to be used to deliver signals to application right
> > away and then restart the system call. With EINTR applications would see
> > an error in response to a VIDIOC_QUERYMENU call if SIGALRM arrives at
> > the same time. I don't think that's something we want.
>
> Good point. I never realized that. I will have to modify my patch series
> for that. BTW, a quick scan in the kernel sources once again shows random
> behavior regarding EINTR vs ERESTARTSYS.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] uvcvideo: Lock controls mutex when querying menus
On Sunday, November 21, 2010 22:45:48 Laurent Pinchart wrote:
> Hi Hans,
>
> Thanks for the comment.
>
> On Sunday 21 November 2010 22:18:41 Hans Verkuil wrote:
> > On Sunday, November 21, 2010 21:32:49 Laurent Pinchart wrote:
> > > uvc_find_control() must be called with the controls mutex locked. Fix
> > > uvc_query_v4l2_menu() accordingly.
> > >
> > > Signed-off-by: Laurent Pinchart
> > > ---
> > >
> > > drivers/media/video/uvc/uvc_ctrl.c | 48
> > > +++- drivers/media/video/uvc/uvc_v4l2.c
> > > | 36 +-- drivers/media/video/uvc/uvcvideo.h |
> > > 4 +-
> > > 3 files changed, 50 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/drivers/media/video/uvc/uvc_ctrl.c
> > > b/drivers/media/video/uvc/uvc_ctrl.c index f169f77..59f8a9a 100644
> > > --- a/drivers/media/video/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/video/uvc/uvc_ctrl.c
> > > @@ -785,7 +785,7 @@ static void __uvc_find_control(struct uvc_entity
> > > *entity, __u32 v4l2_id,
> > >
> > > }
> > >
> > > }
> > >
> > > -struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> > > +static struct uvc_control *uvc_find_control(struct uvc_video_chain
> > > *chain,
> > >
> > > __u32 v4l2_id, struct uvc_control_mapping **mapping)
> > >
> > > {
> > >
> > > struct uvc_control *ctrl = NULL;
> > >
> > > @@ -944,6 +944,52 @@ done:
> > > return ret;
> > >
> > > }
> > >
> > > +/*
> > > + * Mapping V4L2 controls to UVC controls can be straighforward if done
> > > well. + * Most of the UVC controls exist in V4L2, and can be mapped
> > > directly. Some + * must be grouped (for instance the Red Balance, Blue
> > > Balance and Do White + * Balance V4L2 controls use the White Balance
> > > Component UVC control) or + * otherwise translated. The approach we take
> > > here is to use a translation + * table for the controls that can be
> > > mapped directly, and handle the others + * manually.
> > > + */
> > > +int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > > + struct v4l2_querymenu *query_menu)
> > > +{
> > > + struct uvc_menu_info *menu_info;
> > > + struct uvc_control_mapping *mapping;
> > > + struct uvc_control *ctrl;
> > > + u32 index = query_menu->index;
> > > + u32 id = query_menu->id;
> > > + int ret;
> > > +
> > > + memset(query_menu, 0, sizeof(*query_menu));
> > > + query_menu->id = id;
> > > + query_menu->index = index;
> > > +
> > > + ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> > > + if (ret < 0)
> > > + return -ERESTARTSYS;
> >
> > Just return 'ret' here (which is -EINTR).
>
> Hmmm... The uvcvideo driver uses -ERESTARTSYS extensively. What's the
> rationale behind using -EINTR instead ? Allowing users to interrupt the ioctl
> with ctrl-C ?
> If so, I wonder if it's worth it in this case, as the controls
> mutex will not be locked by another thread for an extensive period of time
> anyway.
I don't think it is worth it here. It's perfectly fine to switch to mutex_lock.
>
> ERESTARTSYS is meant to be used to deliver signals to application right away
> and then restart the system call. With EINTR applications would see an error
> in response to a VIDIOC_QUERYMENU call if SIGALRM arrives at the same time. I
> don't think that's something we want.
Good point. I never realized that. I will have to modify my patch series for
that. BTW, a quick scan in the kernel sources once again shows random behavior
regarding EINTR vs ERESTARTSYS.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] uvcvideo: Lock controls mutex when querying menus
Hi Hans,
Thanks for the comment.
On Sunday 21 November 2010 22:18:41 Hans Verkuil wrote:
> On Sunday, November 21, 2010 21:32:49 Laurent Pinchart wrote:
> > uvc_find_control() must be called with the controls mutex locked. Fix
> > uvc_query_v4l2_menu() accordingly.
> >
> > Signed-off-by: Laurent Pinchart
> > ---
> >
> > drivers/media/video/uvc/uvc_ctrl.c | 48
> > +++- drivers/media/video/uvc/uvc_v4l2.c
> > | 36 +-- drivers/media/video/uvc/uvcvideo.h |
> > 4 +-
> > 3 files changed, 50 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/media/video/uvc/uvc_ctrl.c
> > b/drivers/media/video/uvc/uvc_ctrl.c index f169f77..59f8a9a 100644
> > --- a/drivers/media/video/uvc/uvc_ctrl.c
> > +++ b/drivers/media/video/uvc/uvc_ctrl.c
> > @@ -785,7 +785,7 @@ static void __uvc_find_control(struct uvc_entity
> > *entity, __u32 v4l2_id,
> >
> > }
> >
> > }
> >
> > -struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> > +static struct uvc_control *uvc_find_control(struct uvc_video_chain
> > *chain,
> >
> > __u32 v4l2_id, struct uvc_control_mapping **mapping)
> >
> > {
> >
> > struct uvc_control *ctrl = NULL;
> >
> > @@ -944,6 +944,52 @@ done:
> > return ret;
> >
> > }
> >
> > +/*
> > + * Mapping V4L2 controls to UVC controls can be straighforward if done
> > well. + * Most of the UVC controls exist in V4L2, and can be mapped
> > directly. Some + * must be grouped (for instance the Red Balance, Blue
> > Balance and Do White + * Balance V4L2 controls use the White Balance
> > Component UVC control) or + * otherwise translated. The approach we take
> > here is to use a translation + * table for the controls that can be
> > mapped directly, and handle the others + * manually.
> > + */
> > +int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > + struct v4l2_querymenu *query_menu)
> > +{
> > + struct uvc_menu_info *menu_info;
> > + struct uvc_control_mapping *mapping;
> > + struct uvc_control *ctrl;
> > + u32 index = query_menu->index;
> > + u32 id = query_menu->id;
> > + int ret;
> > +
> > + memset(query_menu, 0, sizeof(*query_menu));
> > + query_menu->id = id;
> > + query_menu->index = index;
> > +
> > + ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> > + if (ret < 0)
> > + return -ERESTARTSYS;
>
> Just return 'ret' here (which is -EINTR).
Hmmm... The uvcvideo driver uses -ERESTARTSYS extensively. What's the
rationale behind using -EINTR instead ? Allowing users to interrupt the ioctl
with ctrl-C ? If so, I wonder if it's worth it in this case, as the controls
mutex will not be locked by another thread for an extensive period of time
anyway.
ERESTARTSYS is meant to be used to deliver signals to application right away
and then restart the system call. With EINTR applications would see an error
in response to a VIDIOC_QUERYMENU call if SIGALRM arrives at the same time. I
don't think that's something we want.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] uvcvideo: Lock controls mutex when querying menus
Just one comment:
On Sunday, November 21, 2010 21:32:49 Laurent Pinchart wrote:
> uvc_find_control() must be called with the controls mutex locked. Fix
> uvc_query_v4l2_menu() accordingly.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/video/uvc/uvc_ctrl.c | 48
> +++-
> drivers/media/video/uvc/uvc_v4l2.c | 36 +--
> drivers/media/video/uvc/uvcvideo.h |4 +-
> 3 files changed, 50 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/media/video/uvc/uvc_ctrl.c
> b/drivers/media/video/uvc/uvc_ctrl.c
> index f169f77..59f8a9a 100644
> --- a/drivers/media/video/uvc/uvc_ctrl.c
> +++ b/drivers/media/video/uvc/uvc_ctrl.c
> @@ -785,7 +785,7 @@ static void __uvc_find_control(struct uvc_entity *entity,
> __u32 v4l2_id,
> }
> }
>
> -struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> +static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> __u32 v4l2_id, struct uvc_control_mapping **mapping)
> {
> struct uvc_control *ctrl = NULL;
> @@ -944,6 +944,52 @@ done:
> return ret;
> }
>
> +/*
> + * Mapping V4L2 controls to UVC controls can be straighforward if done well.
> + * Most of the UVC controls exist in V4L2, and can be mapped directly. Some
> + * must be grouped (for instance the Red Balance, Blue Balance and Do White
> + * Balance V4L2 controls use the White Balance Component UVC control) or
> + * otherwise translated. The approach we take here is to use a translation
> + * table for the controls that can be mapped directly, and handle the others
> + * manually.
> + */
> +int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> + struct v4l2_querymenu *query_menu)
> +{
> + struct uvc_menu_info *menu_info;
> + struct uvc_control_mapping *mapping;
> + struct uvc_control *ctrl;
> + u32 index = query_menu->index;
> + u32 id = query_menu->id;
> + int ret;
> +
> + memset(query_menu, 0, sizeof(*query_menu));
> + query_menu->id = id;
> + query_menu->index = index;
> +
> + ret = mutex_lock_interruptible(&chain->ctrl_mutex);
> + if (ret < 0)
> + return -ERESTARTSYS;
Just return 'ret' here (which is -EINTR).
> +
> + ctrl = uvc_find_control(chain, query_menu->id, &mapping);
> + if (ctrl == NULL || mapping->v4l2_type != V4L2_CTRL_TYPE_MENU) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + if (query_menu->index >= mapping->menu_count) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + menu_info = &mapping->menu_info[query_menu->index];
> + strlcpy(query_menu->name, menu_info->name, sizeof query_menu->name);
> +
> +done:
> + mutex_unlock(&chain->ctrl_mutex);
> + return ret;
> +}
> +
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
