linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GIT PULL FOR 3.6] V4L2 API cleanups
@ 2012-06-10 20:22 Sakari Ailus
  2012-06-11  7:50 ` Laurent Pinchart
  2012-07-05 20:55 ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 13+ messages in thread
From: Sakari Ailus @ 2012-06-10 20:22 UTC (permalink / raw)
  To: linux-media@vger.kernel.org

Hi Mauro,

Here are two V4L2 API cleanup patches; the first removes __user from
videodev2.h from a few places, making it possible to use the header file
as such in user space, while the second one changes the
v4l2_buffer.input field back to reserved.


The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:

  [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
09:27:24 -0300)

are available in the git repository at:
  ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6

Sakari Ailus (2):
      v4l: Remove __user from interface structure definitions
      v4l: drop v4l2_buffer.input and V4L2_BUF_FLAG_INPUT

 Documentation/DocBook/media/v4l/compat.xml      |   12 ++++++++++++
 Documentation/DocBook/media/v4l/io.xml          |   19 +++++--------------
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml |    9 +++------
 drivers/media/video/cpia2/cpia2_v4l.c           |    2 +-
 drivers/media/video/v4l2-compat-ioctl32.c       |   11 +++++------
 drivers/media/video/videobuf-core.c             |   16 ----------------
 drivers/media/video/videobuf2-core.c            |    5 ++---
 include/linux/videodev2.h                       |    9 ++++-----
 8 files changed, 32 insertions(+), 51 deletions(-)


Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-10 20:22 [GIT PULL FOR 3.6] V4L2 API cleanups Sakari Ailus
@ 2012-06-11  7:50 ` Laurent Pinchart
  2012-06-11  9:39   ` Sakari Ailus
  2012-07-05 20:55 ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2012-06-11  7:50 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org

Hi Sakari,

On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
> Hi Mauro,
> 
> Here are two V4L2 API cleanup patches; the first removes __user from
> videodev2.h from a few places, making it possible to use the header file
> as such in user space, while the second one changes the
> v4l2_buffer.input field back to reserved.
> 
> 
> The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:
> 
>   [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> 09:27:24 -0300)
> 
> are available in the git repository at:
>   ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> 
> Sakari Ailus (2):
>       v4l: Remove __user from interface structure definitions

NAK, sorry.

__user has a purpose, we need to add it where it's missing, not remove it 
where it's rightfully present.

>       v4l: drop v4l2_buffer.input and V4L2_BUF_FLAG_INPUT
> 
>  Documentation/DocBook/media/v4l/compat.xml      |   12 ++++++++++++
>  Documentation/DocBook/media/v4l/io.xml          |   19 +++++--------------
>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml |    9 +++------
>  drivers/media/video/cpia2/cpia2_v4l.c           |    2 +-
>  drivers/media/video/v4l2-compat-ioctl32.c       |   11 +++++------
>  drivers/media/video/videobuf-core.c             |   16 ----------------
>  drivers/media/video/videobuf2-core.c            |    5 ++---
>  include/linux/videodev2.h                       |    9 ++++-----
>  8 files changed, 32 insertions(+), 51 deletions(-)
> 
> 
> Kind regards,

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-11  7:50 ` Laurent Pinchart
@ 2012-06-11  9:39   ` Sakari Ailus
  2012-06-16 22:03     ` Laurent Pinchart
  2012-07-05 20:46     ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 13+ messages in thread
From: Sakari Ailus @ 2012-06-11  9:39 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media@vger.kernel.org

On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
> > Hi Mauro,
> > 
> > Here are two V4L2 API cleanup patches; the first removes __user from
> > videodev2.h from a few places, making it possible to use the header file
> > as such in user space, while the second one changes the
> > v4l2_buffer.input field back to reserved.
> > 
> > 
> > The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:
> > 
> >   [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> > 09:27:24 -0300)
> > 
> > are available in the git repository at:
> >   ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> > 
> > Sakari Ailus (2):
> >       v4l: Remove __user from interface structure definitions
> 
> NAK, sorry.
> 
> __user has a purpose, we need to add it where it's missing, not remove it 
> where it's rightfully present.

It's not quite as simple as adding __user everywhere it might belong to ---
these structs are being used in kernel space, too. The structs that are part
of the user space interface may at some point contain pointers to memory
which is in user space. That is being dealt by video_usercopy(), so the
individual drivers or the rest of the V4L2 framework always gets pointers
pointing to kernel memory.

These particular fields aren't handled by the framework currently, so
removing __user there requires adding the support to video_usercopy().

Regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-11  9:39   ` Sakari Ailus
@ 2012-06-16 22:03     ` Laurent Pinchart
  2012-06-17  7:54       ` Sakari Ailus
  2012-07-05 20:46     ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2012-06-16 22:03 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org

Hi Sakari,

On Monday 11 June 2012 12:39:44 Sakari Ailus wrote:
> On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote:
> > On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
> > > Hi Mauro,
> > > 
> > > Here are two V4L2 API cleanup patches; the first removes __user from
> > > videodev2.h from a few places, making it possible to use the header file
> > > as such in user space, while the second one changes the
> > > v4l2_buffer.input field back to reserved.
> > > 
> > > The following changes since commit 
5472d3f17845c4398c6a510b46855820920c2181:
> > >   [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> > > 
> > > 09:27:24 -0300)
> > > 
> > > are available in the git repository at:
> > >   ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> > > 
> > > Sakari Ailus (2):
> > >       v4l: Remove __user from interface structure definitions
> > 
> > NAK, sorry.
> > 
> > __user has a purpose, we need to add it where it's missing, not remove it
> > where it's rightfully present.
> 
> It's not quite as simple as adding __user everywhere it might belong to ---
> these structs are being used in kernel space, too. The structs that are part
> of the user space interface may at some point contain pointers to memory
> which is in user space. That is being dealt by video_usercopy(), so the
> individual drivers or the rest of the V4L2 framework always gets pointers
> pointing to kernel memory.

Very good point, I haven't thought about that. I'm not sure how to deal with 
this, splitting structures in a __user and a non __user version isn't really a 
good option. Maybe the sparse tool should be somehow extended ?

> These particular fields aren't handled by the framework currently, so
> removing __user there requires adding the support to video_usercopy().

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-16 22:03     ` Laurent Pinchart
@ 2012-06-17  7:54       ` Sakari Ailus
  2012-06-18 10:53         ` Laurent Pinchart
  0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2012-06-17  7:54 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media@vger.kernel.org

On Sun, Jun 17, 2012 at 12:03:06AM +0200, Laurent Pinchart wrote:
> Hi Sakari,

Hi Laurent,

> On Monday 11 June 2012 12:39:44 Sakari Ailus wrote:
> > On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote:
> > > On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
> > > > Hi Mauro,
> > > > 
> > > > Here are two V4L2 API cleanup patches; the first removes __user from
> > > > videodev2.h from a few places, making it possible to use the header file
> > > > as such in user space, while the second one changes the
> > > > v4l2_buffer.input field back to reserved.
> > > > 
> > > > The following changes since commit 
> 5472d3f17845c4398c6a510b46855820920c2181:
> > > >   [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> > > > 
> > > > 09:27:24 -0300)
> > > > 
> > > > are available in the git repository at:
> > > >   ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> > > > 
> > > > Sakari Ailus (2):
> > > >       v4l: Remove __user from interface structure definitions
> > > 
> > > NAK, sorry.
> > > 
> > > __user has a purpose, we need to add it where it's missing, not remove it
> > > where it's rightfully present.
> > 
> > It's not quite as simple as adding __user everywhere it might belong to ---
> > these structs are being used in kernel space, too. The structs that are part
> > of the user space interface may at some point contain pointers to memory
> > which is in user space. That is being dealt by video_usercopy(), so the
> > individual drivers or the rest of the V4L2 framework always gets pointers
> > pointing to kernel memory.
> 
> Very good point, I haven't thought about that. I'm not sure how to deal with 
> this, splitting structures in a __user and a non __user version isn't really a 
> good option. Maybe the sparse tool should be somehow extended ?

Wouldn't type casting in video_usercopy() just do the job? Albeit I'm far
from certain it'd make the code better, just make the sparse warnings go
away...

regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-17  7:54       ` Sakari Ailus
@ 2012-06-18 10:53         ` Laurent Pinchart
  0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2012-06-18 10:53 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org

Hi Sakari,

