Re: [PATCH v5 3/7] media: videobuf2: Expose helpers to implement the _ext_fmt and _ext_buf hooks
Hi Tomasz,
Thank you for your comments,
On 12/14/20 5:52 AM, Tomasz Figa wrote:
> Hi Helen,
>
> On Tue, Aug 04, 2020 at 04:29:35PM -0300, Helen Koike wrote:
>> The VB2 layer is used by a lot of drivers. Patch it to support the
>> _EXT_PIX_FMT and _EXT_BUF ioctls in order to ease conversion of existing
>> drivers to these new APIs.
>>
>> Note that internally, the VB2 core is now only using ext structs and old
>> APIs are supported through conversion wrappers.
>
> We decided to only support V4L2_BUF_TYPE_VIDEO* for the ext structs. Still,
> existing drivers may use vb2 with the other, legacy, buf types. How would
> they be handled with this change?
I completly refactored this patch in my wip branch, I'll submit for comments
soon after finishing addressing the other comments.
The way I'm approaching this is to support both structures v4l2_buffer and
v4l2_ext_buffer, but only in the vb2 entry points, since all the
information we need is in vb2_buffer struct.
To implement this I had to use new hooks in the framework. I think it is easier
if you take a look in next version when I submited it.
>
>>
>> Signed-off-by: Boris Brezillon
>> Signed-off-by: Helen Koike
>> ---
>> Changes in v5:
>> - Update with new format and buffer structs
>> - Updated commit message with the uAPI prefix
>>
>> Changes in v4:
>> - Update with new format and buffer structs
>> - Fix some bugs caught by v4l2-compliance
>> - Rebased on top of media/master (post 5.8-rc1)
>>
>> Changes in v3:
>> - Rebased on top of media/master (post 5.4-rc1)
>>
>> Changes in v2:
>> - New patch
>> ---
>> .../media/common/videobuf2/videobuf2-core.c | 2 +
>> .../media/common/videobuf2/videobuf2-v4l2.c | 560 ++
>> include/media/videobuf2-core.h| 6 +-
>> include/media/videobuf2-v4l2.h| 21 +-
>> 4 files changed, 345 insertions(+), 244 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index f544d3393e9d6..d719b1e9c148b 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1270,6 +1270,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>> vb->planes[plane].length = 0;
>> vb->planes[plane].m.fd = 0;
>> vb->planes[plane].data_offset = 0;
>> +vb->planes[plane].dbuf_offset = 0;
>>
>> /* Acquire each plane's memory */
>> mem_priv = call_ptr_memop(vb, attach_dmabuf,
>> @@ -1313,6 +1314,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>> vb->planes[plane].length = planes[plane].length;
>> vb->planes[plane].m.fd = planes[plane].m.fd;
>> vb->planes[plane].data_offset = planes[plane].data_offset;
>> +vb->planes[plane].dbuf_offset = planes[plane].dbuf_offset;
>> }
>>
>> if (reacquired) {
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 30caad27281e1..911681d24b3ae 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -29,6 +29,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>>
>> @@ -56,72 +57,39 @@ module_param(debug, int, 0644);
>> V4L2_BUF_FLAG_TIMECODE | \
>> V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>>
>> -/*
>> - * __verify_planes_array() - verify that the planes array passed in struct
>> - * v4l2_buffer from userspace can be safely used
>> - */
>> -static int __verify_planes_array(struct vb2_buffer *vb, const struct
>> v4l2_buffer *b)
>> -{
>> -if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
>> -return 0;
>> -
>> -/* Is memory for copying plane information present? */
>> -if (b->m.planes == NULL) {
>> -dprintk(vb->vb2_queue, 1,
>> -"multi-planar buffer passed but planes array not
>> provided\n");
>> -return -EINVAL;
>> -}
>> -
>> -if (b->length < vb->num_planes || b->length > VB2_MAX_PLANES) {
>> -dprintk(vb->vb2_queue, 1,
>> -"incorrect planes array length, expected %d, got %d\n",
>> -vb->num_planes, b->length);
>> -return -EINVAL;
>> -}
>> -
>> -return 0;
>> -}
>> -
>> static int __verify_planes_array_core(struct vb2_buffer *vb, const void *pb)
>> {
>> -return __verify_planes_array(vb, pb);
>> +return 0;
>> }
>>
>> /*
>> * __verify_length() - Verify that the bytesused value for each plane fits
>> in
>> * the plane length and that the data offset doesn't exceed the bytesused
>> value.
>> */
>> -static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer
>> *b)
>> +static int __verify_length(struct vb2_buffer *vb,
>> + const struct v4l2_ext_buffer *b)
>>
Re: [PATCH v5 3/7] media: videobuf2: Expose helpers to implement the _ext_fmt and _ext_buf hooks
Hi Helen,
On Tue, Aug 04, 2020 at 04:29:35PM -0300, Helen Koike wrote:
> The VB2 layer is used by a lot of drivers. Patch it to support the
> _EXT_PIX_FMT and _EXT_BUF ioctls in order to ease conversion of existing
> drivers to these new APIs.
>
> Note that internally, the VB2 core is now only using ext structs and old
> APIs are supported through conversion wrappers.
We decided to only support V4L2_BUF_TYPE_VIDEO* for the ext structs. Still,
existing drivers may use vb2 with the other, legacy, buf types. How would
they be handled with this change?
>
> Signed-off-by: Boris Brezillon
> Signed-off-by: Helen Koike
> ---
> Changes in v5:
> - Update with new format and buffer structs
> - Updated commit message with the uAPI prefix
>
> Changes in v4:
> - Update with new format and buffer structs
> - Fix some bugs caught by v4l2-compliance
> - Rebased on top of media/master (post 5.8-rc1)
>
> Changes in v3:
> - Rebased on top of media/master (post 5.4-rc1)
>
> Changes in v2:
> - New patch
> ---
> .../media/common/videobuf2/videobuf2-core.c | 2 +
> .../media/common/videobuf2/videobuf2-v4l2.c | 560 ++
> include/media/videobuf2-core.h| 6 +-
> include/media/videobuf2-v4l2.h| 21 +-
> 4 files changed, 345 insertions(+), 244 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index f544d3393e9d6..d719b1e9c148b 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1270,6 +1270,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> vb->planes[plane].length = 0;
> vb->planes[plane].m.fd = 0;
> vb->planes[plane].data_offset = 0;
> + vb->planes[plane].dbuf_offset = 0;
>
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(vb, attach_dmabuf,
> @@ -1313,6 +1314,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> vb->planes[plane].length = planes[plane].length;
> vb->planes[plane].m.fd = planes[plane].m.fd;
> vb->planes[plane].data_offset = planes[plane].data_offset;
> + vb->planes[plane].dbuf_offset = planes[plane].dbuf_offset;
> }
>
> if (reacquired) {
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 30caad27281e1..911681d24b3ae 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -56,72 +57,39 @@ module_param(debug, int, 0644);
>V4L2_BUF_FLAG_TIMECODE | \
>V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
>
> -/*
> - * __verify_planes_array() - verify that the planes array passed in struct
> - * v4l2_buffer from userspace can be safely used
> - */
> -static int __verify_planes_array(struct vb2_buffer *vb, const struct
> v4l2_buffer *b)
> -{
> - if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
> - return 0;
> -
> - /* Is memory for copying plane information present? */
> - if (b->m.planes == NULL) {
> - dprintk(vb->vb2_queue, 1,
> - "multi-planar buffer passed but planes array not
> provided\n");
> - return -EINVAL;
> - }
> -
> - if (b->length < vb->num_planes || b->length > VB2_MAX_PLANES) {
> - dprintk(vb->vb2_queue, 1,
> - "incorrect planes array length, expected %d, got %d\n",
> - vb->num_planes, b->length);
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int __verify_planes_array_core(struct vb2_buffer *vb, const void *pb)
> {
> - return __verify_planes_array(vb, pb);
> + return 0;
> }
>
> /*
> * __verify_length() - Verify that the bytesused value for each plane fits in
> * the plane length and that the data offset doesn't exceed the bytesused
> value.
> */
> -static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer
> *b)
> +static int __verify_length(struct vb2_buffer *vb,
> +const struct v4l2_ext_buffer *b)
> {
> unsigned int length;
> unsigned int bytesused;
> - unsigned int plane;
> + unsigned int i;
>
> if (V4L2_TYPE_IS_CAPTURE(b->type))
> return 0;
>
> - if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> - for (plane = 0; plane < vb->num_planes; ++plane) {
> - length = (b->memory == VB2_MEMORY_USERPTR ||
> - b->memory == VB2_MEMORY_DMABUF)
> -? b->m.planes[plane].length
> - : vb->planes[plane].length;
> - bytesused = b->m.planes[plane].bytesused
> -
[PATCH v5 3/7] media: videobuf2: Expose helpers to implement the _ext_fmt and _ext_buf hooks
The VB2 layer is used by a lot of drivers. Patch it to support the
_EXT_PIX_FMT and _EXT_BUF ioctls in order to ease conversion of existing
drivers to these new APIs.
Note that internally, the VB2 core is now only using ext structs and old
APIs are supported through conversion wrappers.
Signed-off-by: Boris Brezillon
Signed-off-by: Helen Koike
---
Changes in v5:
- Update with new format and buffer structs
- Updated commit message with the uAPI prefix
Changes in v4:
- Update with new format and buffer structs
- Fix some bugs caught by v4l2-compliance
- Rebased on top of media/master (post 5.8-rc1)
Changes in v3:
- Rebased on top of media/master (post 5.4-rc1)
Changes in v2:
- New patch
---
.../media/common/videobuf2/videobuf2-core.c | 2 +
.../media/common/videobuf2/videobuf2-v4l2.c | 560 ++
include/media/videobuf2-core.h| 6 +-
include/media/videobuf2-v4l2.h| 21 +-
4 files changed, 345 insertions(+), 244 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index f544d3393e9d6..d719b1e9c148b 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1270,6 +1270,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
vb->planes[plane].length = 0;
vb->planes[plane].m.fd = 0;
vb->planes[plane].data_offset = 0;
+ vb->planes[plane].dbuf_offset = 0;
/* Acquire each plane's memory */
mem_priv = call_ptr_memop(vb, attach_dmabuf,
@@ -1313,6 +1314,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
vb->planes[plane].length = planes[plane].length;
vb->planes[plane].m.fd = planes[plane].m.fd;
vb->planes[plane].data_offset = planes[plane].data_offset;
+ vb->planes[plane].dbuf_offset = planes[plane].dbuf_offset;
}
if (reacquired) {
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 30caad27281e1..911681d24b3ae 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
@@ -56,72 +57,39 @@ module_param(debug, int, 0644);
V4L2_BUF_FLAG_TIMECODE | \
V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
-/*
- * __verify_planes_array() - verify that the planes array passed in struct
- * v4l2_buffer from userspace can be safely used
- */
-static int __verify_planes_array(struct vb2_buffer *vb, const struct
v4l2_buffer *b)
-{
- if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
- return 0;
-
- /* Is memory for copying plane information present? */
- if (b->m.planes == NULL) {
- dprintk(vb->vb2_queue, 1,
- "multi-planar buffer passed but planes array not
provided\n");
- return -EINVAL;
- }
-
- if (b->length < vb->num_planes || b->length > VB2_MAX_PLANES) {
- dprintk(vb->vb2_queue, 1,
- "incorrect planes array length, expected %d, got %d\n",
- vb->num_planes, b->length);
- return -EINVAL;
- }
-
- return 0;
-}
-
static int __verify_planes_array_core(struct vb2_buffer *vb, const void *pb)
{
- return __verify_planes_array(vb, pb);
+ return 0;
}
/*
* __verify_length() - Verify that the bytesused value for each plane fits in
* the plane length and that the data offset doesn't exceed the bytesused
value.
*/
-static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+static int __verify_length(struct vb2_buffer *vb,
+ const struct v4l2_ext_buffer *b)
{
unsigned int length;
unsigned int bytesused;
- unsigned int plane;
+ unsigned int i;
if (V4L2_TYPE_IS_CAPTURE(b->type))
return 0;
- if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
- for (plane = 0; plane < vb->num_planes; ++plane) {
- length = (b->memory == VB2_MEMORY_USERPTR ||
- b->memory == VB2_MEMORY_DMABUF)
- ? b->m.planes[plane].length
- : vb->planes[plane].length;
- bytesused = b->m.planes[plane].bytesused
- ? b->m.planes[plane].bytesused : length;
-
- if (b->m.planes[plane].bytesused > length)
- return -EINVAL;
-
- if (b->m.planes[plane].data_offset > 0 &&
- b->m.planes[plane].data_offset >= bytesused)
- return -EINVAL;
- }
- } else {
-
