<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 21, 2023 at 2:28 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Vedant Paranjape (2023-08-21 15:43:55)<br>
> Hello Gabby,<br>
> <br>
> Thanks for your patch. This patch looks good to me.<br>
> <br></blockquote><div>Thanks Vedant! <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>> wrote:<br>
> ><br>
> > To perform a memory mapping using mmap, the MappedFrameBuffer class<br>
> > needs the plane offset and file descriptor information of the frame<br>
> > buffer's plane(s). This information is provided in the response to<br>
> > REQBUF, which happens during buffer allocation. Store the plane offset<br>
> > and file descriptor information in the buffer's plane at the time of<br>
> > allocation.<br>
> ><br>
> > Currently, there is a metadata buffer type (metadata format UVCH) that<br>
> > does not support exporting buffers using EXPBUF, so this should only<br>
> > be done if the buffer type is metadata capture.<br>
> ><br>
> > Signed-off-by: Gabby George <<a href="mailto:gabbymg94@gmail.com" target="_blank">gabbymg94@gmail.com</a>><br>
> <br>
> Reviewed-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank">vedantparanjape160201@gmail.com</a>><br>
> <br>
> > ---<br>
> > src/libcamera/v4l2_videodevice.cpp | 31 +++++++++++++++++++-----------<br>
> > 1 file changed, 20 insertions(+), 11 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp<br>
> > index a72ef64d..e57cb131 100644<br>
> > --- a/src/libcamera/v4l2_videodevice.cpp<br>
> > +++ b/src/libcamera/v4l2_videodevice.cpp<br>
> > @@ -1402,18 +1402,27 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)<br>
> ><br>
> > std::vector<FrameBuffer::Plane> planes;<br>
> > for (unsigned int nplane = 0; nplane < numPlanes; nplane++) {<br>
> > - UniqueFD fd = exportDmabufFd(buf.index, nplane);<br>
> > - if (!fd.isValid())<br>
> > - return nullptr;<br>
> > -<br>
> > FrameBuffer::Plane plane;<br>
> > - plane.fd = SharedFD(std::move(fd));<br>
> > - /*<br>
> > - * V4L2 API doesn't provide dmabuf offset information of plane.<br>
> > - * Set 0 as a placeholder offset.<br>
> > - * \todo Set the right offset once V4L2 API provides a way.<br>
> > - */<br>
> > - plane.offset = 0;<br>
> > +<br>
> > + if (buf.type == V4L2_BUF_TYPE_META_CAPTURE) {<br>
> > + /*<br>
> > + * Dmabuf fd is not exported for metadata, so store<br>
> > + * the offset from the querybuf call and this device's fd.<br>
> > + */<br>
> > + plane.fd = SharedFD(this->fd());<br>
> > + plane.offset = buf.m.offset;<br>
> > + } else {<br>
> > + UniqueFD fd = exportDmabufFd(buf.index, nplane);<br>
> > + if (!fd.isValid())<br>
> > + return nullptr;<br>
> > + plane.fd = SharedFD(std::move(fd));<br>
> > + /*<br>
> > + * V4L2 API doesn't provide dmabuf offset information of plane.<br>
<br>
This indent is still incorrect.<br>
/*<br>
*<br>
instead of <br>
/*<br>
*<br>
<br>
<br></blockquote><div>I will fix it! <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > + * Set 0 as a placeholder offset.<br>
> > + * \todo Set the right offset once V4L2 API provides a way.<br>
> > + */<br>
> > + plane.offset = 0;<br>
<br>
So are plane.offset's *always* zero ? Does anything else ever set it to<br>
something else? That looks like it signals the same information as<br>
'usePlaneOffset' to me in that case ?<br>
<br></blockquote><div><br></div><div>I looked into this and there are cases in android code where it gets set to something other than 0, and for multi planar formats it will be set to something else a few lines down in this function. <br></div><div><br></div><div>I can't easily determine if MappedFrameBuffers would be used in these scenarios, but I suspect it could. </div><div><br></div><div>Here's why (warning, this is kind of confusing):</div><div>It does appear that multiplanar code could end up being used for the frame buffer that gets mapped in MappedFrameBuffers in camera_device.cpp. There is a loop that runs through all possible planes and sets the buffer offset to what is presumably a non-zero value since the planes would have different offsets.<br></div><div><br></div><div>It seems like the returned frame buffer of that function can end up being used in a MappedFrameBuffer constructor (for example, the one in PostProcessorYuv::process, which takes in a buffer that could have been created using "createFrameBuffer") and therefore end up hitting the flagged code, but it's really hard to tell if that would happen in practice. <br><br></div><div>Saying that, I think this connection makes sense, because if you're creating an mmap-ed address for YUV data which is what PostProcessorYuv::process appears to do, you would be using multiple planes and therefore would need to make use of the (non-zero) offset.</div><div><br></div><div>I'll keep thinking about ways to possibly get rid of the usePlaneOffset flag, but I'm not sure just assuming that plane offset will always be 0 unless we're dealing with metadata won't break something. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > + }<br>
> > plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;<br>
> ><br>
> > planes.push_back(std::move(plane));<br>
> > --<br>
> > 2.34.1<br>
> ><br>
</blockquote></div></div>