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 -- 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
