V4L2 API cleanups

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,

Hi Sakari,

Quoted 16 lines · 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

Quoted 2 lines

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.

Quoted 9 lines · Laurent Pinchart

Quoted 8 lines · 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

Quoted 7 lines

Quoted 7 lines

[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

Quoted 7 lines

Quoted 2 lines

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

Hi Sakari,

Quoted 10 lines · Laurent Pinchart

Quoted 9 lines · On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:

Quoted 8 lines

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:

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

[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

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

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 ?

Quoted 37 lines · Laurent Pinchart

Quoted 11 lines · On Monday 11 June 2012 12:39:44 Sakari Ailus wrote:

Quoted 10 lines · Laurent Pinchart

Quoted 9 lines · 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:

Quoted 22 lines

Quoted 7 lines

Quoted 7 lines

[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

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 22 lines

Hi Sakari,

Quoted 18 lines · 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

Quoted 4 lines

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.

Em 11-06-2012 06:39, Sakari Ailus escreveu:

Quoted 10 lines · Laurent Pinchart

Quoted 9 lines · On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote:

Quoted 8 lines

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

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

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

Quoted 3 lines

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.

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

Em 10-06-2012 17:22, Sakari Ailus escreveu:

Quoted 13 lines

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)

Quoted 7 lines

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.

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

Quoted 5 lines

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.

Hi Mauro,

Quoted 42 lines · Mauro Carvalho Chehab

Em 10-06-2012 17:22, Sakari Ailus escreveu:

Quoted 13 lines

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)

Quoted 7 lines

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;

Quoted 8 lines

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