All of lore.kernel.org
 help / color / mirror / Atom feed
* Possible issue in videobuf2 with buffer length check at QBUF time
@ 2011-08-05  8:55 Laurent Pinchart
  2011-08-05 15:01 ` Pawel Osciak
  0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2011-08-05  8:55 UTC (permalink / raw)
  To: Marek Szyprowski, Pawel Osciak; +Cc: linux-media

Hi Marek and Pawel,

While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify 
the buffer length field value when a new USERPTR buffer is queued.

The length given by userspace is copied to the internal buffer length field. 
It seems to be up to drivers to verify that the value is equal to or higher 
than the minimum required to store the image, but that's not clearly 
documented. Should the buf_init operation be made mandatory for drivers that 
support USERPTR, and documented as required to implement a length check ?

Alternatively the check could be performed in videobuf2-core, which would 
probably make sense as the check is required. videobuf2 doesn't store the 
minimum size for USERPTR buffers though (but that can of course be changed).

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-05  8:55 Possible issue in videobuf2 with buffer length check at QBUF time Laurent Pinchart
@ 2011-08-05 15:01 ` Pawel Osciak
  2011-08-05 16:14   ` Pawel Osciak
  2011-08-08 10:11   ` Laurent Pinchart
  0 siblings, 2 replies; 8+ messages in thread
From: Pawel Osciak @ 2011-08-05 15:01 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Marek Szyprowski, linux-media

Hi Laurent,

On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Marek and Pawel,
>
> While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify
> the buffer length field value when a new USERPTR buffer is queued.
>

That's a good catch. We should definitely do something about it.

> The length given by userspace is copied to the internal buffer length field.
> It seems to be up to drivers to verify that the value is equal to or higher
> than the minimum required to store the image, but that's not clearly
> documented. Should the buf_init operation be made mandatory for drivers that
> support USERPTR, and documented as required to implement a length check ?
>

Technically, drivers can do the length checks on buf_prepare if they
don't allow format changes after REQBUFS. On the other hand though, if
a driver supports USERPTR, the buffers queued from userspace have to
be verified on qbuf and the only place to do that would be buf_init.
So every driver that supports USERPTR would have to implement
buf_init, as you said.

> Alternatively the check could be performed in videobuf2-core, which would
> probably make sense as the check is required. videobuf2 doesn't store the
> minimum size for USERPTR buffers though (but that can of course be changed).
>

Let's say we make vb2 save minimum buffer size. This would have to be
done on queue_setup I imagine. We could add a new field to vb2_buffer
for that. One problem is that if the driver actually supports changing
format after REQBUFS, we would need a new function in vb2 to change
minimum buffer size. Actually, this has to be minimum plane sizes. So
the alternatives are:

1. Make buf_init required for drivers that support USERPTR; or
2. Add minimum plane sizes to vb2_buffer,add a new return array
argument to queue_setup to return minimum plane sizes that would be
stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2
function for drivers to call when minimum sizes have to be changed.

The first solution is way simpler for drivers that require this. The
second solution is maybe a bit simpler for drivers that do not, as
they would only have to return the sizes in queue_setup, but
implementing buf_init instead wouldn't be a big of a difference I
think. So I'm leaning towards the second solution.
Any comments, did I miss something?