On Sunday 17 June 2012 10:54:32 Sakari Ailus wrote:
> On Sun, Jun 17, 2012 at 12:03:06AM +0200, Laurent Pinchart wrote:
> > On Monday 11 June 2012 12:39:44 Sakari Ailus wrote:
> > > On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote:
> > > > On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
> > > > > Hi Mauro,
> > > > > 
> > > > > Here are two V4L2 API cleanup patches; the first removes __user from
> > > > > videodev2.h from a few places, making it possible to use the header
> > > > > file
> > > > > as such in user space, while the second one changes the
> > > > > v4l2_buffer.input field back to reserved.
> > > > > 
> > > > > The following changes since commit
> > 
> > 5472d3f17845c4398c6a510b46855820920c2181:
> > > > >   [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> > > > > 
> > > > > 09:27:24 -0300)
> > > > > 
> > > > > are available in the git repository at:
> > > > >   ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> > > > > 
> > > > > Sakari Ailus (2):
> > > > >       v4l: Remove __user from interface structure definitions
> > > > 
> > > > NAK, sorry.
> > > > 
> > > > __user has a purpose, we need to add it where it's missing, not remove
> > > > it where it's rightfully present.
> > > 
> > > It's not quite as simple as adding __user everywhere it might belong to
> > > ---
> > > these structs are being used in kernel space, too. The structs that are
> > > part of the user space interface may at some point contain pointers to
> > > memory which is in user space. That is being dealt by video_usercopy(),
> > > so the individual drivers or the rest of the V4L2 framework always gets
> > > pointers pointing to kernel memory.
> > 
> > Very good point, I haven't thought about that. I'm not sure how to deal
> > with this, splitting structures in a __user and a non __user version
> > isn't really a good option. Maybe the sparse tool should be somehow
> > extended ?
> 
> Wouldn't type casting in video_usercopy() just do the job? Albeit I'm far
> from certain it'd make the code better, just make the sparse warnings go
> away...

For pointers that are completely handled in the v4l core (such as the 
v4l2_buffer and v4l2_ext_controls pointer fields), casting casting in 
video_usercopy() is enough as no driver will ever see the user pointers (we 
already cast in those cases). For pointers that are to be handled by drivers, 
I think we need to keep __user (or make the v4l core handle them).

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-11  9:39   ` Sakari Ailus
  2012-06-16 22:03     ` Laurent Pinchart
@ 2012-07-05 20:46     ` Mauro Carvalho Chehab
  2012-07-06  8:51       ` Laurent Pinchart
  1 sibling, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-07-05 20:46 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Laurent Pinchart, linux-media@vger.kernel.org

Em 11-06-2012 06:39, Sakari Ailus escreveu:
> On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
>>> Hi Mauro,
>>>
>>> Here are two V4L2 API cleanup patches; the first removes __user from
>>> videodev2.h from a few places, making it possible to use the header file
>>> as such in user space, while the second one changes the
>>> v4l2_buffer.input field back to reserved.
>>>
>>>
>>> The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:
>>>
>>>    [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
>>> 09:27:24 -0300)
>>>
>>> are available in the git repository at:
>>>    ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
>>>
>>> Sakari Ailus (2):
>>>        v4l: Remove __user from interface structure definitions
>>
>> NAK, sorry.
>>
>> __user has a purpose, we need to add it where it's missing, not remove it
>> where it's rightfully present.
> 
> It's not quite as simple as adding __user everywhere it might belong to ---
> these structs are being used in kernel space, too.

