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
[PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management
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, struct v4l2_create_buffers) +#define VIDIOC_DESTROY_BUFS_IOWR('V', 93, struct v4l2_buffer_span) +#define VIDIOC_SUBMIT_BUF