-- 
Best regards,
Pawel Osciak

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-05 15:01 ` Pawel Osciak
@ 2011-08-05 16:14   ` Pawel Osciak
  2011-08-08 10:11   ` Laurent Pinchart
  1 sibling, 0 replies; 8+ messages in thread
From: Pawel Osciak @ 2011-08-05 16:14 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Marek Szyprowski, linux-media

Sorry, just realized I mixed up callback names a bit in my previous
response. Let me rephrase:

Drivers may allow changing formats after buffers have been
initialized. This means that it's possible to s_fmt, reqbufs,
streamon, streamoff and s_fmt again without changing buffers. buf_init
will still be called for USERPTR though, even if the same buffers are
used after resume. So enforcing buf_init will always work.

If buf_init were not to be required for USERPTR, we'd need the drivers
to report change of minimum buffer (planes) size asynchronously to vb2
from the driver. And get them on alloc from the driver on queue_setup.
So the two options I described in my first response are still
standing, although I made a typo there, saying I'd prefer the second
solution, I obviously meant the first one, i.e. enforcing buf_init().
Sorry.

One additional detail to the second proposal below: if we were to have
the new callback, it could only be allowed when not streaming.
Otherwise we'd have to forcibly dequeue buffers if they became too
small and this would be confusing for applications.

Pawel


On Fri, Aug 5, 2011 at 08:01, Pawel Osciak <pawel@osciak.com> wrote:
> Hi Laurent,
>
> On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Marek and Pawel,
>>
>> While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't verify
>> the buffer length field value when a new USERPTR buffer is queued.
>>
>
> That's a good catch. We should definitely do something about it.
>
>> The length given by userspace is copied to the internal buffer length field.
>> It seems to be up to drivers to verify that the value is equal to or higher
>> than the minimum required to store the image, but that's not clearly
>> documented. Should the buf_init operation be made mandatory for drivers that
>> support USERPTR, and documented as required to implement a length check ?
>>
>
> Technically, drivers can do the length checks on buf_prepare if they
> don't allow format changes after REQBUFS. On the other hand though, if
> a driver supports USERPTR, the buffers queued from userspace have to
> be verified on qbuf and the only place to do that would be buf_init.
> So every driver that supports USERPTR would have to implement
> buf_init, as you said.
>
>> Alternatively the check could be performed in videobuf2-core, which would
>> probably make sense as the check is required. videobuf2 doesn't store the
>> minimum size for USERPTR buffers though (but that can of course be changed).
>>
>
> Let's say we make vb2 save minimum buffer size. This would have to be
> done on queue_setup I imagine. We could add a new field to vb2_buffer
> for that. One problem is that if the driver actually supports changing
> format after REQBUFS, we would need a new function in vb2 to change
> minimum buffer size. Actually, this has to be minimum plane sizes. So
> the alternatives are:
>
> 1. Make buf_init required for drivers that support USERPTR; or
> 2. Add minimum plane sizes to vb2_buffer,add a new return array
> argument to queue_setup to return minimum plane sizes that would be
> stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2
> function for drivers to call when minimum sizes have to be changed.
>
> The first solution is way simpler for drivers that require this. The
> second solution is maybe a bit simpler for drivers that do not, as
> they would only have to return the sizes in queue_setup, but
> implementing buf_init instead wouldn't be a big of a difference I
> think. So I'm leaning towards the second solution.
> Any comments, did I miss something?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-05 15:01 ` Pawel Osciak
  2011-08-05 16:14   ` Pawel Osciak
@ 2011-08-08 10:11   ` Laurent Pinchart
  2011-08-09  9:14     ` Marek Szyprowski
  1 sibling, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2011-08-08 10:11 UTC (permalink / raw)
  To: Pawel Osciak; +Cc: Marek Szyprowski, linux-media

Hi Pawel,

On Friday 05 August 2011 17:01:09 Pawel Osciak wrote:
> On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote:
> > Hi Marek and Pawel,
> > 
> > While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't
> > verify the buffer length field value when a new USERPTR buffer is
> > queued.
> 
> That's a good catch. We should definitely do something about it.
> 
> > The length given by userspace is copied to the internal buffer length
> > field. It seems to be up to drivers to verify that the value is equal to
> > or higher than the minimum required to store the image, but that's not
> > clearly documented. Should the buf_init operation be made mandatory for
> > drivers that support USERPTR, and documented as required to implement a
> > length check ?
> 
> Technically, drivers can do the length checks on buf_prepare if they
> don't allow format changes after REQBUFS. On the other hand though, if
> a driver supports USERPTR, the buffers queued from userspace have to
> be verified on qbuf and the only place to do that would be buf_init.
> So every driver that supports USERPTR would have to implement
> buf_init, as you said.
> 
> > Alternatively the check could be performed in videobuf2-core, which would
> > probably make sense as the check is required. videobuf2 doesn't store the
> > minimum size for USERPTR buffers though (but that can of course be
> > changed).
> 
> Let's say we make vb2 save minimum buffer size. This would have to be
> done on queue_setup I imagine. We could add a new field to vb2_buffer
> for that. One problem is that if the driver actually supports changing
> format after REQBUFS, we would need a new function in vb2 to change
> minimum buffer size. Actually, this has to be minimum plane sizes. So
> the alternatives are:
> 
> 1. Make buf_init required for drivers that support USERPTR; or
> 2. Add minimum plane sizes to vb2_buffer,add a new return array
> argument to queue_setup to return minimum plane sizes that would be
> stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2
> function for drivers to call when minimum sizes have to be changed.
> 
> The first solution is way simpler for drivers that require this. The
> second solution is maybe a bit simpler for drivers that do not, as
> they would only have to return the sizes in queue_setup, but
> implementing buf_init instead wouldn't be a big of a difference I
> think. So I'm leaning towards the second solution.
> Any comments, did I miss something?

Thanks for the analysis. I would go for the second solution as well.

Do we have any driver that supports changing the format after REQBUFS ? If not 
we can delay adding the new vb2 function until we get such a driver.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-08 10:11   ` Laurent Pinchart
@ 2011-08-09  9:14     ` Marek Szyprowski
  2011-08-09 11:51       ` Laurent Pinchart
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Szyprowski @ 2011-08-09  9:14 UTC (permalink / raw)
  To: 'Laurent Pinchart', 'Pawel Osciak',
	'Marek Szyprowski'
  Cc: linux-media