Only kernelspace see __user. The "make headers_install" target removes __user
from the userspace copy.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-06-10 20:22 [GIT PULL FOR 3.6] V4L2 API cleanups Sakari Ailus
  2012-06-11  7:50 ` Laurent Pinchart
@ 2012-07-05 20:55 ` Mauro Carvalho Chehab
  2012-07-05 21:21   ` Sakari Ailus
  1 sibling, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-07-05 20:55 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org

Em 10-06-2012 17:22, Sakari Ailus escreveu:
> Hi Mauro,
> 
> Here are two V4L2 API cleanup patches; the first removes __user from
> videodev2.h from a few places, making it possible to use the header file
> as such in user space, while the second one changes the
> v4l2_buffer.input field back to reserved.
> 
> 
> The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:
> 
>    [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> 09:27:24 -0300)
> 
> are available in the git repository at:
>    ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> 
> Sakari Ailus (2):
>        v4l: Remove __user from interface structure definitions

>        v4l: drop v4l2_buffer.input and V4L2_BUF_FLAG_INPUT

Indeed, no drivers use V4L2_BUF_FLAG_INPUT, although I think this should be
used there, for some devices.

There are several surveillance boards (mostly bttv boards, but there are
also cx88 and saa7134 models in the market) where the same chip is used 
by up to 4 cameras. What software does is to switch the video input
to sample one of those cameras on a given frequency (1/60Hz or 1/30Hz),
in order to collect the streams for the 4 cameras.

Without an input field there at the buffer metadata, it might happen that 
software would look into the wrong input.

That's said, considering that:

1) no driver is currently filling buffer queue with its "inputs" field,
   this flag is not used anywhere;

2) an implementation for input switch currently requires userspace to tell
   Kernel to switch to the next input, with is racy;

3) a model where the Kernel itself would switch to the next input would
   require some Kernelspace changes.

I agree that we can just remove this bad implementation. If latter needed,
we'll need to not only reapply this patch but also to add a better way to
allow time-sharing the same video sampler with multiple inputs.

So, I'll apply this patch.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-07-05 20:55 ` Mauro Carvalho Chehab
@ 2012-07-05 21:21   ` Sakari Ailus
  2012-07-05 21:31     ` Ezequiel Garcia
  2012-07-05 21:41     ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 13+ messages in thread
From: Sakari Ailus @ 2012-07-05 21:21 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linux-media@vger.kernel.org, elezegarcia, Hans Verkuil

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Em 10-06-2012 17:22, Sakari Ailus escreveu:
>> Hi Mauro,
>>
>> Here are two V4L2 API cleanup patches; the first removes __user from
>> videodev2.h from a few places, making it possible to use the header file
>> as such in user space, while the second one changes the
>> v4l2_buffer.input field back to reserved.
>>
>>
>> The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:
>>
>>     [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
>> 09:27:24 -0300)
>>
>> are available in the git repository at:
>>     ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
>>
>> Sakari Ailus (2):
>>         v4l: Remove __user from interface structure definitions
>
>>         v4l: drop v4l2_buffer.input and V4L2_BUF_FLAG_INPUT
>
> Indeed, no drivers use V4L2_BUF_FLAG_INPUT, although I think this should be
> used there, for some devices.
>
> There are several surveillance boards (mostly bttv boards, but there are
> also cx88 and saa7134 models in the market) where the same chip is used
> by up to 4 cameras. What software does is to switch the video input
> to sample one of those cameras on a given frequency (1/60Hz or 1/30Hz),
> in order to collect the streams for the 4 cameras.
>
> Without an input field there at the buffer metadata, it might happen that
> software would look into the wrong input.
>
> That's said, considering that:
>
> 1) no driver is currently filling buffer queue with its "inputs" field,
>     this flag is not used anywhere;
>
> 2) an implementation for input switch currently requires userspace to tell
>     Kernel to switch to the next input, with is racy;
>
> 3) a model where the Kernel itself would switch to the next input would
>     require some Kernelspace changes.
>
> I agree that we can just remove this bad implementation. If latter needed,
> we'll need to not only reapply this patch but also to add a better way to
> allow time-sharing the same video sampler with multiple inputs.
>
> So, I'll apply this patch.

Thanks!

