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