Hello,

On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote:

> On Friday 05 August 2011 17:01:09 Pawel Osciak wrote:
> > On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote:
> > > Hi Marek and Pawel,
> > >
> > > While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't
> > > verify the buffer length field value when a new USERPTR buffer is
> > > queued.
> >
> > That's a good catch. We should definitely do something about it.
> >
> > > The length given by userspace is copied to the internal buffer length
> > > field. It seems to be up to drivers to verify that the value is equal to
> > > or higher than the minimum required to store the image, but that's not
> > > clearly documented. Should the buf_init operation be made mandatory for
> > > drivers that support USERPTR, and documented as required to implement a
> > > length check ?
> >
> > Technically, drivers can do the length checks on buf_prepare if they
> > don't allow format changes after REQBUFS. On the other hand though, if
> > a driver supports USERPTR, the buffers queued from userspace have to
> > be verified on qbuf and the only place to do that would be buf_init.
> > So every driver that supports USERPTR would have to implement
> > buf_init, as you said.
> >
> > > Alternatively the check could be performed in videobuf2-core, which would
> > > probably make sense as the check is required. videobuf2 doesn't store the
> > > minimum size for USERPTR buffers though (but that can of course be
> > > changed).
> >
> > Let's say we make vb2 save minimum buffer size. This would have to be
> > done on queue_setup I imagine. We could add a new field to vb2_buffer
> > for that. One problem is that if the driver actually supports changing
> > format after REQBUFS, we would need a new function in vb2 to change
> > minimum buffer size. Actually, this has to be minimum plane sizes. So
> > the alternatives are:
> >
> > 1. Make buf_init required for drivers that support USERPTR; or
> > 2. Add minimum plane sizes to vb2_buffer,add a new return array
> > argument to queue_setup to return minimum plane sizes that would be
> > stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2
> > function for drivers to call when minimum sizes have to be changed.
> >
> > The first solution is way simpler for drivers that require this. The
> > second solution is maybe a bit simpler for drivers that do not, as
> > they would only have to return the sizes in queue_setup, but
> > implementing buf_init instead wouldn't be a big of a difference I
> > think. So I'm leaning towards the second solution.
> > Any comments, did I miss something?
> 
> Thanks for the analysis. I would go for the second solution as well.
> 
> Do we have any driver that supports changing the format after REQBUFS ? If not
> we can delay adding the new vb2 function until we get such a driver.

I really wonder if we should support changing the format which results in 
buffer/plane size change after REQBUFS. Please notice that it causes additional
problems with mmap-style buffers, which are allocated once during the REQBUFS
call. Also none driver support it right now and I doubt that changing buffer
size after REQBUFS can be implemented without breaking anything other (there
are a lot of things that depends on buffer size, both in vb2 and the drivers).

I would just simplify solution number 2 - imho vb2 should just store the
minimal buffer/plane size acquired in queue_setup and check if any buffers
that have been queued are large enough.

If one wants to change format to the one that requires other buffer size,
then he should just call STREAM_OFF, REQBUFS(0), S_FMT, REQBUFS(n) and
STREAM_ON. The other solution will be to use separately created buffers,
which already have format/size information attached (Guennadi's proposal).

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-09  9:14     ` Marek Szyprowski
@ 2011-08-09 11:51       ` Laurent Pinchart
  2011-08-09 12:07         ` Marek Szyprowski
  0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2011-08-09 11:51 UTC (permalink / raw)
  To: Marek Szyprowski; +Cc: 'Pawel Osciak', linux-media

Hi Marek,

On Tuesday 09 August 2011 11:14:43 Marek Szyprowski wrote:
> On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote:
> > On Friday 05 August 2011 17:01:09 Pawel Osciak wrote:
> > > On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote:
> > > > Hi Marek and Pawel,
> > > > 
> > > > While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't
> > > > verify the buffer length field value when a new USERPTR buffer is
> > > > queued.
> > > 
> > > That's a good catch. We should definitely do something about it.
> > > 
> > > > The length given by userspace is copied to the internal buffer length
> > > > field. It seems to be up to drivers to verify that the value is equal
> > > > to or higher than the minimum required to store the image, but
> > > > that's not clearly documented. Should the buf_init operation be made
> > > > mandatory for drivers that support USERPTR, and documented as
> > > > required to implement a length check ?
> > > 
> > > Technically, drivers can do the length checks on buf_prepare if they
> > > don't allow format changes after REQBUFS. On the other hand though, if
> > > a driver supports USERPTR, the buffers queued from userspace have to
> > > be verified on qbuf and the only place to do that would be buf_init.
> > > So every driver that supports USERPTR would have to implement
> > > buf_init, as you said.
> > > 
> > > > Alternatively the check could be performed in videobuf2-core, which
> > > > would probably make sense as the check is required. videobuf2
> > > > doesn't store the minimum size for USERPTR buffers though (but that
> > > > can of course be changed).
> > > 
> > > Let's say we make vb2 save minimum buffer size. This would have to be
> > > done on queue_setup I imagine. We could add a new field to vb2_buffer
> > > for that. One problem is that if the driver actually supports changing
> > > format after REQBUFS, we would need a new function in vb2 to change
> > > minimum buffer size. Actually, this has to be minimum plane sizes. So
> > > the alternatives are:
> > > 
> > > 1. Make buf_init required for drivers that support USERPTR; or
> > > 2. Add minimum plane sizes to vb2_buffer,add a new return array
> > > argument to queue_setup to return minimum plane sizes that would be
> > > stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2
> > > function for drivers to call when minimum sizes have to be changed.
> > > 
> > > The first solution is way simpler for drivers that require this. The
> > > second solution is maybe a bit simpler for drivers that do not, as
> > > they would only have to return the sizes in queue_setup, but
> > > implementing buf_init instead wouldn't be a big of a difference I
> > > think. So I'm leaning towards the second solution.
> > > Any comments, did I miss something?
> > 
> > Thanks for the analysis. I would go for the second solution as well.
> > 
> > Do we have any driver that supports changing the format after REQBUFS ?
> > If not we can delay adding the new vb2 function until we get such a
> > driver.
> 
> I really wonder if we should support changing the format which results in
> buffer/plane size change after REQBUFS. Please notice that it causes
> additional problems with mmap-style buffers, which are allocated once
> during the REQBUFS call. Also none driver support it right now and I doubt
> that changing buffer size after REQBUFS can be implemented without
> breaking anything other (there are a lot of things that depends on buffer
> size, both in vb2 and the drivers).

I wasn't awake enough when I sent my previous reply. We probably have no 
driver that supports this feature at the moment, but changing the format after 
REQBUFS is the whole point of the CREATE_BUFS and PREPARE_BUF ioctls, so I 
think we definitely need to support that :-)

> I would just simplify solution number 2 - imho vb2 should just store the
> minimal buffer/plane size acquired in queue_setup and check if any buffers
> that have been queued are large enough.
> 
> If one wants to change format to the one that requires other buffer size,
> then he should just call STREAM_OFF, REQBUFS(0), S_FMT, REQBUFS(n) and
> STREAM_ON. The other solution will be to use separately created buffers,
> which already have format/size information attached (Guennadi's proposal).

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-09 11:51       ` Laurent Pinchart
@ 2011-08-09 12:07         ` Marek Szyprowski
  2011-08-09 12:17           ` Laurent Pinchart
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Szyprowski @ 2011-08-09 12:07 UTC (permalink / raw)
  To: 'Laurent Pinchart'
  Cc: 'Pawel Osciak', linux-media, 'Marek Szyprowski'

Hello,

On Tuesday, August 09, 2011 1:52 PM Laurent Pinchart wrote:
> On Tuesday 09 August 2011 11:14:43 Marek Szyprowski wrote:
> > On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote:
> > > On Friday 05 August 2011 17:01:09 Pawel Osciak wrote:
> > > > On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote:
> > > > > Hi Marek and Pawel,
> > > > >
> > > > > While reviewing an OMAP3 ISP patch, I noticed that videobuf2 doesn't
> > > > > verify the buffer length field value when a new USERPTR buffer is
> > > > > queued.
> > > >
> > > > That's a good catch. We should definitely do something about it.
> > > >
> > > > > The length given by userspace is copied to the internal buffer length
> > > > > field. It seems to be up to drivers to verify that the value is equal
> > > > > to or higher than the minimum required to store the image, but
> > > > > that's not clearly documented. Should the buf_init operation be made
> > > > > mandatory for drivers that support USERPTR, and documented as
> > > > > required to implement a length check ?
> > > >
> > > > Technically, drivers can do the length checks on buf_prepare if they
> > > > don't allow format changes after REQBUFS. On the other hand though, if
> > > > a driver supports USERPTR, the buffers queued from userspace have to
> > > > be verified on qbuf and the only place to do that would be buf_init.
> > > > So every driver that supports USERPTR would have to implement
> > > > buf_init, as you said.
> > > >
> > > > > Alternatively the check could be performed in videobuf2-core, which
> > > > > would probably make sense as the check is required. videobuf2
> > > > > doesn't store the minimum size for USERPTR buffers though (but that
> > > > > can of course be changed).
> > > >
> > > > Let's say we make vb2 save minimum buffer size. This would have to be
> > > > done on queue_setup I imagine. We could add a new field to vb2_buffer
> > > > for that. One problem is that if the driver actually supports changing
> > > > format after REQBUFS, we would need a new function in vb2 to change
> > > > minimum buffer size. Actually, this has to be minimum plane sizes. So
> > > > the alternatives are:
> > > >
> > > > 1. Make buf_init required for drivers that support USERPTR; or
> > > > 2. Add minimum plane sizes to vb2_buffer,add a new return array
> > > > argument to queue_setup to return minimum plane sizes that would be
> > > > stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new vb2
> > > > function for drivers to call when minimum sizes have to be changed.
> > > >
> > > > The first solution is way simpler for drivers that require this. The
> > > > second solution is maybe a bit simpler for drivers that do not, as
> > > > they would only have to return the sizes in queue_setup, but
> > > > implementing buf_init instead wouldn't be a big of a difference I
> > > > think. So I'm leaning towards the second solution.
> > > > Any comments, did I miss something?
> > >
> > > Thanks for the analysis. I would go for the second solution as well.
> > >
> > > Do we have any driver that supports changing the format after REQBUFS ?
> > > If not we can delay adding the new vb2 function until we get such a
> > > driver.
> >
> > I really wonder if we should support changing the format which results in
> > buffer/plane size change after REQBUFS. Please notice that it causes
> > additional problems with mmap-style buffers, which are allocated once
> > during the REQBUFS call. Also none driver support it right now and I doubt
> > that changing buffer size after REQBUFS can be implemented without
> > breaking anything other (there are a lot of things that depends on buffer
> > size, both in vb2 and the drivers).
> 
> I wasn't awake enough when I sent my previous reply. We probably have no
> driver that supports this feature at the moment, but changing the format after
> REQBUFS is the whole point of the CREATE_BUFS and PREPARE_BUF ioctls, so I
> think we definitely need to support that :-)

Right, but this is an extension that will come with CRATE_BUFS/PREPARE_BUF
calls.
Until that, to handle buffers correctly we only need to add min_size entry to
the
queue and check if queued buffers are large enough.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Possible issue in videobuf2 with buffer length check at QBUF time
  2011-08-09 12:07         ` Marek Szyprowski
@ 2011-08-09 12:17           ` Laurent Pinchart
  0 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2011-08-09 12:17 UTC (permalink / raw)
  To: Marek Szyprowski; +Cc: 'Pawel Osciak', linux-media

Hi Marek,

On Tuesday 09 August 2011 14:07:12 Marek Szyprowski wrote:
> On Tuesday, August 09, 2011 1:52 PM Laurent Pinchart wrote:
> > On Tuesday 09 August 2011 11:14:43 Marek Szyprowski wrote:
> > > On Monday, August 08, 2011 12:11 PM Laurent Pinchart wrote:
> > > > On Friday 05 August 2011 17:01:09 Pawel Osciak wrote:
> > > > > On Fri, Aug 5, 2011 at 01:55, Laurent Pinchart wrote:
> > > > > > Hi Marek and Pawel,
> > > > > > 
> > > > > > While reviewing an OMAP3 ISP patch, I noticed that videobuf2
> > > > > > doesn't verify the buffer length field value when a new USERPTR
> > > > > > buffer is queued.
> > > > > 
> > > > > That's a good catch. We should definitely do something about it.
> > > > > 
> > > > > > The length given by userspace is copied to the internal buffer
> > > > > > length field. It seems to be up to drivers to verify that the
> > > > > > value is equal to or higher than the minimum required to store
> > > > > > the image, but that's not clearly documented. Should the
> > > > > > buf_init operation be made mandatory for drivers that support
> > > > > > USERPTR, and documented as required to implement a length check
> > > > > > ?
> > > > > 
> > > > > Technically, drivers can do the length checks on buf_prepare if
> > > > > they don't allow format changes after REQBUFS. On the other hand
> > > > > though, if a driver supports USERPTR, the buffers queued from
> > > > > userspace have to be verified on qbuf and the only place to do
> > > > > that would be buf_init. So every driver that supports USERPTR
> > > > > would have to implement buf_init, as you said.
> > > > > 
> > > > > > Alternatively the check could be performed in videobuf2-core,
> > > > > > which would probably make sense as the check is required.
> > > > > > videobuf2 doesn't store the minimum size for USERPTR buffers
> > > > > > though (but that can of course be changed).
> > > > > 
> > > > > Let's say we make vb2 save minimum buffer size. This would have to
> > > > > be done on queue_setup I imagine. We could add a new field to
> > > > > vb2_buffer for that. One problem is that if the driver actually
> > > > > supports changing format after REQBUFS, we would need a new
> > > > > function in vb2 to change minimum buffer size. Actually, this has
> > > > > to be minimum plane sizes. So the alternatives are:
> > > > > 
> > > > > 1. Make buf_init required for drivers that support USERPTR; or
> > > > > 2. Add minimum plane sizes to vb2_buffer,add a new return array
> > > > > argument to queue_setup to return minimum plane sizes that would be
> > > > > stored in vb2. Make vb2 verify sizes on qbuf of USERPTR. Add a new
> > > > > vb2 function for drivers to call when minimum sizes have to be
> > > > > changed.
> > > > > 
> > > > > The first solution is way simpler for drivers that require this.
> > > > > The second solution is maybe a bit simpler for drivers that do
> > > > > not, as they would only have to return the sizes in queue_setup,
> > > > > but implementing buf_init instead wouldn't be a big of a
> > > > > difference I think. So I'm leaning towards the second solution.
> > > > > Any comments, did I miss something?
> > > > 
> > > > Thanks for the analysis. I would go for the second solution as well.
> > > > 
> > > > Do we have any driver that supports changing the format after REQBUFS
> > > > ? If not we can delay adding the new vb2 function until we get such
> > > > a driver.
> > > 
> > > I really wonder if we should support changing the format which results
> > > in buffer/plane size change after REQBUFS. Please notice that it
> > > causes additional problems with mmap-style buffers, which are
> > > allocated once during the REQBUFS call. Also none driver support it
> > > right now and I doubt that changing buffer size after REQBUFS can be
> > > implemented without breaking anything other (there are a lot of things
> > > that depends on buffer size, both in vb2 and the drivers).
> > 
> > I wasn't awake enough when I sent my previous reply. We probably have no
> > driver that supports this feature at the moment, but changing the format
> > after REQBUFS is the whole point of the CREATE_BUFS and PREPARE_BUF
> > ioctls, so I think we definitely need to support that :-)
> 
> Right, but this is an extension that will come with CRATE_BUFS/PREPARE_BUF
> calls. Until that, to handle buffers correctly we only need to add min_size
> entry to the queue and check if queued buffers are large enough.

I'm fine with that, but that's not a reason to ignore the bigger picture, 
especially as it will come back to bite us pretty soon :-)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-08-09 12:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-05  8:55 Possible issue in videobuf2 with buffer length check at QBUF time Laurent Pinchart
2011-08-05 15:01 ` Pawel Osciak
2011-08-05 16:14   ` Pawel Osciak
2011-08-08 10:11   ` Laurent Pinchart
2011-08-09  9:14     ` Marek Szyprowski
2011-08-09 11:51       ` Laurent Pinchart
2011-08-09 12:07         ` Marek Szyprowski
2011-08-09 12:17           ` Laurent Pinchart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.