* [PATCH] v4l: omap3isp: Move code out of mutex-protected section
@ 2013-11-04 10:07 Laurent Pinchart
2013-11-04 11:20 ` Sakari Ailus
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2013-11-04 10:07 UTC (permalink / raw)
To: linux-media; +Cc: sakari.ailus
The pad::get_fmt call must be protected by a mutex, but preparing its
arguments doesn't need to be. Move the non-critical code out of the
mutex-protected section.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/omap3isp/ispvideo.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index a908d00..f6304bb 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, struct v4l2_format *format)
if (subdev == NULL)
return -EINVAL;
- mutex_lock(&video->mutex);
-
fmt.pad = pad;
fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
- ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
- if (ret == -ENOIOCTLCMD)
- ret = -EINVAL;
+ mutex_lock(&video->mutex);
+ ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
mutex_unlock(&video->mutex);
if (ret)
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] v4l: omap3isp: Move code out of mutex-protected section
2013-11-04 10:07 [PATCH] v4l: omap3isp: Move code out of mutex-protected section Laurent Pinchart
@ 2013-11-04 11:20 ` Sakari Ailus
2013-11-04 13:17 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Sakari Ailus @ 2013-11-04 11:20 UTC (permalink / raw)
To: Laurent Pinchart, g; +Cc: linux-media
Hi Laurent,
Thanks for the patch.
On Mon, Nov 04, 2013 at 11:07:48AM +0100, Laurent Pinchart wrote:
> The pad::get_fmt call must be protected by a mutex, but preparing its
> arguments doesn't need to be. Move the non-critical code out of the
> mutex-protected section.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/platform/omap3isp/ispvideo.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index a908d00..f6304bb 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, struct v4l2_format *format)
> if (subdev == NULL)
> return -EINVAL;
>
> - mutex_lock(&video->mutex);
> -
> fmt.pad = pad;
> fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> - if (ret == -ENOIOCTLCMD)
> - ret = -EINVAL;
By removing these lines, you're also returning -ENOIOCTLCMD to the caller.
Is this intentional?
That return value will end up to at least one place which seems to be
isp_video_streamon() and, unless I'm mistaken, will cause
ioctl(VIDIOC_STREAMON) also return ENOTTY.
> + mutex_lock(&video->mutex);
> + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> mutex_unlock(&video->mutex);
>
> if (ret)
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] v4l: omap3isp: Move code out of mutex-protected section
2013-11-04 11:20 ` Sakari Ailus
@ 2013-11-04 13:17 ` Laurent Pinchart
2013-11-04 13:21 ` Sakari Ailus
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2013-11-04 13:17 UTC (permalink / raw)
To: Sakari Ailus; +Cc: g, linux-media
Hi Sakari,
On Monday 04 November 2013 13:20:11 Sakari Ailus wrote:
> Hi Laurent,
>
> Thanks for the patch.
>
> On Mon, Nov 04, 2013 at 11:07:48AM +0100, Laurent Pinchart wrote:
> > The pad::get_fmt call must be protected by a mutex, but preparing its
> > arguments doesn't need to be. Move the non-critical code out of the
> > mutex-protected section.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/media/platform/omap3isp/ispvideo.c | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> > b/drivers/media/platform/omap3isp/ispvideo.c index a908d00..f6304bb
> > 100644
> > --- a/drivers/media/platform/omap3isp/ispvideo.c
> > +++ b/drivers/media/platform/omap3isp/ispvideo.c
> > @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video,
> > struct v4l2_format *format)>
> > if (subdev == NULL)
> >
> > return -EINVAL;
> >
> > - mutex_lock(&video->mutex);
> > -
> >
> > fmt.pad = pad;
> > fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> > - if (ret == -ENOIOCTLCMD)
> > - ret = -EINVAL;
>
> By removing these lines, you're also returning -ENOIOCTLCMD to the caller.
> Is this intentional?
>
> That return value will end up to at least one place which seems to be
> isp_video_streamon() and, unless I'm mistaken, will cause
> ioctl(VIDIOC_STREAMON) also return ENOTTY.
I should have split this in two patches, or at least explained the rationale
in the commit message. The remote subdev is always an internal ISP subdev, the
pad::get_fmt operation is thus guaranteed to be implemented. There's no need
to check for ENOIOCTLCMD.
> > + mutex_lock(&video->mutex);
> > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> >
> > mutex_unlock(&video->mutex);
> >
> > if (ret)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] v4l: omap3isp: Move code out of mutex-protected section
2013-11-04 13:17 ` Laurent Pinchart
@ 2013-11-04 13:21 ` Sakari Ailus
0 siblings, 0 replies; 4+ messages in thread
From: Sakari Ailus @ 2013-11-04 13:21 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
On Mon, Nov 04, 2013 at 02:17:53PM +0100, Laurent Pinchart wrote:
> > That return value will end up to at least one place which seems to be
> > isp_video_streamon() and, unless I'm mistaken, will cause
> > ioctl(VIDIOC_STREAMON) also return ENOTTY.
>
> I should have split this in two patches, or at least explained the rationale
> in the commit message. The remote subdev is always an internal ISP subdev, the
> pad::get_fmt operation is thus guaranteed to be implemented. There's no need
> to check for ENOIOCTLCMD.
Right; true.
If you add an explanation on that to the commit message (which I think is
much less self-evident than access to the local struct not requiring
serialisation),
Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-11-04 13:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-04 10:07 [PATCH] v4l: omap3isp: Move code out of mutex-protected section Laurent Pinchart
2013-11-04 11:20 ` Sakari Ailus
2013-11-04 13:17 ` Laurent Pinchart
2013-11-04 13:21 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).