| Message ID | 20230821131039.127370-4-gabbymg94@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hello Gabby, Thanks for your patch. This patch looks good to me. On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@gmail.com> wrote: > > To perform a memory mapping using mmap, the MappedFrameBuffer class > needs the plane offset and file descriptor information of the frame > buffer's plane(s). This information is provided in the response to > REQBUF, which happens during buffer allocation. Store the plane offset > and file descriptor information in the buffer's plane at the time of > allocation. > > Currently, there is a metadata buffer type (metadata format UVCH) that > does not support exporting buffers using EXPBUF, so this should only > be done if the buffer type is metadata capture. > > Signed-off-by: Gabby George <gabbymg94@gmail.com> Reviewed-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > src/libcamera/v4l2_videodevice.cpp | 31 +++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index a72ef64d..e57cb131 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1402,18 +1402,27 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > std::vector<FrameBuffer::Plane> planes; > for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { > - UniqueFD fd = exportDmabufFd(buf.index, nplane); > - if (!fd.isValid()) > - return nullptr; > - > FrameBuffer::Plane plane; > - plane.fd = SharedFD(std::move(fd)); > - /* > - * V4L2 API doesn't provide dmabuf offset information of plane. > - * Set 0 as a placeholder offset. > - * \todo Set the right offset once V4L2 API provides a way. > - */ > - plane.offset = 0; > + > + if (buf.type == V4L2_BUF_TYPE_META_CAPTURE) { > + /* > + * Dmabuf fd is not exported for metadata, so store > + * the offset from the querybuf call and this device's fd. > + */ > + plane.fd = SharedFD(this->fd()); > + plane.offset = buf.m.offset; > + } else { > + UniqueFD fd = exportDmabufFd(buf.index, nplane); > + if (!fd.isValid()) > + return nullptr; > + plane.fd = SharedFD(std::move(fd)); > + /* > + * V4L2 API doesn't provide dmabuf offset information of plane. > + * Set 0 as a placeholder offset. > + * \todo Set the right offset once V4L2 API provides a way. > + */ > + plane.offset = 0; > + } > plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > planes.push_back(std::move(plane)); > -- > 2.34.1 >
Quoting Vedant Paranjape (2023-08-21 15:43:55) > Hello Gabby, > > Thanks for your patch. This patch looks good to me. > > On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@gmail.com> wrote: > > > > To perform a memory mapping using mmap, the MappedFrameBuffer class > > needs the plane offset and file descriptor information of the frame > > buffer's plane(s). This information is provided in the response to > > REQBUF, which happens during buffer allocation. Store the plane offset > > and file descriptor information in the buffer's plane at the time of > > allocation. > > > > Currently, there is a metadata buffer type (metadata format UVCH) that > > does not support exporting buffers using EXPBUF, so this should only > > be done if the buffer type is metadata capture. > > > > Signed-off-by: Gabby George <gabbymg94@gmail.com> > > Reviewed-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > --- > > src/libcamera/v4l2_videodevice.cpp | 31 +++++++++++++++++++----------- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index a72ef64d..e57cb131 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -1402,18 +1402,27 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > std::vector<FrameBuffer::Plane> planes; > > for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { > > - UniqueFD fd = exportDmabufFd(buf.index, nplane); > > - if (!fd.isValid()) > > - return nullptr; > > - > > FrameBuffer::Plane plane; > > - plane.fd = SharedFD(std::move(fd)); > > - /* > > - * V4L2 API doesn't provide dmabuf offset information of plane. > > - * Set 0 as a placeholder offset. > > - * \todo Set the right offset once V4L2 API provides a way. > > - */ > > - plane.offset = 0; > > + > > + if (buf.type == V4L2_BUF_TYPE_META_CAPTURE) { > > + /* > > + * Dmabuf fd is not exported for metadata, so store > > + * the offset from the querybuf call and this device's fd. > > + */ > > + plane.fd = SharedFD(this->fd()); > > + plane.offset = buf.m.offset; > > + } else { > > + UniqueFD fd = exportDmabufFd(buf.index, nplane); > > + if (!fd.isValid()) > > + return nullptr; > > + plane.fd = SharedFD(std::move(fd)); > > + /* > > + * V4L2 API doesn't provide dmabuf offset information of plane. This indent is still incorrect. /* * instead of /* * > > + * Set 0 as a placeholder offset. > > + * \todo Set the right offset once V4L2 API provides a way. > > + */ > > + plane.offset = 0; So are plane.offset's *always* zero ? Does anything else ever set it to something else? That looks like it signals the same information as 'usePlaneOffset' to me in that case ? > > + } > > plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > planes.push_back(std::move(plane)); > > -- > > 2.34.1 > >
On Mon, Aug 21, 2023 at 2:28 PM Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Vedant Paranjape (2023-08-21 15:43:55) > > Hello Gabby, > > > > Thanks for your patch. This patch looks good to me. > > > Thanks Vedant! > > On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@gmail.com> > wrote: > > > > > > To perform a memory mapping using mmap, the MappedFrameBuffer class > > > needs the plane offset and file descriptor information of the frame > > > buffer's plane(s). This information is provided in the response to > > > REQBUF, which happens during buffer allocation. Store the plane offset > > > and file descriptor information in the buffer's plane at the time of > > > allocation. > > > > > > Currently, there is a metadata buffer type (metadata format UVCH) that > > > does not support exporting buffers using EXPBUF, so this should only > > > be done if the buffer type is metadata capture. > > > > > > Signed-off-by: Gabby George <gabbymg94@gmail.com> > > > > Reviewed-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > > > > --- > > > src/libcamera/v4l2_videodevice.cpp | 31 +++++++++++++++++++----------- > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > b/src/libcamera/v4l2_videodevice.cpp > > > index a72ef64d..e57cb131 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -1402,18 +1402,27 @@ std::unique_ptr<FrameBuffer> > V4L2VideoDevice::createBuffer(unsigned int index) > > > > > > std::vector<FrameBuffer::Plane> planes; > > > for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { > > > - UniqueFD fd = exportDmabufFd(buf.index, nplane); > > > - if (!fd.isValid()) > > > - return nullptr; > > > - > > > FrameBuffer::Plane plane; > > > - plane.fd = SharedFD(std::move(fd)); > > > - /* > > > - * V4L2 API doesn't provide dmabuf offset information > of plane. > > > - * Set 0 as a placeholder offset. > > > - * \todo Set the right offset once V4L2 API provides a > way. > > > - */ > > > - plane.offset = 0; > > > + > > > + if (buf.type == V4L2_BUF_TYPE_META_CAPTURE) { > > > + /* > > > + * Dmabuf fd is not exported for metadata, so > store > > > + * the offset from the querybuf call and this > device's fd. > > > + */ > > > + plane.fd = SharedFD(this->fd()); > > > + plane.offset = buf.m.offset; > > > + } else { > > > + UniqueFD fd = exportDmabufFd(buf.index, > nplane); > > > + if (!fd.isValid()) > > > + return nullptr; > > > + plane.fd = SharedFD(std::move(fd)); > > > + /* > > > + * V4L2 API doesn't provide dmabuf offset > information of plane. > > This indent is still incorrect. > /* > * > instead of > /* > * > > > I will fix it! > > > + * Set 0 as a placeholder offset. > > > + * \todo Set the right offset once V4L2 API > provides a way. > > > + */ > > > + plane.offset = 0; > > So are plane.offset's *always* zero ? Does anything else ever set it to > something else? That looks like it signals the same information as > 'usePlaneOffset' to me in that case ? > > 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. I can't easily determine if MappedFrameBuffers would be used in these scenarios, but I suspect it could. Here's why (warning, this is kind of confusing): 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. 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. 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. 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. > > > + } > > > plane.length = multiPlanar ? > buf.m.planes[nplane].length : buf.length; > > > > > > planes.push_back(std::move(plane)); > > > -- > > > 2.34.1 > > > >
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index a72ef64d..e57cb131 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1402,18 +1402,27 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) std::vector<FrameBuffer::Plane> planes; for (unsigned int nplane = 0; nplane < numPlanes; nplane++) { - UniqueFD fd = exportDmabufFd(buf.index, nplane); - if (!fd.isValid()) - return nullptr; - FrameBuffer::Plane plane; - plane.fd = SharedFD(std::move(fd)); - /* - * V4L2 API doesn't provide dmabuf offset information of plane. - * Set 0 as a placeholder offset. - * \todo Set the right offset once V4L2 API provides a way. - */ - plane.offset = 0; + + if (buf.type == V4L2_BUF_TYPE_META_CAPTURE) { + /* + * Dmabuf fd is not exported for metadata, so store + * the offset from the querybuf call and this device's fd. + */ + plane.fd = SharedFD(this->fd()); + plane.offset = buf.m.offset; + } else { + UniqueFD fd = exportDmabufFd(buf.index, nplane); + if (!fd.isValid()) + return nullptr; + plane.fd = SharedFD(std::move(fd)); + /* + * V4L2 API doesn't provide dmabuf offset information of plane. + * Set 0 as a placeholder offset. + * \todo Set the right offset once V4L2 API provides a way. + */ + plane.offset = 0; + } plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; planes.push_back(std::move(plane));
To perform a memory mapping using mmap, the MappedFrameBuffer class needs the plane offset and file descriptor information of the frame buffer's plane(s). This information is provided in the response to REQBUF, which happens during buffer allocation. Store the plane offset and file descriptor information in the buffer's plane at the time of allocation. Currently, there is a metadata buffer type (metadata format UVCH) that does not support exporting buffers using EXPBUF, so this should only be done if the buffer type is metadata capture. Signed-off-by: Gabby George <gabbymg94@gmail.com> --- src/libcamera/v4l2_videodevice.cpp | 31 +++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-)