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 <laurent.pinch...@ideasonboard.com>
> > > > > ---
> > > > >
> > > > > 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
