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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
