[PATCH 2/3] libcamera: v4l2_videodevice: Report offset in dequeueBuffer

[Paul Elder]

On Wed, Apr 02, 2025 at 03:09:42AM +0300, Laurent Pinchart wrote:
> On Fri, Mar 28, 2025 at 04:51:12PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 26 mars 2025 à 16:51 +0900, Paul Elder a écrit :
> > > Fill in the offset field of the frame metadata so that it the
> > > application can read it.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp
> > > b/src/libcamera/v4l2_videodevice.cpp
> > > index f5b3fa09d9a0..5d163f7fd853 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1957,10 +1957,13 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >  		 * The number of planes in the frame buffer and in the
> > >  		 * V4L2 buffer is guaranteed to be equal at this point.
> > >  		 */
> > > -		for (unsigned int i = 0; i < numV4l2Planes; ++i)
> > > +		for (unsigned int i = 0; i < numV4l2Planes; ++i) {
> > >  			metadata.planes()[i].bytesused = planes[i].bytesused;
> > > +			metadata.planes()[i].offset = planes[i].data_offset;
> > 
> > In V4L2, data_offset is included in the total bytesused. Once you hit a
> > non-zero offset, you will endup with byteused possibly bigger then
> > FrameBuffer.length, which to my reading contradicts the doc.
> > 
> > https://www.linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/buffer.html?highlight=data_offset#struct-v4l2-plane
> > 
> > > +		}
> > >  	} else {
> > >  		metadata.planes()[0].bytesused = buf.bytesused;
> > > +		metadata.planes()[0].offset = buf.m.offset;
> > 
> > Should be:
> > 
> > +		metadata.planes()[0].offset = 0;
> > 
> > There is no support for data_offset in the original v4l2_buffer API.
> > This offset match planes[i].mem_offset and is only useful for CPU
> > access using mmap() and the video node FD.
> > 
> > I think some introspection is needed, since this offset was expected to
> > be in the framebuffer, which apparently needs to be constant. Once
> > these issue resovled, do you mind fixing GStreamer code to use the
> > right one, it reads like this:
> > 
> > 		GstMemory *mem = gst_fd_allocator_alloc(allocator, plane.fd.get(),
> > 							plane.offset + plane.length,
> > 							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> > 		gst_memory_resize(mem, plane.offset, plane.length);
> > 
> > Plane.offset is never set (always zero atm), but if its constant, its
> > the wrong one, this code should use the metadata once (the variable
> > one). You will also notice the semantic difference, we assume length
> > does not include the offset, this might need adjustment.
> 
> Outside of codec drivers, data_offset is a bit of a problematic API. The
> issue with reporting a dynamic offset is that it make it impossible to
> use fences and queue the buffer with a consumer before we know the
> offset value. The concern is of course theoretical, given that V4L2
> doesn't have fence support, but I would really not to see widespread
> data_offset usage before we figure this issue out.

Ah ok, I see. The data is practically static but it's treated as
dynamic. We can't expose it as static on the libcamera side either
because... it's dynamic as exposed by the kernel.

> Not counting the vivid test driver and codec drivers, data_offset is
> only used by the omap3isp driver, for entirely historical reasons (we
> didn't know what we were doing at the time). I'd like to understand your
> use cases better in order to determine the best way forward.

It's for a client's custom platform, so I suppose I'll keep these
out-of-tree then, since upstream support of this would need a larger
scale kernel change.


Thanks,

Paul

> Note that there is a different, and in my opinion entirely valid, use of
> offsets, which is indicating where a plane is located within a dmabuf.
> This isn't supported by V4L2 at the moment (there are old patches on the
> list for this, but they died out), but is I think a useful and needed
> feature (DRM supports it). The FrameBuffer::Plane::offset field is meant
> for this.
> 
> > >  	}
> > >  
> > >  	return buffer;