[PATCH 1/1] yavta: Set plane size for mplane buffers in qbuf

[PATCH 1/1] yavta: Set plane size for mplane buffers in qbuf

[Sakari Ailus]

The plane size was left zero for mplane buffers when queueing a buffer. Fix
this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 yavta.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/yavta.c b/yavta.c
index 7f9e814..77e5a41 100644
--- a/yavta.c
+++ b/yavta.c
@@ -979,8 +979,12 @@ static int video_queue_buffer(struct device *dev, int index, enum buffer_fill_mo
 
 	if (dev->memtype == V4L2_MEMORY_USERPTR) {
 		if (video_is_mplane(dev)) {
-			for (i = 0; i < dev->num_planes; i++)
-				buf.m.planes[i].m.userptr = (unsigned long)dev->buffers[index].mem[i];
+			for (i = 0; i < dev->num_planes; i++) {
+				buf.m.planes[i].m.userptr = (unsigned long)
+					dev->buffers[index].mem[i];
+				buf.m.planes[i].length =
+					dev->buffers[index].size[i];
+			}
 		} else {
 			buf.m.userptr = (unsigned long)dev->buffers[index].mem[0];
 		}
-- 
2.1.0.231.g7484e3b

Re: [PATCH 1/1] yavta: Set plane size for mplane buffers in qbuf

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 15 December 2014 17:27:44 Sakari Ailus wrote:
> The plane size was left zero for mplane buffers when queueing a buffer. Fix
> this.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  yavta.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/yavta.c b/yavta.c
> index 7f9e814..77e5a41 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -979,8 +979,12 @@ static int video_queue_buffer(struct device *dev, int
> index, enum buffer_fill_mo
> 
>  	if (dev->memtype == V4L2_MEMORY_USERPTR) {
>  		if (video_is_mplane(dev)) {
> -			for (i = 0; i < dev->num_planes; i++)
> -				buf.m.planes[i].m.userptr = (unsigned long)dev->
> buffers[index].mem[i];
> +			for (i = 0; i < dev->num_planes; i++) {
> +				buf.m.planes[i].m.userptr = (unsigned long)
> +					dev->buffers[index].mem[i];
> +				buf.m.planes[i].length =
> +					dev->buffers[index].size[i];

According to the V4L2 API, this field is set by the driver. That's not in line 
with the videobuf2 implementation, which requires the length field to be set 
for USERPTR and DMABUF. Would you like to submit a documentation patch ?

> +			}
>  		} else {
>  			buf.m.userptr = (unsigned long)dev->buffers[index].mem[0];

For consistency, what would you think about moving the buf.length assignment 
from the else statement right before this hunk to here ? If you're fine with 
that there's no need to resubmit, I'll apply the modification locally.

>  		}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] yavta: Set plane size for mplane buffers in qbuf

[Sakari Ailus]

Hi Laurent,

On Mon, Dec 15, 2014 at 06:31:01PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Monday 15 December 2014 17:27:44 Sakari Ailus wrote:
> > The plane size was left zero for mplane buffers when queueing a buffer. Fix
> > this.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  yavta.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/yavta.c b/yavta.c
> > index 7f9e814..77e5a41 100644
> > --- a/yavta.c
> > +++ b/yavta.c
> > @@ -979,8 +979,12 @@ static int video_queue_buffer(struct device *dev, int
> > index, enum buffer_fill_mo
> > 
> >  	if (dev->memtype == V4L2_MEMORY_USERPTR) {
> >  		if (video_is_mplane(dev)) {
> > -			for (i = 0; i < dev->num_planes; i++)
> > -				buf.m.planes[i].m.userptr = (unsigned long)dev->
> > buffers[index].mem[i];
> > +			for (i = 0; i < dev->num_planes; i++) {
> > +				buf.m.planes[i].m.userptr = (unsigned long)
> > +					dev->buffers[index].mem[i];
> > +				buf.m.planes[i].length =
> > +					dev->buffers[index].size[i];
> 
> According to the V4L2 API, this field is set by the driver. That's not in line 
> with the videobuf2 implementation, which requires the length field to be set 
> for USERPTR and DMABUF. Would you like to submit a documentation patch ?

Certainly. I actually missed that this is what the documentation states.

> > +			}
> >  		} else {
> >  			buf.m.userptr = (unsigned long)dev->buffers[index].mem[0];
> 
> For consistency, what would you think about moving the buf.length assignment 
> from the else statement right before this hunk to here ? If you're fine with 
> that there's no need to resubmit, I'll apply the modification locally.

Feel free to add that.

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk