[RFCv1,4/6] videobuf2-core: fill in length field for multiplanar buffers.

Commit Message

Hans Verkuil Sept. 19, 2012, 2:37 p.m. UTC

From: Hans Verkuil <[e-mail]>

length should be set to num_planes in __fill_v4l2_buffer(). That way the
caller knows how many planes there are in the buffer.

Signed-off-by: Hans Verkuil <[e-mail]>
---
 drivers/media/v4l2-core/videobuf2-core.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Hans Verkuil Sept. 21, 2012, 9:54 a.m. UTC | #1

On Wed September 19 2012 16:37:38 Hans Verkuil wrote:
> From: Hans Verkuil <[e-mail]>
> 
> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> caller knows how many planes there are in the buffer.

Can someone review this? I'd like to know whether it will cause problems
for existing applications to set the length back to the actual number of
planes, and whether it makes sense at all to do so. I believe it does, but
I don't know if anyone is using the current behavior.

Note that the documentation currently doesn't specify what will happen with
length.

Since the only drivers implementing multiplanar support are Samsung drivers,
I assume that Samsung will know best whether this change might cause problems.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <[e-mail]>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 929cc99..bbfe022 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		 * Fill in plane-related data if userspace provided an array
>  		 * for it. The memory and size is verified above.
>  		 */
> +		b->length = q->num_planes;
>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
>  	} else {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Sept. 21, 2012, 4:13 p.m. UTC | #2

Hi Hans,

On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <[e-mail]>
> 
> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> caller knows how many planes there are in the buffer.
> 
> Signed-off-by: Hans Verkuil <[e-mail]>

I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
Consider a use case where device is streaming with 2-planar pixel format
and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
queue there will be buffers with different number of planes. The number of 
planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
won't work.

> ---
>  drivers/media/v4l2-core/videobuf2-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 929cc99..bbfe022 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		 * Fill in plane-related data if userspace provided an array
>  		 * for it. The memory and size is verified above.
>  		 */
> +		b->length = q->num_planes;
>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
>  	} else {

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hans Verkuil Sept. 21, 2012, 4:23 p.m. UTC | #3

On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
> Hi Hans,
> 
> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <[e-mail]>
> > 
> > length should be set to num_planes in __fill_v4l2_buffer(). That way the
> > caller knows how many planes there are in the buffer.
> > 
> > Signed-off-by: Hans Verkuil <[e-mail]>
> 
> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
> Consider a use case where device is streaming with 2-planar pixel format
> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
> queue there will be buffers with different number of planes. The number of 
> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
> won't work.

That's a very good point and one I need to meditate on.

However, your comment applies to patch 1/6, not to this one.
This patch is about whether or not the length field of v4l2_buffer should
be filled in with the actual number of planes used by that buffer or not.

Regards,

	Hans

> 
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 929cc99..bbfe022 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  		 * Fill in plane-related data if userspace provided an array
> >  		 * for it. The memory and size is verified above.
> >  		 */
> > +		b->length = q->num_planes;
> >  		memcpy(b->m.planes, vb->v4l2_planes,
> >  			b->length * sizeof(struct v4l2_plane));
> >  	} else {
> 
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Sept. 21, 2012, 4:47 p.m. UTC | #4

On 09/21/2012 06:23 PM, Hans Verkuil wrote:
> On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
>> Hi Hans,
>>
>> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil <[e-mail]>
>>>
>>> length should be set to num_planes in __fill_v4l2_buffer(). That way the
>>> caller knows how many planes there are in the buffer.
>>>
>>> Signed-off-by: Hans Verkuil <[e-mail]>
>>
>> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
>> Consider a use case where device is streaming with 2-planar pixel format
>> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
>> queue there will be buffers with different number of planes. The number of 
>> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
>> won't work.
> 
> That's a very good point and one I need to meditate on.
> 
> However, your comment applies to patch 1/6, not to this one.
> This patch is about whether or not the length field of v4l2_buffer should
> be filled in with the actual number of planes used by that buffer or not.

Yes, right. Sorry, I was editing response to multiple patches from this
series and have mixed things a bit. I agree that it is logical and expected
to update struct v4l2_buffer for user space.

I have spent some time on this series, and even prepared a patch for s5p-mfc,
as it relies on num_planes being in struct vb2_buffer. But then a realized
there could be buffers with distinct number of planes an a single queue.

Regards,
Sylwester

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hans Verkuil Sept. 21, 2012, 4:54 p.m. UTC | #5

On Fri September 21 2012 18:47:54 Sylwester Nawrocki wrote:
> On 09/21/2012 06:23 PM, Hans Verkuil wrote:
> > On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
> >> Hi Hans,
> >>
> >> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> >>> From: Hans Verkuil <[e-mail]>
> >>>
> >>> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> >>> caller knows how many planes there are in the buffer.
> >>>
> >>> Signed-off-by: Hans Verkuil <[e-mail]>
> >>
> >> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
> >> Consider a use case where device is streaming with 2-planar pixel format
> >> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
> >> queue there will be buffers with different number of planes. The number of 
> >> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
> >> won't work.
> > 
> > That's a very good point and one I need to meditate on.
> > 
> > However, your comment applies to patch 1/6, not to this one.
> > This patch is about whether or not the length field of v4l2_buffer should
> > be filled in with the actual number of planes used by that buffer or not.
> 
> Yes, right. Sorry, I was editing response to multiple patches from this
> series and have mixed things a bit. I agree that it is logical and expected
> to update struct v4l2_buffer for user space.

OK, great. That's was actually the main reason for working on this as this
unexpected behavior bit me when writing mplane streaming support for v4l2-ctl.

> I have spent some time on this series, and even prepared a patch for s5p-mfc,
> as it relies on num_planes being in struct vb2_buffer. But then a realized
> there could be buffers with distinct number of planes an a single queue.

I'll get back to you about this, probably on Monday. I need to think about the
implications of this.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hans Verkuil Sept. 24, 2012, 1:52 p.m. UTC | #6

On Fri September 21 2012 18:54:12 Hans Verkuil wrote:
> On Fri September 21 2012 18:47:54 Sylwester Nawrocki wrote:
> > On 09/21/2012 06:23 PM, Hans Verkuil wrote:
> > > On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
> > >> Hi Hans,
> > >>
> > >> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > >>> From: Hans Verkuil <[e-mail]>
> > >>>
> > >>> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> > >>> caller knows how many planes there are in the buffer.
> > >>>
> > >>> Signed-off-by: Hans Verkuil <[e-mail]>
> > >>
> > >> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
> > >> Consider a use case where device is streaming with 2-planar pixel format
> > >> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
> > >> queue there will be buffers with different number of planes. The number of 
> > >> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
> > >> won't work.
> > > 
> > > That's a very good point and one I need to meditate on.
> > > 
> > > However, your comment applies to patch 1/6, not to this one.
> > > This patch is about whether or not the length field of v4l2_buffer should
> > > be filled in with the actual number of planes used by that buffer or not.
> > 
> > Yes, right. Sorry, I was editing response to multiple patches from this
> > series and have mixed things a bit. I agree that it is logical and expected
> > to update struct v4l2_buffer for user space.
> 
> OK, great. That's was actually the main reason for working on this as this
> unexpected behavior bit me when writing mplane streaming support for v4l2-ctl.
> 
> > I have spent some time on this series, and even prepared a patch for s5p-mfc,
> > as it relies on num_planes being in struct vb2_buffer. But then a realized
> > there could be buffers with distinct number of planes an a single queue.
> 
> I'll get back to you about this, probably on Monday. I need to think about the
> implications of this.

You are absolutely right about that. It makes my patch a bit more complex since
you have to be careful in VIDIOC_DQBUF not to dequeue unless the provided
v4l2_buffer struct has enough room to store the plane information.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 929cc99..bbfe022 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -348,6 +348,7 @@  static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 		 * Fill in plane-related data if userspace provided an array
 		 * for it. The memory and size is verified above.
 		 */
+		b->length = q->num_planes;
 		memcpy(b->m.planes, vb->v4l2_planes,
 			b->length * sizeof(struct v4l2_plane));
 	} else {