There was a discussion between Ezequiel and Hans that in my 
understanding led to a conclusion there's no such use case, at least one 
which would be properly supported by the hardware. (Please correct me if 
I'm mistaken.)

<URL:http://www.spinics.net/lists/linux-media/msg48474.html>

So if we ever get such hardware then we could start rethinking it. :-)

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-07-05 21:21   ` Sakari Ailus
@ 2012-07-05 21:31     ` Ezequiel Garcia
  2012-07-05 22:27       ` Mauro Carvalho Chehab
  2012-07-05 21:41     ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 13+ messages in thread
From: Ezequiel Garcia @ 2012-07-05 21:31 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, linux-media@vger.kernel.org, Hans Verkuil

On Thu, Jul 5, 2012 at 6:21 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> There was a discussion between Ezequiel and Hans that in my understanding
> led to a conclusion there's no such use case, at least one which would be
> properly supported by the hardware. (Please correct me if I'm mistaken.)
>

Concerning stk1160 devices with several video input (I own one with
four video inputs),
I can say that this is currently handled throught ioctl
VIDIOC_S_INPUT. I.e, user must
explicitly select one (and only one) input.

In my very humble opinion (and assuming I understand this discussion properly)
I think that if there is no hardware support for streaming multiple
inputs at the same time,
it's not kernel job to "simulate it" and cycle through several inputs
and several buffer queues.

My two cents,
Ezequiel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-07-05 21:21   ` Sakari Ailus
  2012-07-05 21:31     ` Ezequiel Garcia
@ 2012-07-05 21:41     ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-07-05 21:41 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org, elezegarcia, Hans Verkuil

Em 05-07-2012 18:21, Sakari Ailus escreveu:
> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
>> Em 10-06-2012 17:22, Sakari Ailus escreveu:
>>> Hi Mauro,
>>>
>>> Here are two V4L2 API cleanup patches; the first removes __user from
>>> videodev2.h from a few places, making it possible to use the header file
>>> as such in user space, while the second one changes the
>>> v4l2_buffer.input field back to reserved.
>>>
>>>
>>> The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181:
>>>
>>>     [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
>>> 09:27:24 -0300)
>>>
>>> are available in the git repository at:
>>>     ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
>>>
>>> Sakari Ailus (2):
>>>         v4l: Remove __user from interface structure definitions
>>
>>>         v4l: drop v4l2_buffer.input and V4L2_BUF_FLAG_INPUT
>>
>> Indeed, no drivers use V4L2_BUF_FLAG_INPUT, although I think this should be
>> used there, for some devices.
>>
>> There are several surveillance boards (mostly bttv boards, but there are
>> also cx88 and saa7134 models in the market) where the same chip is used
>> by up to 4 cameras. What software does is to switch the video input
>> to sample one of those cameras on a given frequency (1/60Hz or 1/30Hz),
>> in order to collect the streams for the 4 cameras.
>>
>> Without an input field there at the buffer metadata, it might happen that
>> software would look into the wrong input.
>>
>> That's said, considering that:
>>
>> 1) no driver is currently filling buffer queue with its "inputs" field,
>>     this flag is not used anywhere;
>>
>> 2) an implementation for input switch currently requires userspace to tell
>>     Kernel to switch to the next input, with is racy;
>>
>> 3) a model where the Kernel itself would switch to the next input would
>>     require some Kernelspace changes.
>>
>> I agree that we can just remove this bad implementation. If latter needed,
>> we'll need to not only reapply this patch but also to add a better way to
>> allow time-sharing the same video sampler with multiple inputs.
>>
>> So, I'll apply this patch.
> 
> Thanks!
> 
> There was a discussion between Ezequiel and Hans that in my understanding led to a conclusion there's no such use case, at least one which would be properly supported by the hardware. (Please correct me if I'm mistaken.)
> 
> <URL:http://www.spinics.net/lists/linux-media/msg48474.html>
> 
> So if we ever get such hardware then we could start rethinking it. :-)

This use case exists and I've seen several embedded surveillance systems
doing the right thing there (didn't look inside the source code),
but I suspect that there's a lack of open-source applications over there
and perhaps this used to be working with V4L1 API.

Once I got one of such hardware borrowed and I noticed the issue, but I
didn't manage to get more than one camera in order to properly address it
there.

It probably makes sense to have one set of video buffers per input, and let
the Kernel to do switch the buffer per input, but doing that is not trivial
with the V4L2 API.

Another alternative would be to add an ioctl that would allow userspace to
tell what inputs should be multiplexed, and then use the current way.

Doing input switching everytime switching is bad, as the framerate per
input will reduce. Also, input switching may generate artifacts, so
drivers need to be aware of that and do the switching during the vertical
retrace time.

Anyway, let's discuss it the next time someone come up with this issue, and
have some hardware with multiple cameras per input to test it.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-07-05 21:31     ` Ezequiel Garcia
@ 2012-07-05 22:27       ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-07-05 22:27 UTC (permalink / raw)
  To: Ezequiel Garcia; +Cc: Sakari Ailus, linux-media@vger.kernel.org, Hans Verkuil

