Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tue, Jun 07, 2011 at 02:14:06PM +0200, Guennadi Liakhovetski wrote:
On Mon, 6 Jun 2011, Sakari Ailus wrote:
Hi Guennadi,
On Mon, Jun 06, 2011 at 03:10:54PM +0200, Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Sakari Ailus wrote:
Hi Guennadi and Laurent,
Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
What about making it possible to pass an array of buffer indices
to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this
would be
perfect, but it would avoid the problem of requiring continuous
ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its
length
would be count.
I don't understand this. We do _not_ want to allow holes in
indices. For
now we decide to not implement DESTROY at all. In this case
indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse
order
- without holes and in the same ranges, as buffers have been
CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need
something
else after 6 months.
Ok, so, we pass an array from user-space with CREATE of size count.
The
kernel fills it with as many buffers entries as it has allocated. But
currently drivers are also allowed to allocate more buffers, than the
user-space has requested. What do we do in such a case?
That's a good point.
But even if there was no array, shouldn't the user be allowed to create
the buffers using a number of separate CREATE_BUF calls? The result
would be still the same n buffers as with a single call allocating the n
buffers at once.
Also, consider the (hopefully!) forthcoming DMA buffer management API
patches. It looks like that those buffers will be referred to by file
handles. To associate several DMA buffer objects to V4L2 buffers at
once, there would have to be an array of those objects.
URL:http://www.spinics.net/lists/linux-media/msg32448.html
So, does this mean now, that we have to wait for those APIs to become
solid before or even implemented we proceed with this one?
No. But I think we should take into account the foreseeable future. Any
which form the buffer id passing mechanism will take, it will very likely
involve referring to individual memory buffers the ids of which are not
contiguous ranges in a general case. In short, my point is that CREATE_BUF
should allow associating generic buffer ids to V4L2 buffers.
Ok, so, first point is: yes, it can well happen, that video buffers will
use some further buffer allocator as a back-end, that each videobuf will
possibly reference more than one of those memory buffers, and that those
memory buffers will have arbitrary IDs. AFAIC, this so far doesn't affect
our design of the CREATE ioctl(), right? As you say, both indices are
unrelated.
Both indices are unrelated. However, consider a case where the user wants to
allocate two video buffers with three planes on each. This would quite
possibly involve six different generic buffer object ids. How would buffers
like this be created using the current proposal, assuming the hardware
requires at least two of them?
I can imagine, that in the future, when we implement DESTROY, videobuf
indices can become sparse. Say, if then we have indices 3 to 5 allocated,
and the user requests 4 buffers. We can either use indices 0-2 and 6, or
6-9. Personally, I would go with the latter option. Maybe we'll have to
increase or completely eliminate the VIDEO_MAX_FRAME. But otherwise I
I think we should specify indices returned by CREATE_BUF so that they are
not limited by VIDEO_MAX_FRAME. The actual implementation may still be that
they're limited to, say, 128, as it's likely easier that way. This makes it
extensible for the future use --- think of very high-speed cameras, for
example, where the number of required buffers may well be large.
think, this would be the best way to handle this. Which means, our CREATE
ioctl() with contiguous indics should be fine. As for DESTROY, the idea
was to only allow destroying same sets of buffers, that have been
previously allocated
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Mon, 6 Jun 2011, Sakari Ailus wrote:
Hi Guennadi,
On Mon, Jun 06, 2011 at 03:10:54PM +0200, Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Sakari Ailus wrote:
Hi Guennadi and Laurent,
Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
What about making it possible to pass an array of buffer indices to
the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would
be
perfect, but it would avoid the problem of requiring continuous
ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its
length
would be count.
I don't understand this. We do _not_ want to allow holes in indices.
For
now we decide to not implement DESTROY at all. In this case indices
just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse
order
- without holes and in the same ranges, as buffers have been CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need
something
else after 6 months.
Ok, so, we pass an array from user-space with CREATE of size count. The
kernel fills it with as many buffers entries as it has allocated. But
currently drivers are also allowed to allocate more buffers, than the
user-space has requested. What do we do in such a case?
That's a good point.
But even if there was no array, shouldn't the user be allowed to create
the buffers using a number of separate CREATE_BUF calls? The result
would be still the same n buffers as with a single call allocating the n
buffers at once.
Also, consider the (hopefully!) forthcoming DMA buffer management API
patches. It looks like that those buffers will be referred to by file
handles. To associate several DMA buffer objects to V4L2 buffers at
once, there would have to be an array of those objects.
URL:http://www.spinics.net/lists/linux-media/msg32448.html
So, does this mean now, that we have to wait for those APIs to become
solid before or even implemented we proceed with this one?
No. But I think we should take into account the foreseeable future. Any
which form the buffer id passing mechanism will take, it will very likely
involve referring to individual memory buffers the ids of which are not
contiguous ranges in a general case. In short, my point is that CREATE_BUF
should allow associating generic buffer ids to V4L2 buffers.
Ok, so, first point is: yes, it can well happen, that video buffers will
use some further buffer allocator as a back-end, that each videobuf will
possibly reference more than one of those memory buffers, and that those
memory buffers will have arbitrary IDs. AFAIC, this so far doesn't affect
our design of the CREATE ioctl(), right? As you say, both indices are
unrelated.
I can imagine, that in the future, when we implement DESTROY, videobuf
indices can become sparse. Say, if then we have indices 3 to 5 allocated,
and the user requests 4 buffers. We can either use indices 0-2 and 6, or
6-9. Personally, I would go with the latter option. Maybe we'll have to
increase or completely eliminate the VIDEO_MAX_FRAME. But otherwise I
think, this would be the best way to handle this. Which means, our CREATE
ioctl() with contiguous indics should be fine. As for DESTROY, the idea
was to only allow destroying same sets of buffers, that have been
previously allocated with one CREATE, i.e., it will also only take
contiguous index sets. Am I still missing anything?
If the hardware requires more than one buffer to operate, STREAMON could
return ERANGE in a case there ane not enough queued, for example.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Wed, 18 May 2011, Sakari Ailus wrote:
Hi Guennadi and Laurent,
Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
What about making it possible to pass an array of buffer indices to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its length
would be count.
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order
- without holes and in the same ranges, as buffers have been CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need something
else after 6 months.
Ok, so, we pass an array from user-space with CREATE of size count. The
kernel fills it with as many buffers entries as it has allocated. But
currently drivers are also allowed to allocate more buffers, than the
user-space has requested. What do we do in such a case?
That's a good point.
But even if there was no array, shouldn't the user be allowed to create
the buffers using a number of separate CREATE_BUF calls? The result
would be still the same n buffers as with a single call allocating the n
buffers at once.
Also, consider the (hopefully!) forthcoming DMA buffer management API
patches. It looks like that those buffers will be referred to by file
handles. To associate several DMA buffer objects to V4L2 buffers at
once, there would have to be an array of those objects.
URL:http://www.spinics.net/lists/linux-media/msg32448.html
So, does this mean now, that we have to wait for those APIs to become
solid before or even implemented we proceed with this one?
Thanks
Guennadi
(See the links, too!)
Thus, I would think that CREATE_BUF can be used to create buffers but
not to enforce how many of them are required by a device on a single
CREATE_BUF call.
I don't have a good answer for the stated problem, but these ones
crossed my mind:
- Have a new ioctl to tell the minimum number of buffers to make
streaming possible.
- Add a field for the minimum number of buffers to CREATE_BUF.
- Use the old REQBUFS to tell the number. It didn't do much other work
in the past either, right?
Regards,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Guennadi,
On Mon, Jun 06, 2011 at 03:10:54PM +0200, Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Sakari Ailus wrote:
Hi Guennadi and Laurent,
Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
What about making it possible to pass an array of buffer indices to
the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its length
would be count.
I don't understand this. We do _not_ want to allow holes in indices.
For
now we decide to not implement DESTROY at all. In this case indices
just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse
order
- without holes and in the same ranges, as buffers have been CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need
something
else after 6 months.
Ok, so, we pass an array from user-space with CREATE of size count. The
kernel fills it with as many buffers entries as it has allocated. But
currently drivers are also allowed to allocate more buffers, than the
user-space has requested. What do we do in such a case?
That's a good point.
But even if there was no array, shouldn't the user be allowed to create
the buffers using a number of separate CREATE_BUF calls? The result
would be still the same n buffers as with a single call allocating the n
buffers at once.
Also, consider the (hopefully!) forthcoming DMA buffer management API
patches. It looks like that those buffers will be referred to by file
handles. To associate several DMA buffer objects to V4L2 buffers at
once, there would have to be an array of those objects.
URL:http://www.spinics.net/lists/linux-media/msg32448.html
So, does this mean now, that we have to wait for those APIs to become
solid before or even implemented we proceed with this one?
No. But I think we should take into account the foreseeable future. Any
which form the buffer id passing mechanism will take, it will very likely
involve referring to individual memory buffers the ids of which are not
contiguous ranges in a general case. In short, my point is that CREATE_BUF
should allow associating generic buffer ids to V4L2 buffers.
If the hardware requires more than one buffer to operate, STREAMON could
return ERANGE in a case there ane not enough queued, for example.
Regards,
--
Sakari Ailus
sakari.ai...@iki.fi
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tue, 17 May 2011, Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order -
without holes and in the same ranges, as buffers have been CREATEd before.
So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can. But
even with single index, it's always possible to issue the ioctl more
than once and achieve the same result as if there was an array of indices.
What would be the reason to disallow creating holes to index range? I
don't see much reason from application or implementation point of view,
as we're even being limited to such low numbers.
I think, there are a few locations in V4L2, that assume, that for N number
of buffers currently allocated, their indices are 0...N-1. Just look for
loops like
for (buffer = 0; buffer q-num_buffers; ++buffer) {
in videobuf2-core.c.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Guennadi Liakhovetski wrote:
On Tue, 17 May 2011, Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order -
without holes and in the same ranges, as buffers have been CREATEd before.
So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can. But
even with single index, it's always possible to issue the ioctl more
than once and achieve the same result as if there was an array of indices.
What would be the reason to disallow creating holes to index range? I
don't see much reason from application or implementation point of view,
as we're even being limited to such low numbers.
I think, there are a few locations in V4L2, that assume, that for N number
of buffers currently allocated, their indices are 0...N-1. Just look for
loops like
for (buffer = 0; buffer q-num_buffers; ++buffer) {
in videobuf2-core.c.
This code is in implementation of videobuf2, it's not the spec. We're
designing a new interface here and its behaviour musn't be restrained by
the current codebase. The videobuf2 must be changed to support the new
ioctls in any case; those functions must be fixed as the support for
CREATE_BUF and other new IOCTLs is added to videobuf2.
The above loop also likely assumes that the index of the first video
buffer to be allocated is zero; this would mean that no more than one
allocation of n buffers could be made, defeating the purpose of the new
interface.
Regards,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Friday 13 May 2011 09:45:51 Guennadi Liakhovetski wrote:
I've found some more time to get back to this. Let me try to recap, what
has been discussed. I've looked through all replies again (thanks to
all!), so, I'll present a summary. Any mistakes and misinterpretations are
mine;) If I misunderstand someone or forget anything - please, shout!
On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+ include/linux/videodev2.h
| 24 include/media/v4l2-ioctl.h|
3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c index 7c26947..d71b289
100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
int cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been
created */ +__u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
Shouldn't it be NO_CACHE_CLEAN ?
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
during a running operation. Shall these flags be added to values, taken by
struct v4l2_buffer::flags, since that is the struct, that will be used as
the argument for the new version of the SUBMIT ioctl()?
Do you have a
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
Hi Sakari
Hi Guennadi,
[clip]
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+__u32 index; /* output: buffers
index...index + count - 1 have
been created */ +__u32 count;
+__u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE(1 0)
1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
Should this be called FLAG_NO_CACHE_CLEAN?
Invalidate == Make contents of the cache invalid
Clean == Make sure no dirty stuff resides in the cache
and mark it clean?...
Flush == invalidate + clean
Maybe you meant first clean, then invalidate?
In principle, I think, yes, CLEAN would define it more strictly.
Yes; I'd prefer that. :-)
It occurred to me to wonder if two flags are needed for this, but I
think the answer is yes, since there can be memory-to-memory devices
which are both OUTPUT and CAPTURE.
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
during a running operation. Shall these flags be added to values, taken
by struct v4l2_buffer::flags, since that is the struct, that will be
used as the argument for the new version of the SUBMIT ioctl()?
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+__u32 index; /* output: buffers
index...index + count - 1
have
been created */ +__u32 count;
+__u32 flags; /* V4L2_BUFFER_FLAG_* */
+enum v4l2_memorymemory;
+__u32 size; /* Explicit size, e.g.,
for compressed streams
*/
+struct v4l2_format format; /* type is used
always, the rest
if
size == 0 */ +};
1. Care must be taken to keep index = V4L2_MAX_FRAME
This will make allocating new ranges of buffers impossible if the
existing buffer indices are scattered within the range.
What about making it possible to pass an array of buffer indices to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its length
would be count.
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order
- without holes and in the same ranges, as buffers have been CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need something
else after 6 months.
But even with single index, it's always possible to issue the ioctl more
than once and achieve the same result as if there was an array of indices.
What would be the reason to disallow creating holes to index range? I
don't see much reason from application or implementation point of view,
as we're even being limited to such low numbers.
Speaking of which; perhaps I'm bringing this up rather late, but should
we define the API to allow larger numbers than VIDEO_MAX_FRAME? 32 isn't
all that much after all --- this might become a limiting factor later on
when there are devices with huge amounts of memory.
Allowing CREATE_BUF to do that right now would be possible since
applications using it are new users and can be expected to be using it
properly. :-)
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
What about making it possible to pass an array of buffer indices to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its length
would be count.
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order
- without holes and in the same ranges, as buffers have been CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need something
else after 6 months.
Ok, so, we pass an array from user-space with CREATE of size count. The
kernel fills it with as many buffers entries as it has allocated. But
currently drivers are also allowed to allocate more buffers, than the
user-space has requested. What do we do in such a case?
Thanks
Guennadi
But even with single index, it's always possible to issue the ioctl more
than once and achieve the same result as if there was an array of indices.
What would be the reason to disallow creating holes to index range? I
don't see much reason from application or implementation point of view,
as we're even being limited to such low numbers.
Speaking of which; perhaps I'm bringing this up rather late, but should
we define the API to allow larger numbers than VIDEO_MAX_FRAME? 32 isn't
all that much after all --- this might become a limiting factor later on
when there are devices with huge amounts of memory.
Allowing CREATE_BUF to do that right now would be possible since
applications using it are new users and can be expected to be using it
properly. :-)
--
Regards,
Laurent Pinchart
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Friday 13 May 2011 09:45:51 Guennadi Liakhovetski wrote:
[snip]
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
during a running operation. Shall these flags be added to values, taken by
struct v4l2_buffer::flags, since that is the struct, that will be used as
the argument for the new version of the SUBMIT ioctl()?
Do you have a use case for per-submit cache handling ?
This was Sakari's idea, I think, ask him;) But I think, he suggested a
case, when not all buffers have to be processed in the user-space. In
principle, I can imagine such a case too. E.g., if most of the buffers you
pass directly to output / further processing, and only some of them you
analyse in software, perhaps, for some WB, exposure, etc.
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have
been
created */ + __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if
size
== 0 */ +};
1. Care must be taken to keep index = V4L2_MAX_FRAME
Does that requirement still make sense ?
Don't know, again, not my idea. videobuf2-core still uses it. At one
location it seems to be pretty arbitrary, at another it is the size of an
array in struct vb2_fileio_data, but maybe we could allocate that one
dynamically too. So, do I understand it right, that this would affect our
case, if we would CREATE our buffers and then the user would decide to do
read() / write() on them?
2. A reserved field is needed.
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
This has become the hottest point for discussion.
1. VIDIOC_CREATE_BUFS: should the REQBUFS and CREATE/DESTROY APIs be
allowed to be mixed? REQBUFS is compulsory, CREATE/DESTROY will be
optional. But shall applications be allowed to mix them? No consensus has
been riched. This will also depend on whether DESTROY will be implemented
at all (see below).
Would mixing them help in any known use case ? The API and implementation
would be cleaner if we didn't allow mixing them.
It would help us avoid designing non-mature APIs and still have the
functionality, we need;)
2. VIDIOC_DESTROY_BUFS: has been discussed a lot
(a) shall it be allowed to create holes in indices? agreement was: not at
this stage, but in the future this might be needed.
(b) ioctl() argument: shall it take a span or an array of indices? I don't
think arrays make any sense here: on CREATE you always get contiguous
index sequences, and you are only allowed to DESTROY the same index sets.
(c) shall it be implemented at all, now that we don't know, how to handle
holes, or shall we just continue using REQBUFS(0) or close() to release
all buffers at once? Not implementing DESTROY now has the disadvantage,
that if you allocate 2 buffer sets of sizes A (small) and B (big), and
then don't need B any more, but instead need C != B (big), you cannot do
this. But this is just one of hypothetical use-cases. No consensus
reached.
We could go with (c) as a first step, but it might be temporary only. I feel
a
bit uneasy implementing an API that we know will be temporary.
It shouldn't be temporary. CREATE and PREPARE should stay. It's just
DESTROY that we're not certain about yet.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi,
Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Friday 13 May 2011 09:45:51 Guennadi Liakhovetski wrote:
[snip]
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
during a running operation. Shall these flags be added to values, taken by
struct v4l2_buffer::flags, since that is the struct, that will be used as
the argument for the new version of the SUBMIT ioctl()?
Do you have a use case for per-submit cache handling ?
This was Sakari's idea, I think, ask him;) But I think, he suggested a
case, when not all buffers have to be processed in the user-space. In
principle, I can imagine such a case too. E.g., if most of the buffers you
pass directly to output / further processing, and only some of them you
analyse in software, perhaps, for some WB, exposure, etc.
Yes; I think I mentioned this. It's possible that some rather
CPU-intensive processing is only done on the CPU on every n:th image,
for example. In this case flushing the cache on images that won't be
touched by the CPU is not necessary.
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have
been
created */ + __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if
size
== 0 */ +};
1. Care must be taken to keep index = V4L2_MAX_FRAME
Does that requirement still make sense ?
Don't know, again, not my idea. videobuf2-core still uses it. At one
location it seems to be pretty arbitrary, at another it is the size of an
array in struct vb2_fileio_data, but maybe we could allocate that one
dynamically too. So, do I understand it right, that this would affect our
case, if we would CREATE our buffers and then the user would decide to do
read() / write() on them?
My issue with this number, as I gave it a little more thought, is that
it is actually quite low. The devices will have more memory in the
future and 32 might become a real limitation. I think it would be wise
to define the API so that the number of simultaneous buffers allocated
on a device is not limited by this number.
Kind regards,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Guennadi and Laurent,
Guennadi Liakhovetski wrote:
On Wed, 18 May 2011, Laurent Pinchart wrote:
On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
Guennadi Liakhovetski wrote:
[snip]
What about making it possible to pass an array of buffer indices to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its length
would be count.
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order
- without holes and in the same ranges, as buffers have been CREATEd
before. So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can.
I second that. I don't like rushing new APIs to find out we need something
else after 6 months.
Ok, so, we pass an array from user-space with CREATE of size count. The
kernel fills it with as many buffers entries as it has allocated. But
currently drivers are also allowed to allocate more buffers, than the
user-space has requested. What do we do in such a case?
That's a good point.
But even if there was no array, shouldn't the user be allowed to create
the buffers using a number of separate CREATE_BUF calls? The result
would be still the same n buffers as with a single call allocating the n
buffers at once.
Also, consider the (hopefully!) forthcoming DMA buffer management API
patches. It looks like that those buffers will be referred to by file
handles. To associate several DMA buffer objects to V4L2 buffers at
once, there would have to be an array of those objects.
URL:http://www.spinics.net/lists/linux-media/msg32448.html
(See the links, too!)
Thus, I would think that CREATE_BUF can be used to create buffers but
not to enforce how many of them are required by a device on a single
CREATE_BUF call.
I don't have a good answer for the stated problem, but these ones
crossed my mind:
- Have a new ioctl to tell the minimum number of buffers to make
streaming possible.
- Add a field for the minimum number of buffers to CREATE_BUF.
- Use the old REQBUFS to tell the number. It didn't do much other work
in the past either, right?
Regards,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Guennadi,
Thanks for the patch!
Guennadi Liakhovetski wrote:
I've found some more time to get back to this. Let me try to recap, what
has been discussed. I've looked through all replies again (thanks to
all!), so, I'll present a summary. Any mistakes and misinterpretations are
mine;) If I misunderstand someone or forget anything - please, shout!
On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int
cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+case VIDIOC_CREATE_BUFS:
+case VIDIOC_DESTROY_BUFS:
+case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+[_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+[_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+[_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+case VIDIOC_CREATE_BUFS:
+{
+struct v4l2_create_buffers *create = arg;
+
+if (!ops-vidioc_create_bufs)
+break;
+ret = check_fmt(ops, create-format.type);
+if (ret)
+break;
+
+if (create-size)
+CLEAR_AFTER_FIELD(create, count);
+
+ret = ops-vidioc_create_bufs(file, fh, create);
+
+dbgarg(cmd, count=%d\n, create-count);
+break;
+}
+case VIDIOC_DESTROY_BUFS:
+{
+struct v4l2_buffer_span *span = arg;
+
+if (!ops-vidioc_destroy_bufs)
+break;
+
+ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+dbgarg(cmd, count=%d, span-count);
+break;
+}
+case VIDIOC_SUBMIT_BUF:
+{
+unsigned int *i = arg;
+
+if (!ops-vidioc_submit_buf)
+break;
+ret = ops-vidioc_submit_buf(file, fh, *i);
+dbgarg(cmd, index=%d, *i);
+break;
+}
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+__u32 index; /* output: buffers index...index +
count - 1 have been created */
+__u32 count;
+__u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE(1 0)
1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
Should this be called FLAG_NO_CACHE_CLEAN?
Invalidate == Make contents of the cache invalid
Clean == Make sure no dirty stuff resides in the cache
Flush == invalidate + clean
It occurred to me to wonder if two flags are needed for this, but I
think the answer is yes, since there can be memory-to-memory devices
which are both OUTPUT and CAPTURE.
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Sakari
On Mon, 16 May 2011, Sakari Ailus wrote:
Hi Guennadi,
Thanks for the patch!
Guennadi Liakhovetski wrote:
I've found some more time to get back to this. Let me try to recap, what
has been discussed. I've looked through all replies again (thanks to
all!), so, I'll present a summary. Any mistakes and misinterpretations are
mine;) If I misunderstand someone or forget anything - please, shout!
On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
int cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
Should this be called FLAG_NO_CACHE_CLEAN?
Invalidate == Make contents of the cache invalid
Clean == Make sure no dirty stuff resides in the cache
and mark it clean?...
Flush == invalidate + clean
Maybe you meant first clean, then invalidate?
In principle, I think, yes, CLEAN would define it more strictly.
It occurred to me to wonder if two flags are needed for this, but I
think the answer is yes, since there can be memory-to-memory devices
which are both OUTPUT and CAPTURE.
2.
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Guennadi Liakhovetski wrote:
Hi Sakari
Hi Guennadi,
[clip]
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
Should this be called FLAG_NO_CACHE_CLEAN?
Invalidate == Make contents of the cache invalid
Clean == Make sure no dirty stuff resides in the cache
and mark it clean?...
Flush == invalidate + clean
Maybe you meant first clean, then invalidate?
In principle, I think, yes, CLEAN would define it more strictly.
Yes; I'd prefer that. :-)
It occurred to me to wonder if two flags are needed for this, but I
think the answer is yes, since there can be memory-to-memory devices
which are both OUTPUT and CAPTURE.
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
during a running operation. Shall these flags be added to values, taken by
struct v4l2_buffer::flags, since that is the struct, that will be used as
the argument for the new version of the SUBMIT ioctl()?
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been created */
+ __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if size == 0 */
+};
1. Care must be taken to keep index = V4L2_MAX_FRAME
This will make allocating new ranges of buffers impossible if the
existing buffer indices are scattered within the range.
What about making it possible to pass an array of buffer indices to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.
struct v4l2_create_buffers {
__u32 *index;
__u32 count;
__u32 flags;
enum v4l2_memorymemory;
__u32 size;
struct v4l2_format format;
};
Index would be a pointer to an array of buffer indices and its length
would be count.
I don't understand this. We do _not_ want to allow holes in indices. For
now we decide to not implement DESTROY at all. In this case indices just
increment contiguously.
The next stage is to implement DESTROY, but only in strict reverse order -
without holes and in the same ranges, as buffers have been CREATEd before.
So, I really don't understand why we need arrays, sorry.
Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can. But
even with single index, it's always possible to issue the ioctl more
than once and achieve the same result as if there was an array of indices.
What would be the reason to disallow creating holes to index range? I
don't see much reason from application or implementation point of view,
as we're even being limited to such low numbers.
Speaking of which; perhaps I'm bringing this up rather late, but should
we define the API to allow larger numbers than VIDEO_MAX_FRAME? 32 isn't
all that much after all --- this might become a limiting factor later on
when there are devices with huge amounts of memory.
Allowing CREATE_BUF to do that right now would be possible since
applications using it are new users and can be expected to be using it
properly. :-)
Regards,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Friday, May 13, 2011 09:45:51 Guennadi Liakhovetski wrote:
I've found some more time to get back to this. Let me try to recap, what
has been discussed. I've looked through all replies again (thanks to
all!), so, I'll present a summary. Any mistakes and misinterpretations are
mine;) If I misunderstand someone or forget anything - please, shout!
On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
int cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
2. Both these flags should not be passed with CREATE, but with SUBMIT
(which will be renamed to PREPARE or something similar). It should be
possible to prepare the same buffer with different cacheing attributes
during a running operation. Shall these flags be added to values, taken by
struct v4l2_buffer::flags, since that is the struct, that will be used as
the argument for the new version of the SUBMIT ioctl()?
Yes. You may want to give these flags to QBUF as well, so they belong in
v4l2_buffer.
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Guennadi,
Thanks for the RFC! I have a few comments below.
Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int
cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been created */
+ __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if size == 0 */
+};
This assumes that the buffer indices that are returned by these ioctls
will be incremented beyond V4L2_MAX_FRAME. Don't you think this is an issue?
Proper handling of this still requires that once the count reaches
UINT_MAX, you do check that the buffer indices actually are available.
This might not be easier than keeping the range as contiguous as
possible and returning a set of
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Hans,
Hans Verkuil wrote:
Hi Hans,
On Thursday 07 April 2011 09:50:13 Hans Verkuil wrote:
On Thu, 7 Apr 2011, Hans Verkuil wrote:
[snip]
Regarding DESTROY_BUFS: perhaps we should just skip this for now and
wait
for the first use-case. That way we don't need to care about holes. I
don't like artificial restrictions like 'no holes'. If someone has a
good
use-case for selectively destroying buffers, then we need to look at
this
again.
Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
close()?
Yes.
I don't really like that as it would mix CREATE and REQBUFS calls.
Applications should either use the old API (REQBUFS) or the new one, but
not
mix both.
That's a completely unnecessary limitation. And from the point of view of
vb2 it shouldn't even matter.
If calls to {CREATE,DESTROY}_BUF and REQBUFS are allowed to be mixed, it
would be nice to define the API so that one could implement REQBUFS
using CREATE_BUFS and DESTROY_BUFS. Then, drivers would not need to
implement REQBUFS separately which would still be used by majority of
applications. And Videobuf2 wouldn't need to implement REQBUFS at all.
Would this require more than to require buffer indices starting from
zero and be contiguous when there are no existing allocations?
The spec says under VIDIOC_QBUF that Valid index numbers range from
zero to the number of buffers allocated with VIDIOC_REQBUFS (struct
v4l2_requestbuffers count) minus one.
The fact that freeing arbitrary spans of buffers gives us uneasy feelings
might be a sign that the CREATE/DESTROY API is not mature enough. I'd
rather
try to solve the issue now instead of postponing it for later and discover
that our CREATE API should have been different.
What gives me an uneasy feeling is prohibiting freeing arbitrary spans of
buffers. I rather choose not to implement the DESTROY ioctl instead of
implementing a limited version of it, also because we do not have proper
use cases yet. But I have no problems with the CREATE/DESTROY API as such.
What would you think about using an array of index numbers rather than a
range for both? One must manage index number allocation even when using
a range and it might not be easier than to allocate all buffers from a
relatively small range (e.g. index numbers from 0 to 63), whose
implementation could be a bit field.
Cheers,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#defineVIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #defineVIDIOC_UNSUBSCRIBE_EVENT
_IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS_IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.
As I said, I didn't like this very much, because it involves redundant
data, but if we want to call .buf_prepare() from it, then we need
v4l2_buffer...
I don't see a problem with this. Applications already *have* the v4l2_buffer
after all. It's not as if they have to fill that structure just for this call.
Furthermore, you need all that data anyway because you need to do the same
checks that vb2_qbuf does.
Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait for
the first use-case. That way we don't need to care about holes. I don't like
artificial restrictions like 'no holes'. If someone has a good use-case for
selectively destroying buffers, then we need to look at this again.
Regards,
Hans
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Thu, 7 Apr 2011, Hans Verkuil wrote:
On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT
_IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF
do.
As I said, I didn't like this very much, because it involves redundant
data, but if we want to call .buf_prepare() from it, then we need
v4l2_buffer...
I don't see a problem with this. Applications already *have* the v4l2_buffer
after all. It's not as if they have to fill that structure just for this call.
Furthermore, you need all that data anyway because you need to do the same
checks that vb2_qbuf does.
Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait for
the first use-case. That way we don't need to care about holes. I don't like
artificial restrictions like 'no holes'. If someone has a good use-case for
selectively destroying buffers, then we need to look at this again.
Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
close()?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Thu, 7 Apr 2011, Hans Verkuil wrote:
On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT
_IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct
v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF_IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like
QBUF/DQBUF do.
As I said, I didn't like this very much, because it involves redundant
data, but if we want to call .buf_prepare() from it, then we need
v4l2_buffer...
I don't see a problem with this. Applications already *have* the
v4l2_buffer
after all. It's not as if they have to fill that structure just for this
call.
Furthermore, you need all that data anyway because you need to do the
same
checks that vb2_qbuf does.
Regarding DESTROY_BUFS: perhaps we should just skip this for now and
wait for
the first use-case. That way we don't need to care about holes. I don't
like
artificial restrictions like 'no holes'. If someone has a good use-case
for
selectively destroying buffers, then we need to look at this again.
Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
close()?
Yes.
Hans
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Thu, 7 Apr 2011, Hans Verkuil wrote:
On Thu, 7 Apr 2011, Hans Verkuil wrote:
On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
*I O C T L C O D E S F O R V I D E O D E V I C E
S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT
_IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS_IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct
v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like
QBUF/DQBUF do.
As I said, I didn't like this very much, because it involves redundant
data, but if we want to call .buf_prepare() from it, then we need
v4l2_buffer...
I don't see a problem with this. Applications already *have* the
v4l2_buffer
after all. It's not as if they have to fill that structure just for this
call.
Furthermore, you need all that data anyway because you need to do the
same
checks that vb2_qbuf does.
Regarding DESTROY_BUFS: perhaps we should just skip this for now and
wait for
the first use-case. That way we don't need to care about holes. I don't
like
artificial restrictions like 'no holes'. If someone has a good use-case
for
selectively destroying buffers, then we need to look at this again.
Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
close()?
Yes.
Ok, how about this: I remove the ioctl definition and struct
v4l2_ioctl_ops callback for it, but I keep the span struct and the vb2
implementation - in case we need it later? The vb2_destroy_bufs() will be
used to implement freeing the queue as a particular case of freeing some
buffers.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Thursday, April 07, 2011 10:53:32 Guennadi Liakhovetski wrote:
On Thu, 7 Apr 2011, Hans Verkuil wrote:
On Thu, 7 Apr 2011, Hans Verkuil wrote:
On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
* I O C T L C O D E S F O R V I D E O D E V I C E
S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT
_IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct
v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF_IOW('V', 94, int)
+
In case we later need to pass other information (such as flags)
to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like
QBUF/DQBUF do.
As I said, I didn't like this very much, because it involves
redundant
data, but if we want to call .buf_prepare() from it, then we need
v4l2_buffer...
I don't see a problem with this. Applications already *have* the
v4l2_buffer
after all. It's not as if they have to fill that structure just for
this
call.
Furthermore, you need all that data anyway because you need to do the
same
checks that vb2_qbuf does.
Regarding DESTROY_BUFS: perhaps we should just skip this for now and
wait for
the first use-case. That way we don't need to care about holes. I don't
like
artificial restrictions like 'no holes'. If someone has a good use-case
for
selectively destroying buffers, then we need to look at this again.
Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
close()?
Yes.
Ok, how about this: I remove the ioctl definition and struct
v4l2_ioctl_ops callback for it, but I keep the span struct and the vb2
implementation - in case we need it later? The vb2_destroy_bufs() will be
used to implement freeing the queue as a particular case of freeing some
buffers.
I wouldn't do that. Just keep the code clean and the patch as small as
possible. Having unused/unnecessary code is bad practice.
Regards,
Hans
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Hans, On Thursday 07 April 2011 09:50:13 Hans Verkuil wrote: On Thu, 7 Apr 2011, Hans Verkuil wrote: [snip] Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait for the first use-case. That way we don't need to care about holes. I don't like artificial restrictions like 'no holes'. If someone has a good use-case for selectively destroying buffers, then we need to look at this again. Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) / close()? Yes. I don't really like that as it would mix CREATE and REQBUFS calls. Applications should either use the old API (REQBUFS) or the new one, but not mix both. The fact that freeing arbitrary spans of buffers gives us uneasy feelings might be a sign that the CREATE/DESTROY API is not mature enough. I'd rather try to solve the issue now instead of postponing it for later and discover that our CREATE API should have been different. -- 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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Hans, On Thursday 07 April 2011 09:50:13 Hans Verkuil wrote: On Thu, 7 Apr 2011, Hans Verkuil wrote: [snip] Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait for the first use-case. That way we don't need to care about holes. I don't like artificial restrictions like 'no holes'. If someone has a good use-case for selectively destroying buffers, then we need to look at this again. Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) / close()? Yes. I don't really like that as it would mix CREATE and REQBUFS calls. Applications should either use the old API (REQBUFS) or the new one, but not mix both. That's a completely unnecessary limitation. And from the point of view of vb2 it shouldn't even matter. The fact that freeing arbitrary spans of buffers gives us uneasy feelings might be a sign that the CREATE/DESTROY API is not mature enough. I'd rather try to solve the issue now instead of postponing it for later and discover that our CREATE API should have been different. What gives me an uneasy feeling is prohibiting freeing arbitrary spans of buffers. I rather choose not to implement the DESTROY ioctl instead of implementing a limited version of it, also because we do not have proper use cases yet. But I have no problems with the CREATE/DESTROY API as such. Regards, Hans -- 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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.
As I said, I didn't like this very much, because it involves redundant
data, but if we want to call .buf_prepare() from it, then we need
v4l2_buffer...
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Guennadi,
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
[snip]
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
[snip]
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
Why only when create-size is 0 ?
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been
created */ + __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
Shouldn't cache management be handled at submit/qbuf time instead of being a
buffer property ?
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been
created */ + __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if size
==
0 */ +};
You need reserved fields here.
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
Just throwing an idea in here, what about using the same structure for both
ioctls ? Or even a single ioctl for both create and destroy, like we do with
REQBUFS ?
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
/* Reminder: when adding new ioctls please add support for them to
drivers/media/video/v4l2-compat-ioctl32.c as well! */
[snip]
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Monday 04 April 2011 10:06:47 Hans Verkuil wrote: On Mon, 4 Apr 2011, Hans Verkuil wrote: On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote: [snip] BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS is compulsory, while CREATE/DESTROY are optional. Drivers must support REQBUFS and should support CREATE/DESTROY, but I think applications should not be allowed to mix calls. -- 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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+ include/linux/videodev2.h |
24 include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
int cmd, unsigned long arg) case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been
created */ + __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been
created */ + __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if size
==
0 */ +};
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+ include/linux/videodev2.h |
24 include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
int cmd, unsigned long arg) case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count
- 1 have been
created */ +__u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index +
count - 1 have been
created */ +__u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if
size
==
0 */ +};
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#defineVIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #defineVIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92,
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Laurent
On Tue, 5 Apr 2011, Laurent Pinchart wrote:
Hi Guennadi,
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
[snip]
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
[snip]
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
Why only when create-size is 0 ?
Because otherwise create-format contains the frame format, that will be
used for plane-size calculations.
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been
created */ +__u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
Shouldn't cache management be handled at submit/qbuf time instead of being a
buffer property ?
hmm, I'd prefer fixing it at create. Or do you want to be able to create
buffers and then submit / queue them with different flags?...
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been
created */ +__u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
+ struct v4l2_format format; /* type is used always, the
rest if size
==
0 */ +};
You need reserved fields here.
Yes, already discussed with Hans, will add.
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#defineVIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #defineVIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS_IOWR('V', 93, struct v4l2_buffer_span)
Just throwing an idea in here, what about using the same structure for both
ioctls ? Or even a single ioctl for both create and destroy, like we do with
REQBUFS ?
Personally, tbh, I don't like either of them. The first one seems an
overkill - you don't need all those fields for destroy. The second one is
a particular case of the first one, plus it adds confusion by re-using the
ioctl:-) Where with REQBUFS we could just set count = 0 to say - release
all buffers, with this one we need index and count, so, we'd need one more
flag to distinguish between create / destroy...
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
/* Reminder: when adding new ioctls please add support for them to
drivers/media/video/v4l2-compat-ioctl32.c as well! */
[snip]
--
Regards,
Laurent Pinchart
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tue, 5 Apr 2011, Laurent Pinchart wrote: On Monday 04 April 2011 10:06:47 Hans Verkuil wrote: On Mon, 4 Apr 2011, Hans Verkuil wrote: On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote: [snip] BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS is compulsory, while CREATE/DESTROY are optional. Drivers must support REQBUFS and should support CREATE/DESTROY, but I think applications should not be allowed to mix calls. So, you want to track it down in the kernel which mode is being used?... Hans? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tuesday, April 05, 2011 14:02:17 Laurent Pinchart wrote: On Monday 04 April 2011 10:06:47 Hans Verkuil wrote: On Mon, 4 Apr 2011, Hans Verkuil wrote: On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote: [snip] BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS is compulsory, while CREATE/DESTROY are optional. Drivers must support REQBUFS and should support CREATE/DESTROY, but I think applications should not be allowed to mix calls. Why not? The videobuf2-core.c implementation shouldn't care about that, so I don't see why userspace should care. Regards, Hans -- 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 -- 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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tuesday 05 April 2011 14:34:57 Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF
do.
v4l2_buffer has a single reserved field (we could reclaim the input field,
it's not used by any in-tree driver). Shouldn't we a bit more future-proof ?
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tuesday 05 April 2011 14:40:16 Hans Verkuil wrote: On Tuesday, April 05, 2011 14:02:17 Laurent Pinchart wrote: On Monday 04 April 2011 10:06:47 Hans Verkuil wrote: On Mon, 4 Apr 2011, Hans Verkuil wrote: On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote: [snip] BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS is compulsory, while CREATE/DESTROY are optional. Drivers must support REQBUFS and should support CREATE/DESTROY, but I think applications should not be allowed to mix calls. Why not? The videobuf2-core.c implementation shouldn't care about that, so I don't see why userspace should care. Because it makes the API semantics much more complex to define. We would have to precisely define interactions between REQBUFS and CREATE/DESTROY. That will be error-prone, for very little benefits (if any at all). If an application is aware of the CREATE/DESTROY ioctls and wants to use them, why would it need to use REQBUFS ? -- 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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF _IOW('V', 94, int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.
Why??? You do not need all that extra information. Well, we could, of
course, make a struct with reserved fields... but it seems nice to me to
have the clarity here - this ioctl() _only_ gives buffer ownership to the
kernel. No more configuration...
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Guennadi,
On Tuesday 05 April 2011 14:39:19 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
[snip]
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
[snip]
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
Shouldn't cache management be handled at submit/qbuf time instead of
being a buffer property ?
hmm, I'd prefer fixing it at create. Or do you want to be able to create
buffers and then submit / queue them with different flags?...
That's the idea, yes. I'm not sure yet how useful that would be though.
[snip]
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
Just throwing an idea in here, what about using the same structure for
both ioctls ? Or even a single ioctl for both create and destroy, like
we do with REQBUFS ?
Personally, tbh, I don't like either of them. The first one seems an
overkill - you don't need all those fields for destroy. The second one is
a particular case of the first one, plus it adds confusion by re-using the
ioctl:-) Where with REQBUFS we could just set count = 0 to say - release
all buffers, with this one we need index and count, so, we'd need one more
flag to distinguish between create / destroy...
OK, idea dismissed :-)
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Tuesday 05 April 2011 14:52:20 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Hans Verkuil wrote:
On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
[snip]
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#defineVIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription) #defineVIDIOC_UNSUBSCRIBE_EVENT
_IOW('V',
91, struct v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers) +#define VIDIOC_DESTROY_BUFS _IOWR('V', 93,
struct v4l2_buffer_span) +#define VIDIOC_SUBMIT_BUF _IOW('V', 94,
int)
+
In case we later need to pass other information (such as flags) to
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF
do.
Why??? You do not need all that extra information. Well, we could, of
course, make a struct with reserved fields... but it seems nice to me to
have the clarity here - this ioctl() _only_ gives buffer ownership to the
kernel. No more configuration...
Right now you're correct. In the future we might need extra fields, so we need
a structure with reserved fields.
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Laurent Pinchart wrote:
Hi Guennadi,
Hi all,
On Tuesday 05 April 2011 14:39:19 Guennadi Liakhovetski wrote:
On Tue, 5 Apr 2011, Laurent Pinchart wrote:
On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
[snip]
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
[snip]
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
Shouldn't cache management be handled at submit/qbuf time instead of
being a buffer property ?
hmm, I'd prefer fixing it at create. Or do you want to be able to create
buffers and then submit / queue them with different flags?...
That's the idea, yes. I'm not sure yet how useful that would be though.
I agree that it'd be nice to support this. It depends on where the data
is being used.
For example, you could have an algorithm on the host side which does
process the image data but only uses every second frame, with no other
processing done on the host CPU. In this case the cache would be flushed
needlessly for the frames that are not touched by the CPU.
I admit that this is fine optimisation but I don't think that should be
ruled out either.
The default cache behaviour would still be to flush not to break
existing applications, so I don't think this would be a significant
burden for applications.
Cheers,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
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/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int
cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
We also need a FLAG_NO_CACHE_FLUSH. This is for output devices.
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been created */
+ __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?
+ struct v4l2_format format; /* type is used always, the
rest if size == 0 */
Needs some reserved fields as well.
+};
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription)
#define
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Hans
Thanks for the review
On Mon, 4 Apr 2011, Hans Verkuil wrote:
On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
int cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
We also need a FLAG_NO_CACHE_FLUSH. This is for output devices.
Ok.
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have been created */
+ __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?
Not sure. As the comment says, this is mainly for bitstream formats. For
any pixel-based format you really should just fill in the format below. If
you allow the user to specify planes, then you also need to know
alignment, contiguity, padding, etc.
+ struct v4l2_format format; /*
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
Hi Hans
Thanks for the review
On Mon, 4 Apr 2011, Hans Verkuil wrote:
On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.
Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de
---
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
drivers/media/video/v4l2-ioctl.c | 43
+
include/linux/videodev2.h | 24
include/media/v4l2-ioctl.h|3 ++
4 files changed, 73 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file,
unsigned int cmd, unsigned long arg)
case VIDIOC_DQEVENT:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
+ case VIDIOC_CREATE_BUFS:
+ case VIDIOC_DESTROY_BUFS:
+ case VIDIOC_SUBMIT_BUF:
ret = do_video_ioctl(file, cmd, arg);
break;
diff --git a/drivers/media/video/v4l2-ioctl.c
b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DQEVENT)] = VIDIOC_DQEVENT,
[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = VIDIOC_SUBSCRIBE_EVENT,
[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = VIDIOC_UNSUBSCRIBE_EVENT,
+ [_IOC_NR(VIDIOC_CREATE_BUFS)] = VIDIOC_CREATE_BUFS,
+ [_IOC_NR(VIDIOC_DESTROY_BUFS)] = VIDIOC_DESTROY_BUFS,
+ [_IOC_NR(VIDIOC_SUBMIT_BUF)] = VIDIOC_SUBMIT_BUF,
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, type=0x%8.8x, sub-type);
break;
}
+ case VIDIOC_CREATE_BUFS:
+ {
+ struct v4l2_create_buffers *create = arg;
+
+ if (!ops-vidioc_create_bufs)
+ break;
+ ret = check_fmt(ops, create-format.type);
+ if (ret)
+ break;
+
+ if (create-size)
+ CLEAR_AFTER_FIELD(create, count);
+
+ ret = ops-vidioc_create_bufs(file, fh, create);
+
+ dbgarg(cmd, count=%d\n, create-count);
+ break;
+ }
+ case VIDIOC_DESTROY_BUFS:
+ {
+ struct v4l2_buffer_span *span = arg;
+
+ if (!ops-vidioc_destroy_bufs)
+ break;
+
+ ret = ops-vidioc_destroy_bufs(file, fh, span);
+
+ dbgarg(cmd, count=%d, span-count);
+ break;
+ }
+ case VIDIOC_SUBMIT_BUF:
+ {
+ unsigned int *i = arg;
+
+ if (!ops-vidioc_submit_buf)
+ break;
+ ret = ops-vidioc_submit_buf(file, fh, *i);
+ dbgarg(cmd, index=%d, *i);
+ break;
+ }
default:
{
bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
__u32 revision;/* chip revision, chip specific */
} __attribute__ ((packed));
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+ __u32 index; /* output: buffers index...index +
count - 1 have
been created */
+ __u32 count;
+ __u32 reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE (1 0)
We also need a FLAG_NO_CACHE_FLUSH. This is for output devices.
Ok.
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+ __u32 index; /* output: buffers
index...index + count - 1 have
been created */
+ __u32 count;
+ __u32 flags; /* V4L2_BUFFER_FLAG_* */
+ enum v4l2_memorymemory;
+ __u32 size; /* Explicit size, e.g., for
compressed streams */
Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?
Not sure. As the comment says, this is mainly for bitstream formats. For
any pixel-based format you really should just fill in the format below. If
you allow the user to specify planes, then you also need to know
alignment, contiguity, padding, etc.
+ struct v4l2_format format; /* type is used always, the
rest if
size == 0 */
Needs some reserved
Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
On Mon, 4 Apr 2011, Hans Verkuil wrote:
[snip]
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+__u32 index; /* output: buffers
index...index + count - 1 have
been created */
+__u32 count;
+__u32 flags; /* V4L2_BUFFER_FLAG_* */
+enum v4l2_memorymemory;
+__u32 size; /* Explicit size, e.g.,
for compressed streams */
Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?
Not sure. As the comment says, this is mainly for bitstream formats. For
any pixel-based format you really should just fill in the format below. If
you allow the user to specify planes, then you also need to know
alignment, contiguity, padding, etc.
+struct v4l2_format format; /* type is used
always, the rest if
size == 0 */
Needs some reserved fields as well.
v4l2_format is a union with 200 bytes, is this not enough?
You can't add new fields to the v4l2_pix_format struct in this union,
because that would mess up the G/S_FBUF ioctls. Really a V4L2 design flaw.
So it's better to add some extra reserveds at the top level.
Can do that, sure, but just for understanding: as long as we're sure the
other union members don't exceed (a bit fewer than) 200 bytes, why
cannot we change 200 to 196 in the union and add a reserver u32 to all
affected ioctl()s - if / when needed?
+};
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
#define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct
v4l2_event_subscription)
#define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct
v4l2_event_subscription)
+#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct
v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS _IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF_IOW('V', 94, int)
I don't really like the name. I think I'd go for _PRE_QBUF or _PREP_BUF
or
something like that.
I didn't want to use any form of prepare, because we already have a
.buf_prepare() method, and IMHO it would be confusing. Pre-queue I didn't
like very much either, for the same reasons, why we rejected it at the
meeting, which was, I think, that it's not really doing anything with the
queue, right? What this ioctl() does is it passes buffer ownership from
the user-space to the kernel, right? Any good name for that? In fact
submit doesn't sound too bad to me:-)
Well, naming isn't my strongest point. But isn't buf_prepare exactly the
op that SUBMIT is supposed to call (besides pinning the buffers in
memory)? That's the whole point of this ioctl. Anyway, I'd have to hear
what others think.
I've been thinking about that too. Currently .buf_prepare() is called on
each QBUF, so, it performs a different task - a dynamic preparation,
necessary during a running capture / playback. Whereas the new ioctl()
should only perform a one-off operation - passing the ownership on the
buffer. Seems pretty different to me.
BTW, I agree with other reviewers that DESTROY_BUFS shouldn't leave any
holes.
That was our decision at the meeting - I can well remember that, I was
surprised about that too, so, I think, I even asked to clarify this point.
But we can change it of course. Shall we return -EBUSY if the user is
trying to free buffers in the middle?
I'm having second thoughts about this. If you have lots of buffers of
different sizes, then it might make sense to destroy buffers in the
middle.
But we should perhaps just disallow it for now. We can always be more
lenient later if necessary.
That would be the easiest for now. Do we all agree?
+
/* Reminder: when adding new ioctls please add support for them to
drivers/media/video/v4l2-compat-ioctl32.c as well! */
Don't forget this! VIDIOC_CREATE_BUFS will need to be handled in
compat-ioctl32.
I didn't:
drivers/media/video/v4l2-compat-ioctl32.c |3 ++
Or is that not enough? Is any special handling required? AFAICS, REQBUFS
is not treated specially either.
No, you need special handling for the 'enum memory' and the struct
v4l2_format. These have different sizes depending on the architecture.
Hm, I'll have a look at them then...
BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS
is compulsory, while CREATE/DESTROY are optional.
Sure, drivers _must_ implement REQBUFS and _may_ implement CREATE/DESTROY.
The question is - should we allow mixing of them? E.g., an app first
calling REQBUFS, then CREATE to add more buffers, and eventually
REQBUFS(0) to destroy them all?...
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line unsubscribe