Em 05-07-2012 18:31, Ezequiel Garcia escreveu:
> On Thu, Jul 5, 2012 at 6:21 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>
>> There was a discussion between Ezequiel and Hans that in my understanding
>> led to a conclusion there's no such use case, at least one which would be
>> properly supported by the hardware. (Please correct me if I'm mistaken.)
>>
> 
> Concerning stk1160 devices with several video input (I own one with
> four video inputs),
> I can say that this is currently handled throught ioctl
> VIDIOC_S_INPUT. I.e, user must
> explicitly select one (and only one) input.
> 
> In my very humble opinion (and assuming I understand this discussion properly)
> I think that if there is no hardware support for streaming multiple
> inputs at the same time,
> it's not kernel job to "simulate it" and cycle through several inputs
> and several buffer queues.

Sorry, but I don't agree: some devices are clearly targeted to be used with
multiple inputs being cycled.

For example, this one [1]:
	http://www.geovision.com.tw/PT/Prod_GV800.asp

has only 4 BT878 chips, but each one can switch up to 4 cameras, and this
very same card is used on several commercial solutions for surveillance.

As far as I know, the input switching should be commanded externally, as
the hardware doesn't do that automatically. Even so, it has a high-speed
switch, so it should be fine to do it at vertical refresh time.

Implementing support for it using VIDIOC_S_INPUT is a very poor solution,
as an ioctl call may happen after the vertical retrace, causing artifacts.

The proper solution would be for the Kernel to switch to the next input during
the IRQ handler. So, when a frame for input 0 is received, the driver should be
switching to the next active input as soon as possible, in order to avoid
artifacts.

[1] Sorry, I were unable to discover the English version of this specific page.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GIT PULL FOR 3.6] V4L2 API cleanups
  2012-07-05 20:46     ` Mauro Carvalho Chehab
@ 2012-07-06  8:51       ` Laurent Pinchart
  0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2012-07-06  8:51 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Sakari Ailus, linux-media@vger.kernel.org

Hi Mauro,

On Thursday 05 July 2012 17:46:36 Mauro Carvalho Chehab wrote:
> Em 11-06-2012 06:39, Sakari Ailus escreveu:
> > On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote:
> >> On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:
> >>> Hi Mauro,
> >>> 
> >>> Here are two V4L2 API cleanup patches; the first removes __user from
> >>> videodev2.h from a few places, making it possible to use the header file
> >>> as such in user space, while the second one changes the
> >>> v4l2_buffer.input field back to reserved.
> >>> 
> >>> The following changes since commit 
5472d3f17845c4398c6a510b46855820920c2181:
> >>>    [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24
> >>> 
> >>> 09:27:24 -0300)
> >>> 
> >>> are available in the git repository at:
> >>>    ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6
> >>> 
> >>> Sakari Ailus (2):
> >>>        v4l: Remove __user from interface structure definitions
> >> 
> >> NAK, sorry.
> >> 
> >> __user has a purpose, we need to add it where it's missing, not remove it
> >> where it's rightfully present.
> > 
> > It's not quite as simple as adding __user everywhere it might belong to
> > ---
> > these structs are being used in kernel space, too.
> 
> Only kernelspace see __user. The "make headers_install" target removes
> __user from the userspace copy.

The issue at hand is that the same structure is used as an ioctl argument 
(where __user annotation makes sense), but also inside the kernel after 
video_usercopy, where the user pointer fields then store a kernel pointer. We 
thus can't annotate the fields with __user unconditionally.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-07-06  9:27 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-10 20:22 [GIT PULL FOR 3.6] V4L2 API cleanups Sakari Ailus
2012-06-11  7:50 ` Laurent Pinchart
2012-06-11  9:39   ` Sakari Ailus
2012-06-16 22:03     ` Laurent Pinchart
2012-06-17  7:54       ` Sakari Ailus
2012-06-18 10:53         ` Laurent Pinchart
2012-07-05 20:46     ` Mauro Carvalho Chehab
2012-07-06  8:51       ` Laurent Pinchart
2012-07-05 20:55 ` Mauro Carvalho Chehab
2012-07-05 21:21   ` Sakari Ailus
2012-07-05 21:31     ` Ezequiel Garcia
2012-07-05 22:27       ` Mauro Carvalho Chehab
2012-07-05 21:41     ` Mauro Carvalho Chehab

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).