| Message ID | 20210816043138.957984-9-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > V4L2VideDevice::CreateBuffer() creates the same number of s/CreateBuffer/createBuffer/ > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > format single is single-planar format, the created number of > FrameBuffer::Planes is 1. > It should rather create the same number of FrameBuffer::Planes as the > color format planes. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index e767ec84..d0aeb614 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -136,11 +136,13 @@ private: > private: > struct Plane { > Plane(const FrameBuffer::Plane &plane) > - : fd(plane.fd.fd()), length(plane.length) > + : fd(plane.fd.fd()), offset(plane.offset), > + length(plane.length) > { > } > > int fd; > + unsigned int offset; > unsigned int length; > }; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index ce60dff6..549418c8 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -25,6 +25,7 @@ > > #include <libcamera/file_descriptor.h> > > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/media_object.h" > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > for (unsigned int i = 0; i < planes.size(); i++) > if (planes_[i].fd != planes[i].fd.fd() || > + planes_[i].offset != planes_[i].offset || > planes_[i].length != planes[i].length) > return false; > return true; > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) It would be good to capture the fact that the FrameBuffer planes now match the colour planes and not the V4L2 planes. This function is private and isn't documented, so we can't handle it here, and should instead document this in V4L2VideoDevice::allocateBuffers() and V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that state * The number of planes and the plane sizes for the allocation are determined * by the currently active format on the device as set by setFormat(). It could be extended to * The number of planes and their offsets and sizes are determined by the * currently active format on the device as set by setFormat(). They do not map * to the V4L2 buffer planes, but to colour planes of the pixel format. For * instance, if the active format is formats::NV12, the allocated FrameBuffer * instances will have two planes, for the luma and chroma components, * regardless of whether the device uses V4L2_PIX_FMT_NV12 or * V4L2_PIX_FMT_NV12M. > > FrameBuffer::Plane plane; > plane.fd = std::move(fd); > - plane.length = multiPlanar ? > - buf.m.planes[nplane].length : buf.length; > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; The data_offset field defined by V4L2 is, unfortunately, not the offset in the dmabuf, but the offset of the data from the start of the plane. It's used by codec drivers to indicate the size of the header stored at the beginning of the plane. The data_offset is actually contained in the length. I think we can ignore the data offset reported by V4L2, until the V4L2 API is expanded with a "real" data offset. plane.offset can be hardcoded to 0. The change to operator== could also probably be dropped. > > planes.push_back(std::move(plane)); > } > > + V4L2DeviceFormat format{}; > + ret = getFormat(&format); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Failed to get format: " << strerror(-ret); > + return nullptr; > + } > + A comment to explain what's going on would be useful. > + const auto &info = PixelFormatInfo::info(format.fourcc); > + if (info.isValid() && info.numPlanes() != numPlanes) { If info isn't valid, should we return nullptr ? > + ASSERT(numPlanes == 1u); > + planes.resize(info.numPlanes()); > + const FileDescriptor &fd = planes[0].fd; > + size_t offset = 0; > + for (size_t i = 0; i < info.numPlanes(); ++i) { > + planes[i].fd = fd; > + planes[i].offset = offset; > + planes[i].length = info.frameSize(format.size); Isn't it the plane size that we need here, not the full frame size ? Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, will return the total size of the image, including both colour planes. There's also one change missing. You're addressing the buffer allocation side, but not the buffer queuing side. Now that an NV12 FrameBuffer has two planes, we need to translate from two colour planes to one V4L2 plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > + offset += planes[i].length; > + } > + } > + > return std::make_unique<FrameBuffer>(std::move(planes)); > } >
Le lundi 16 août 2021 à 13:31 +0900, Hirokazu Honda a écrit : > V4L2VideDevice::CreateBuffer() creates the same number of > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > format single is single-planar format, the created number of > FrameBuffer::Planes is 1. > It should rather create the same number of FrameBuffer::Planes as the > color format planes. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index e767ec84..d0aeb614 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -136,11 +136,13 @@ private: > private: > struct Plane { > Plane(const FrameBuffer::Plane &plane) > - : fd(plane.fd.fd()), length(plane.length) > + : fd(plane.fd.fd()), offset(plane.offset), > + length(plane.length) > { > } > > int fd; > + unsigned int offset; > unsigned int length; > }; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index ce60dff6..549418c8 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -25,6 +25,7 @@ > > #include <libcamera/file_descriptor.h> > > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/media_object.h" > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > for (unsigned int i = 0; i < planes.size(); i++) > if (planes_[i].fd != planes[i].fd.fd() || > + planes_[i].offset != planes_[i].offset || > planes_[i].length != planes[i].length) > return false; > return true; > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > FrameBuffer::Plane plane; > plane.fd = std::move(fd); > - plane.length = multiPlanar ? > - buf.m.planes[nplane].length : buf.length; > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; There might be a issue here, please carefully review. In V4L2. buf.length (or plane.length in mplane) includes data_offset but in your implementation you add it again. We need to document the semantic we want in libcamera and make sure its all proper. > > planes.push_back(std::move(plane)); > } > > + V4L2DeviceFormat format{}; > + ret = getFormat(&format); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Failed to get format: " << strerror(-ret); > + return nullptr; > + } > + > + const auto &info = PixelFormatInfo::info(format.fourcc); > + if (info.isValid() && info.numPlanes() != numPlanes) { > + ASSERT(numPlanes == 1u); > + planes.resize(info.numPlanes()); > + const FileDescriptor &fd = planes[0].fd; > + size_t offset = 0; > + for (size_t i = 0; i < info.numPlanes(); ++i) { > + planes[i].fd = fd; > + planes[i].offset = offset; > + planes[i].length = info.frameSize(format.size); > + offset += planes[i].length; > + } > + } > + > return std::make_unique<FrameBuffer>(std::move(planes)); > } >
Hi Nicolas, On Wed, Aug 18, 2021 at 09:47:36AM -0400, Nicolas Dufresne wrote: > Le lundi 16 août 2021 à 13:31 +0900, Hirokazu Honda a écrit : > > V4L2VideDevice::CreateBuffer() creates the same number of > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > format single is single-planar format, the created number of > > FrameBuffer::Planes is 1. > > It should rather create the same number of FrameBuffer::Planes as the > > color format planes. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index e767ec84..d0aeb614 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -136,11 +136,13 @@ private: > > private: > > struct Plane { > > Plane(const FrameBuffer::Plane &plane) > > - : fd(plane.fd.fd()), length(plane.length) > > + : fd(plane.fd.fd()), offset(plane.offset), > > + length(plane.length) > > { > > } > > > > int fd; > > + unsigned int offset; > > unsigned int length; > > }; > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index ce60dff6..549418c8 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -25,6 +25,7 @@ > > > > #include <libcamera/file_descriptor.h> > > > > +#include "libcamera/internal/formats.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/media_object.h" > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > for (unsigned int i = 0; i < planes.size(); i++) > > if (planes_[i].fd != planes[i].fd.fd() || > > + planes_[i].offset != planes_[i].offset || > > planes_[i].length != planes[i].length) > > return false; > > return true; > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > FrameBuffer::Plane plane; > > plane.fd = std::move(fd); > > - plane.length = multiPlanar ? > > - buf.m.planes[nplane].length : buf.length; > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > There might be a issue here, please carefully review. In V4L2. buf.length (or > plane.length in mplane) includes data_offset but in your implementation you add > it again. We need to document the semantic we want in libcamera and make sure > its all proper. As mentioned in my review of this patch, V4L2 indeed uses a different semantic, which doesn't match what we want to use. The V4L2 data_offset field is really meant for codecs to indicate the header size. Reusing it as-is would be an API abuse in my opinion. The DRM semantic is better, and one day we may extend V4L2 to support it too, and finally be able to support NV12M stored in a single dmabuf. Until then, I recommend ignoring the V4L2 data_offset completely, and mapping the libcamera formats::NV12 to V4L2_PIX_FMT_NV12, with a translation in this class between the 2 colour planes in the FrameBuffer and the single buffer plane in the V4L2 buffer. > > > > planes.push_back(std::move(plane)); > > } > > > > + V4L2DeviceFormat format{}; > > + ret = getFormat(&format); > > + if (ret < 0) { > > + LOG(V4L2, Error) > > + << "Failed to get format: " << strerror(-ret); > > + return nullptr; > > + } > > + > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > + ASSERT(numPlanes == 1u); > > + planes.resize(info.numPlanes()); > > + const FileDescriptor &fd = planes[0].fd; > > + size_t offset = 0; > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > + planes[i].fd = fd; > > + planes[i].offset = offset; > > + planes[i].length = info.frameSize(format.size); > > + offset += planes[i].length; > > + } > > + } > > + > > return std::make_unique<FrameBuffer>(std::move(planes)); > > } > >
Hi Laurent, thank you for reviewing. On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > > V4L2VideDevice::CreateBuffer() creates the same number of > > s/CreateBuffer/createBuffer/ > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > format single is single-planar format, the created number of > > FrameBuffer::Planes is 1. > > It should rather create the same number of FrameBuffer::Planes as the > > color format planes. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index e767ec84..d0aeb614 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -136,11 +136,13 @@ private: > > private: > > struct Plane { > > Plane(const FrameBuffer::Plane &plane) > > - : fd(plane.fd.fd()), length(plane.length) > > + : fd(plane.fd.fd()), offset(plane.offset), > > + length(plane.length) > > { > > } > > > > int fd; > > + unsigned int offset; > > unsigned int length; > > }; > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index ce60dff6..549418c8 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -25,6 +25,7 @@ > > > > #include <libcamera/file_descriptor.h> > > > > +#include "libcamera/internal/formats.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/media_object.h" > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > for (unsigned int i = 0; i < planes.size(); i++) > > if (planes_[i].fd != planes[i].fd.fd() || > > + planes_[i].offset != planes_[i].offset || > > planes_[i].length != planes[i].length) > > return false; > > return true; > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > It would be good to capture the fact that the FrameBuffer planes now > match the colour planes and not the V4L2 planes. This function is > private and isn't documented, so we can't handle it here, and should > instead document this in V4L2VideoDevice::allocateBuffers() and > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that > state > > * The number of planes and the plane sizes for the allocation are determined > * by the currently active format on the device as set by setFormat(). > > It could be extended to > > * The number of planes and their offsets and sizes are determined by the > * currently active format on the device as set by setFormat(). They do not map > * to the V4L2 buffer planes, but to colour planes of the pixel format. For > * instance, if the active format is formats::NV12, the allocated FrameBuffer > * instances will have two planes, for the luma and chroma components, > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > * V4L2_PIX_FMT_NV12M. > > > > > FrameBuffer::Plane plane; > > plane.fd = std::move(fd); > > - plane.length = multiPlanar ? > > - buf.m.planes[nplane].length : buf.length; > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > The data_offset field defined by V4L2 is, unfortunately, not the offset > in the dmabuf, but the offset of the data from the start of the plane. > It's used by codec drivers to indicate the size of the header stored at > the beginning of the plane. The data_offset is actually contained in the > length. > > I think we can ignore the data offset reported by V4L2, until the V4L2 > API is expanded with a "real" data offset. plane.offset can be hardcoded > to 0. The change to operator== could also probably be dropped. > Yeah, I have faced the problem in developing chromium video accelerated codecs. Although I think setting offset to 0 is not correct either.. but no way of avoiding problem here. > > > > planes.push_back(std::move(plane)); > > } > > > > + V4L2DeviceFormat format{}; > > + ret = getFormat(&format); > > + if (ret < 0) { > > + LOG(V4L2, Error) > > + << "Failed to get format: " << strerror(-ret); > > + return nullptr; > > + } > > + > > A comment to explain what's going on would be useful. > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > If info isn't valid, should we return nullptr ? > > > + ASSERT(numPlanes == 1u); > > + planes.resize(info.numPlanes()); > > + const FileDescriptor &fd = planes[0].fd; > > + size_t offset = 0; > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > + planes[i].fd = fd; > > + planes[i].offset = offset; > > + planes[i].length = info.frameSize(format.size); > > Isn't it the plane size that we need here, not the full frame size ? > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, > will return the total size of the image, including both colour planes. > > There's also one change missing. You're addressing the buffer allocation > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has > two planes, we need to translate from two colour planes to one V4L2 > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > I am confused thanks to various "Plane" structures. In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it? FrameMetadata::Plane is used to fill other v4l2_buffer variables. What shall I fix in queueBuffer()? Best Regards, -Hiro > > + offset += planes[i].length; > > + } > > + } > > + > > return std::make_unique<FrameBuffer>(std::move(planes)); > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote: > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote: > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > > > V4L2VideDevice::CreateBuffer() creates the same number of > > > > s/CreateBuffer/createBuffer/ > > > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > > format single is single-planar format, the created number of > > > FrameBuffer::Planes is 1. > > > It should rather create the same number of FrameBuffer::Planes as the > > > color format planes. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > index e767ec84..d0aeb614 100644 > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > @@ -136,11 +136,13 @@ private: > > > private: > > > struct Plane { > > > Plane(const FrameBuffer::Plane &plane) > > > - : fd(plane.fd.fd()), length(plane.length) > > > + : fd(plane.fd.fd()), offset(plane.offset), > > > + length(plane.length) > > > { > > > } > > > > > > int fd; > > > + unsigned int offset; > > > unsigned int length; > > > }; > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index ce60dff6..549418c8 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -25,6 +25,7 @@ > > > > > > #include <libcamera/file_descriptor.h> > > > > > > +#include "libcamera/internal/formats.h" > > > #include "libcamera/internal/media_device.h" > > > #include "libcamera/internal/media_object.h" > > > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > > for (unsigned int i = 0; i < planes.size(); i++) > > > if (planes_[i].fd != planes[i].fd.fd() || > > > + planes_[i].offset != planes_[i].offset || > > > planes_[i].length != planes[i].length) > > > return false; > > > return true; > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > It would be good to capture the fact that the FrameBuffer planes now > > match the colour planes and not the V4L2 planes. This function is > > private and isn't documented, so we can't handle it here, and should > > instead document this in V4L2VideoDevice::allocateBuffers() and > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that > > state > > > > * The number of planes and the plane sizes for the allocation are determined > > * by the currently active format on the device as set by setFormat(). > > > > It could be extended to > > > > * The number of planes and their offsets and sizes are determined by the > > * currently active format on the device as set by setFormat(). They do not map > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > * instance, if the active format is formats::NV12, the allocated FrameBuffer > > * instances will have two planes, for the luma and chroma components, > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > * V4L2_PIX_FMT_NV12M. > > > > > > > > FrameBuffer::Plane plane; > > > plane.fd = std::move(fd); > > > - plane.length = multiPlanar ? > > > - buf.m.planes[nplane].length : buf.length; > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > The data_offset field defined by V4L2 is, unfortunately, not the offset > > in the dmabuf, but the offset of the data from the start of the plane. > > It's used by codec drivers to indicate the size of the header stored at > > the beginning of the plane. The data_offset is actually contained in the > > length. > > > > I think we can ignore the data offset reported by V4L2, until the V4L2 > > API is expanded with a "real" data offset. plane.offset can be hardcoded > > to 0. The change to operator== could also probably be dropped. > > Yeah, I have faced the problem in developing chromium video accelerated codecs. > Although I think setting offset to 0 is not correct either.. but no > way of avoiding problem here. Let's revisit this part of the code once V4L2 gets proper support for offsets :-) > > > > > > planes.push_back(std::move(plane)); > > > } > > > > > > + V4L2DeviceFormat format{}; > > > + ret = getFormat(&format); > > > + if (ret < 0) { > > > + LOG(V4L2, Error) > > > + << "Failed to get format: " << strerror(-ret); > > > + return nullptr; > > > + } > > > + > > > > A comment to explain what's going on would be useful. > > > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > > > If info isn't valid, should we return nullptr ? > > > > > + ASSERT(numPlanes == 1u); > > > + planes.resize(info.numPlanes()); > > > + const FileDescriptor &fd = planes[0].fd; > > > + size_t offset = 0; > > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > > + planes[i].fd = fd; > > > + planes[i].offset = offset; > > > + planes[i].length = info.frameSize(format.size); > > > > Isn't it the plane size that we need here, not the full frame size ? > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, > > will return the total size of the image, including both colour planes. > > > > There's also one change missing. You're addressing the buffer allocation > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has > > two planes, we need to translate from two colour planes to one V4L2 > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > > I am confused thanks to various "Plane" structures. > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it? > FrameMetadata::Plane is used to fill other v4l2_buffer variables. > What shall I fix in queueBuffer()? A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour planes, with the same dmabuf for both: planes[0].fd = dmabuf; planes[0].offset = 0; planes[0].length = stride * height; planes[1].fd = dmabuf; planes[1].offset = stride * height; planes[1].length = stride * height / 2; This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice, V4L2 will expect a single buffer plane, with v4l2Planes[0].m.fd = dmabuf; Additionally, for output buffers, bytesused and length also need to be set, but we may not need to handle that right away. V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the two colour planes in the FrameBuffer to the single V4L2 buffer plane. The translation is fairly easy, as it only requires reducing the number of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12 isn't available anywhere, so we will need to add this information somewhere). We however needs to verify that the FrameBuffer is suitable for V4L2, and check that - planes[0].fd == planes[1].fd - planes[0].offset == 0 - planes[1].offset == planes[0].length and possibly also check the length. If the buffer isn't suitable, V4L2VideoDevice::queueBuffer() should return an error. > > > + offset += planes[i].length; > > > + } > > > + } > > > + > > > return std::make_unique<FrameBuffer>(std::move(planes)); > > > } > > >
On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote: > > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote: > > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > > > > V4L2VideDevice::CreateBuffer() creates the same number of > > > > > > s/CreateBuffer/createBuffer/ > > > > > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > > > format single is single-planar format, the created number of > > > > FrameBuffer::Planes is 1. > > > > It should rather create the same number of FrameBuffer::Planes as the > > > > color format planes. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > index e767ec84..d0aeb614 100644 > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > @@ -136,11 +136,13 @@ private: > > > > private: > > > > struct Plane { > > > > Plane(const FrameBuffer::Plane &plane) > > > > - : fd(plane.fd.fd()), length(plane.length) > > > > + : fd(plane.fd.fd()), offset(plane.offset), > > > > + length(plane.length) > > > > { > > > > } > > > > > > > > int fd; > > > > + unsigned int offset; > > > > unsigned int length; > > > > }; > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index ce60dff6..549418c8 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -25,6 +25,7 @@ > > > > > > > > #include <libcamera/file_descriptor.h> > > > > > > > > +#include "libcamera/internal/formats.h" > > > > #include "libcamera/internal/media_device.h" > > > > #include "libcamera/internal/media_object.h" > > > > > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > > > > for (unsigned int i = 0; i < planes.size(); i++) > > > > if (planes_[i].fd != planes[i].fd.fd() || > > > > + planes_[i].offset != planes_[i].offset || > > > > planes_[i].length != planes[i].length) > > > > return false; > > > > return true; > > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > > > It would be good to capture the fact that the FrameBuffer planes now > > > match the colour planes and not the V4L2 planes. This function is > > > private and isn't documented, so we can't handle it here, and should > > > instead document this in V4L2VideoDevice::allocateBuffers() and > > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that > > > state > > > > > > * The number of planes and the plane sizes for the allocation are determined > > > * by the currently active format on the device as set by setFormat(). > > > > > > It could be extended to > > > > > > * The number of planes and their offsets and sizes are determined by the > > > * currently active format on the device as set by setFormat(). They do not map > > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > > * instance, if the active format is formats::NV12, the allocated FrameBuffer > > > * instances will have two planes, for the luma and chroma components, > > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > > * V4L2_PIX_FMT_NV12M. > > > > > > > > > > > FrameBuffer::Plane plane; > > > > plane.fd = std::move(fd); > > > > - plane.length = multiPlanar ? > > > > - buf.m.planes[nplane].length : buf.length; > > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > > > The data_offset field defined by V4L2 is, unfortunately, not the offset > > > in the dmabuf, but the offset of the data from the start of the plane. > > > It's used by codec drivers to indicate the size of the header stored at > > > the beginning of the plane. The data_offset is actually contained in the > > > length. > > > > > > I think we can ignore the data offset reported by V4L2, until the V4L2 > > > API is expanded with a "real" data offset. plane.offset can be hardcoded > > > to 0. The change to operator== could also probably be dropped. > > > > Yeah, I have faced the problem in developing chromium video accelerated codecs. > > Although I think setting offset to 0 is not correct either.. but no > > way of avoiding problem here. > > Let's revisit this part of the code once V4L2 gets proper support for > offsets :-) > We'll probably have to carry a downstream patch to handle them initially... Would it be possible to at least make it easy to do that until proper V4L2 support gets merged? (v4l2_ext API is being worked on, but last time I checked it was stuck in review, without any comments...) > > > > > > > > planes.push_back(std::move(plane)); > > > > } > > > > > > > > + V4L2DeviceFormat format{}; > > > > + ret = getFormat(&format); > > > > + if (ret < 0) { > > > > + LOG(V4L2, Error) > > > > + << "Failed to get format: " << strerror(-ret); > > > > + return nullptr; > > > > + } > > > > + > > > > > > A comment to explain what's going on would be useful. > > > > > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > > > > > If info isn't valid, should we return nullptr ? > > > > > > > + ASSERT(numPlanes == 1u); > > > > + planes.resize(info.numPlanes()); > > > > + const FileDescriptor &fd = planes[0].fd; > > > > + size_t offset = 0; > > > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > > > + planes[i].fd = fd; > > > > + planes[i].offset = offset; > > > > + planes[i].length = info.frameSize(format.size); > > > > > > Isn't it the plane size that we need here, not the full frame size ? > > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, > > > will return the total size of the image, including both colour planes. > > > > > > There's also one change missing. You're addressing the buffer allocation > > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has > > > two planes, we need to translate from two colour planes to one V4L2 > > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > > > > I am confused thanks to various "Plane" structures. > > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it? > > FrameMetadata::Plane is used to fill other v4l2_buffer variables. > > What shall I fix in queueBuffer()? > > A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour > planes, with the same dmabuf for both: > > planes[0].fd = dmabuf; > planes[0].offset = 0; > planes[0].length = stride * height; > planes[1].fd = dmabuf; > planes[1].offset = stride * height; > planes[1].length = stride * height / 2; > > This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike > V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice, > V4L2 will expect a single buffer plane, with > > v4l2Planes[0].m.fd = dmabuf; > > Additionally, for output buffers, bytesused and length also need to be > set, but we may not need to handle that right away. > > V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the > two colour planes in the FrameBuffer to the single V4L2 buffer plane. > The translation is fairly easy, as it only requires reducing the number > of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12 > isn't available anywhere, so we will need to add this information > somewhere). We however needs to verify that the FrameBuffer is suitable > for V4L2, and check that > > - planes[0].fd == planes[1].fd planes[0].fd != planes[1].fd doesn't always imply that they refer to a different DMA-buf. I'd skip this check (or just emit a warning) and rely on checking whether the offset matches. Generally the two common patterns in V4L2 are: 1) single plane with 1 DMA-buf for all color planes 2) multiplane with 1 DMA-buf for _each_ color plane, with zero offset within each I haven't seen any other case in practice, so it should be safe to simply check based on the offset. The ext API will solve this by moving the DMA-buf and offset check to the kernel. Best regards, Tomasz > - planes[0].offset == 0 > - planes[1].offset == planes[0].length > > and possibly also check the length. If the buffer isn't suitable, > V4L2VideoDevice::queueBuffer() should return an error. > > > > > + offset += planes[i].length; > > > > + } > > > > + } > > > > + > > > > return std::make_unique<FrameBuffer>(std::move(planes)); > > > > } > > > > > > -- > Regards, > > Laurent Pinchart
Hi Tomasz, On Thu, Aug 26, 2021 at 02:14:45PM +0900, Tomasz Figa wrote: > On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart wrote: > > On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote: > > > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote: > > > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > > > > > V4L2VideDevice::CreateBuffer() creates the same number of > > > > > > > > s/CreateBuffer/createBuffer/ > > > > > > > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > > > > format single is single-planar format, the created number of > > > > > FrameBuffer::Planes is 1. > > > > > It should rather create the same number of FrameBuffer::Planes as the > > > > > color format planes. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > > > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > > index e767ec84..d0aeb614 100644 > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > > @@ -136,11 +136,13 @@ private: > > > > > private: > > > > > struct Plane { > > > > > Plane(const FrameBuffer::Plane &plane) > > > > > - : fd(plane.fd.fd()), length(plane.length) > > > > > + : fd(plane.fd.fd()), offset(plane.offset), > > > > > + length(plane.length) > > > > > { > > > > > } > > > > > > > > > > int fd; > > > > > + unsigned int offset; > > > > > unsigned int length; > > > > > }; > > > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > index ce60dff6..549418c8 100644 > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > @@ -25,6 +25,7 @@ > > > > > > > > > > #include <libcamera/file_descriptor.h> > > > > > > > > > > +#include "libcamera/internal/formats.h" > > > > > #include "libcamera/internal/media_device.h" > > > > > #include "libcamera/internal/media_object.h" > > > > > > > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > > > > > > for (unsigned int i = 0; i < planes.size(); i++) > > > > > if (planes_[i].fd != planes[i].fd.fd() || > > > > > + planes_[i].offset != planes_[i].offset || > > > > > planes_[i].length != planes[i].length) > > > > > return false; > > > > > return true; > > > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > > > > > It would be good to capture the fact that the FrameBuffer planes now > > > > match the colour planes and not the V4L2 planes. This function is > > > > private and isn't documented, so we can't handle it here, and should > > > > instead document this in V4L2VideoDevice::allocateBuffers() and > > > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that > > > > state > > > > > > > > * The number of planes and the plane sizes for the allocation are determined > > > > * by the currently active format on the device as set by setFormat(). > > > > > > > > It could be extended to > > > > > > > > * The number of planes and their offsets and sizes are determined by the > > > > * currently active format on the device as set by setFormat(). They do not map > > > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > > > * instance, if the active format is formats::NV12, the allocated FrameBuffer > > > > * instances will have two planes, for the luma and chroma components, > > > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > > > * V4L2_PIX_FMT_NV12M. > > > > > > > > > > > > > > FrameBuffer::Plane plane; > > > > > plane.fd = std::move(fd); > > > > > - plane.length = multiPlanar ? > > > > > - buf.m.planes[nplane].length : buf.length; > > > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > > > > > The data_offset field defined by V4L2 is, unfortunately, not the offset > > > > in the dmabuf, but the offset of the data from the start of the plane. > > > > It's used by codec drivers to indicate the size of the header stored at > > > > the beginning of the plane. The data_offset is actually contained in the > > > > length. > > > > > > > > I think we can ignore the data offset reported by V4L2, until the V4L2 > > > > API is expanded with a "real" data offset. plane.offset can be hardcoded > > > > to 0. The change to operator== could also probably be dropped. > > > > > > Yeah, I have faced the problem in developing chromium video accelerated codecs. > > > Although I think setting offset to 0 is not correct either.. but no > > > way of avoiding problem here. > > > > Let's revisit this part of the code once V4L2 gets proper support for > > offsets :-) > > We'll probably have to carry a downstream patch to handle them > initially... Would it be possible to at least make it easy to do that > until proper V4L2 support gets merged? > > (v4l2_ext API is being worked on, but last time I checked it was stuck > in review, without any comments...) If there's significant progress with this development we can carry a downstream patch to avoid waiting for the API to be merged upstream, but it indeed seems stuck. What would be your use case for such a downstream patch though ? We would need drivers using the API to make it useful in libcamera. > > > > > > > > > > planes.push_back(std::move(plane)); > > > > > } > > > > > > > > > > + V4L2DeviceFormat format{}; > > > > > + ret = getFormat(&format); > > > > > + if (ret < 0) { > > > > > + LOG(V4L2, Error) > > > > > + << "Failed to get format: " << strerror(-ret); > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > > > > A comment to explain what's going on would be useful. > > > > > > > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > > > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > > > > > > > If info isn't valid, should we return nullptr ? > > > > > > > > > + ASSERT(numPlanes == 1u); > > > > > + planes.resize(info.numPlanes()); > > > > > + const FileDescriptor &fd = planes[0].fd; > > > > > + size_t offset = 0; > > > > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > > > > + planes[i].fd = fd; > > > > > + planes[i].offset = offset; > > > > > + planes[i].length = info.frameSize(format.size); > > > > > > > > Isn't it the plane size that we need here, not the full frame size ? > > > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, > > > > will return the total size of the image, including both colour planes. > > > > > > > > There's also one change missing. You're addressing the buffer allocation > > > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has > > > > two planes, we need to translate from two colour planes to one V4L2 > > > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > > > > > > I am confused thanks to various "Plane" structures. > > > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it? > > > FrameMetadata::Plane is used to fill other v4l2_buffer variables. > > > What shall I fix in queueBuffer()? > > > > A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour > > planes, with the same dmabuf for both: > > > > planes[0].fd = dmabuf; > > planes[0].offset = 0; > > planes[0].length = stride * height; > > planes[1].fd = dmabuf; > > planes[1].offset = stride * height; > > planes[1].length = stride * height / 2; > > > > This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike > > V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice, > > V4L2 will expect a single buffer plane, with > > > > v4l2Planes[0].m.fd = dmabuf; > > > > Additionally, for output buffers, bytesused and length also need to be > > set, but we may not need to handle that right away. > > > > V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the > > two colour planes in the FrameBuffer to the single V4L2 buffer plane. > > The translation is fairly easy, as it only requires reducing the number > > of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12 > > isn't available anywhere, so we will need to add this information > > somewhere). We however needs to verify that the FrameBuffer is suitable > > for V4L2, and check that > > > > - planes[0].fd == planes[1].fd > > planes[0].fd != planes[1].fd doesn't always imply that they refer to a > different DMA-buf. I'd skip this check (or just emit a warning) and > rely on checking whether the offset matches. We can check the inode number with fstat() (I've tested that by dupping a dmabuf fd, and I get the same inode). It's probably overkill for now, but will likely be done at some point. The reason why I'd like this check to stay is to catch userspace that would give use different fds (whether or not they reference the same dmabuf), and then decide how to handle them. > Generally the two common patterns in V4L2 are: > 1) single plane with 1 DMA-buf for all color planes > 2) multiplane with 1 DMA-buf for _each_ color plane, with zero offset > within each I'd say that more than this being a common pattern, it's the only thing that V4L2 supports upstream :-) > I haven't seen any other case in practice, so it should be safe to > simply check based on the offset. The ext API will solve this by > moving the DMA-buf and offset check to the kernel. > > > - planes[0].offset == 0 > > - planes[1].offset == planes[0].length > > > > and possibly also check the length. If the buffer isn't suitable, > > V4L2VideoDevice::queueBuffer() should return an error. > > > > > > > + offset += planes[i].length; > > > > > + } > > > > > + } > > > > > + > > > > > return std::make_unique<FrameBuffer>(std::move(planes)); > > > > > } > > > > >
Le jeudi 26 août 2021 à 14:08 +0300, Laurent Pinchart a écrit : > > We'll probably have to carry a downstream patch to handle them > > initially... Would it be possible to at least make it easy to do that > > until proper V4L2 support gets merged? > > > > (v4l2_ext API is being worked on, but last time I checked it was stuck > > in review, without any comments...) > > If there's significant progress with this development we can carry a > downstream patch to avoid waiting for the API to be merged upstream, but > it indeed seems stuck. What would be your use case for such a downstream > patch though ? We would need drivers using the API to make it useful in > libcamera. I've come across a potential usage of this, but that was too much work to fix back then. The thingy was a very low power camera viewer (in a grid). We wanted to get all the requested frames into the same DMABuf, for that we could have had to be able to import that DMABuf with customized offsets and strides, so that the camera (if capable) would write the frame at the appropriate location in the frame. With 100% of the v4l2 driver not accepting custom strides, and this import limitation, we gave up, and simply reduced the grid size due to memory limitation. Nicolas
Hi Nicolas, On Thu, Aug 26, 2021 at 10:31:41AM -0400, Nicolas Dufresne wrote: > Le jeudi 26 août 2021 à 14:08 +0300, Laurent Pinchart a écrit : > > > We'll probably have to carry a downstream patch to handle them > > > initially... Would it be possible to at least make it easy to do that > > > until proper V4L2 support gets merged? > > > > > > (v4l2_ext API is being worked on, but last time I checked it was stuck > > > in review, without any comments...) > > > > If there's significant progress with this development we can carry a > > downstream patch to avoid waiting for the API to be merged upstream, but > > it indeed seems stuck. What would be your use case for such a downstream > > patch though ? We would need drivers using the API to make it useful in > > libcamera. > > I've come across a potential usage of this, but that was too much work to fix > back then. The thingy was a very low power camera viewer (in a grid). We wanted > to get all the requested frames into the same DMABuf, for that we could have had > to be able to import that DMABuf with customized offsets and strides, so that > the camera (if capable) would write the frame at the appropriate location in the > frame. With 100% of the v4l2 driver not accepting custom strides, and this > import limitation, we gave up, and simply reduced the grid size due to memory > limitation. Interesting use case. I was however more wondering if Tomasz had use cases in existing drivers, possibly drivers that carry out-of-tree patches to support a data offset in V4L2. I fully agree it would be nice to have data offsets in V4L2, but what I wonder if whether that would only be used in the future once it's available, or if there are devices that already use this feature in a downstream kernel.
Le jeu. 26 août 2021 10 h 48, Laurent Pinchart < laurent.pinchart@ideasonboard.com> a écrit : > Hi Nicolas, > > On Thu, Aug 26, 2021 at 10:31:41AM -0400, Nicolas Dufresne wrote: > > Le jeudi 26 août 2021 à 14:08 +0300, Laurent Pinchart a écrit : > > > > We'll probably have to carry a downstream patch to handle them > > > > initially... Would it be possible to at least make it easy to do that > > > > until proper V4L2 support gets merged? > > > > > > > > (v4l2_ext API is being worked on, but last time I checked it was > stuck > > > > in review, without any comments...) > > > > > > If there's significant progress with this development we can carry a > > > downstream patch to avoid waiting for the API to be merged upstream, > but > > > it indeed seems stuck. What would be your use case for such a > downstream > > > patch though ? We would need drivers using the API to make it useful in > > > libcamera. > > > > I've come across a potential usage of this, but that was too much work > to fix > > back then. The thingy was a very low power camera viewer (in a grid). We > wanted > > to get all the requested frames into the same DMABuf, for that we could > have had > > to be able to import that DMABuf with customized offsets and strides, so > that > > the camera (if capable) would write the frame at the appropriate > location in the > > frame. With 100% of the v4l2 driver not accepting custom strides, and > this > > import limitation, we gave up, and simply reduced the grid size due to > memory > > limitation. > > Interesting use case. I was however more wondering if Tomasz had use > cases in existing drivers, possibly drivers that carry out-of-tree > patches to support a data offset in V4L2. I fully agree it would be nice > to have data offsets in V4L2, but what I wonder if whether that would > only be used in the future once it's available, or if there are devices > that already use this feature in a downstream kernel. > I believe in Xilinx tree, data_offset is used for import too. At least this is how this apparently invalid usage made it into GStreamer. If my memory is right, this method was denied by mainline as it would be incompatible with drivers that uses the buffer from the start to data_offset to store some state. While unusual, some codec drivers apparently store motion vectors there. Iirc it might be the case for Qualcomm Venus VP8 decoder. My memory is a bit rusted, so all this needs to be checked. I never really been completely clear why is this method was invalid, it allow per plane offset, hence allow importing single DMAbuf into drivers to that only support true MPLANE. > -- > Regards, > > Laurent Pinchart >
On Thu, Aug 26, 2021 at 8:09 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Tomasz, > > On Thu, Aug 26, 2021 at 02:14:45PM +0900, Tomasz Figa wrote: > > On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart wrote: > > > On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote: > > > > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote: > > > > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > > > > > > V4L2VideDevice::CreateBuffer() creates the same number of > > > > > > > > > > s/CreateBuffer/createBuffer/ > > > > > > > > > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > > > > > format single is single-planar format, the created number of > > > > > > FrameBuffer::Planes is 1. > > > > > > It should rather create the same number of FrameBuffer::Planes as the > > > > > > color format planes. > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > --- > > > > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > > > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > > > > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > > > index e767ec84..d0aeb614 100644 > > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > > > @@ -136,11 +136,13 @@ private: > > > > > > private: > > > > > > struct Plane { > > > > > > Plane(const FrameBuffer::Plane &plane) > > > > > > - : fd(plane.fd.fd()), length(plane.length) > > > > > > + : fd(plane.fd.fd()), offset(plane.offset), > > > > > > + length(plane.length) > > > > > > { > > > > > > } > > > > > > > > > > > > int fd; > > > > > > + unsigned int offset; > > > > > > unsigned int length; > > > > > > }; > > > > > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > > index ce60dff6..549418c8 100644 > > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > > @@ -25,6 +25,7 @@ > > > > > > > > > > > > #include <libcamera/file_descriptor.h> > > > > > > > > > > > > +#include "libcamera/internal/formats.h" > > > > > > #include "libcamera/internal/media_device.h" > > > > > > #include "libcamera/internal/media_object.h" > > > > > > > > > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > > > > > > > > for (unsigned int i = 0; i < planes.size(); i++) > > > > > > if (planes_[i].fd != planes[i].fd.fd() || > > > > > > + planes_[i].offset != planes_[i].offset || > > > > > > planes_[i].length != planes[i].length) > > > > > > return false; > > > > > > return true; > > > > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > > > > > > > It would be good to capture the fact that the FrameBuffer planes now > > > > > match the colour planes and not the V4L2 planes. This function is > > > > > private and isn't documented, so we can't handle it here, and should > > > > > instead document this in V4L2VideoDevice::allocateBuffers() and > > > > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that > > > > > state > > > > > > > > > > * The number of planes and the plane sizes for the allocation are determined > > > > > * by the currently active format on the device as set by setFormat(). > > > > > > > > > > It could be extended to > > > > > > > > > > * The number of planes and their offsets and sizes are determined by the > > > > > * currently active format on the device as set by setFormat(). They do not map > > > > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > > > > * instance, if the active format is formats::NV12, the allocated FrameBuffer > > > > > * instances will have two planes, for the luma and chroma components, > > > > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > > > > * V4L2_PIX_FMT_NV12M. > > > > > > > > > > > > > > > > > FrameBuffer::Plane plane; > > > > > > plane.fd = std::move(fd); > > > > > > - plane.length = multiPlanar ? > > > > > > - buf.m.planes[nplane].length : buf.length; > > > > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > > > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > > > > > > > The data_offset field defined by V4L2 is, unfortunately, not the offset > > > > > in the dmabuf, but the offset of the data from the start of the plane. > > > > > It's used by codec drivers to indicate the size of the header stored at > > > > > the beginning of the plane. The data_offset is actually contained in the > > > > > length. > > > > > > > > > > I think we can ignore the data offset reported by V4L2, until the V4L2 > > > > > API is expanded with a "real" data offset. plane.offset can be hardcoded > > > > > to 0. The change to operator== could also probably be dropped. > > > > > > > > Yeah, I have faced the problem in developing chromium video accelerated codecs. > > > > Although I think setting offset to 0 is not correct either.. but no > > > > way of avoiding problem here. > > > > > > Let's revisit this part of the code once V4L2 gets proper support for > > > offsets :-) > > > > We'll probably have to carry a downstream patch to handle them > > initially... Would it be possible to at least make it easy to do that > > until proper V4L2 support gets merged? > > > > (v4l2_ext API is being worked on, but last time I checked it was stuck > > in review, without any comments...) > > If there's significant progress with this development we can carry a > downstream patch to avoid waiting for the API to be merged upstream, but > it indeed seems stuck. What would be your use case for such a downstream > patch though ? We would need drivers using the API to make it useful in > libcamera. We carry driver patches to use data_offset for plane offset. The background is that for consistency with various graphics APIs, Chrome OS always passes planes as duped FDs + offset + stride, despite generally placing all the color planes inside the same DMA-buf. Also, the offset doesn't necessarily match V4L2 non-M formats to achieve more efficient line and/or plane alignment and so we often need to use M formats with a way to pass plane offsets. > > > > > > > > > > > > > planes.push_back(std::move(plane)); > > > > > > } > > > > > > > > > > > > + V4L2DeviceFormat format{}; > > > > > > + ret = getFormat(&format); > > > > > > + if (ret < 0) { > > > > > > + LOG(V4L2, Error) > > > > > > + << "Failed to get format: " << strerror(-ret); > > > > > > + return nullptr; > > > > > > + } > > > > > > + > > > > > > > > > > A comment to explain what's going on would be useful. > > > > > > > > > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > > > > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > > > > > > > > > If info isn't valid, should we return nullptr ? > > > > > > > > > > > + ASSERT(numPlanes == 1u); > > > > > > + planes.resize(info.numPlanes()); > > > > > > + const FileDescriptor &fd = planes[0].fd; > > > > > > + size_t offset = 0; > > > > > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > > > > > + planes[i].fd = fd; > > > > > > + planes[i].offset = offset; > > > > > > + planes[i].length = info.frameSize(format.size); > > > > > > > > > > Isn't it the plane size that we need here, not the full frame size ? > > > > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, > > > > > will return the total size of the image, including both colour planes. > > > > > > > > > > There's also one change missing. You're addressing the buffer allocation > > > > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has > > > > > two planes, we need to translate from two colour planes to one V4L2 > > > > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > > > > > > > > I am confused thanks to various "Plane" structures. > > > > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it? > > > > FrameMetadata::Plane is used to fill other v4l2_buffer variables. > > > > What shall I fix in queueBuffer()? > > > > > > A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour > > > planes, with the same dmabuf for both: > > > > > > planes[0].fd = dmabuf; > > > planes[0].offset = 0; > > > planes[0].length = stride * height; > > > planes[1].fd = dmabuf; > > > planes[1].offset = stride * height; > > > planes[1].length = stride * height / 2; > > > > > > This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike > > > V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice, > > > V4L2 will expect a single buffer plane, with > > > > > > v4l2Planes[0].m.fd = dmabuf; > > > > > > Additionally, for output buffers, bytesused and length also need to be > > > set, but we may not need to handle that right away. > > > > > > V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the > > > two colour planes in the FrameBuffer to the single V4L2 buffer plane. > > > The translation is fairly easy, as it only requires reducing the number > > > of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12 > > > isn't available anywhere, so we will need to add this information > > > somewhere). We however needs to verify that the FrameBuffer is suitable > > > for V4L2, and check that > > > > > > - planes[0].fd == planes[1].fd > > > > planes[0].fd != planes[1].fd doesn't always imply that they refer to a > > different DMA-buf. I'd skip this check (or just emit a warning) and > > rely on checking whether the offset matches. > > We can check the inode number with fstat() (I've tested that by dupping > a dmabuf fd, and I get the same inode). It's probably overkill for now, > but will likely be done at some point. The reason why I'd like this > check to stay is to catch userspace that would give use different fds > (whether or not they reference the same dmabuf), and then decide how to > handle them. > > > Generally the two common patterns in V4L2 are: > > 1) single plane with 1 DMA-buf for all color planes > > 2) multiplane with 1 DMA-buf for _each_ color plane, with zero offset > > within each > > I'd say that more than this being a common pattern, it's the only thing > that V4L2 supports upstream :-) > > > I haven't seen any other case in practice, so it should be safe to > > simply check based on the offset. The ext API will solve this by > > moving the DMA-buf and offset check to the kernel. > > > > > - planes[0].offset == 0 > > > - planes[1].offset == planes[0].length > > > > > > and possibly also check the length. If the buffer isn't suitable, > > > V4L2VideoDevice::queueBuffer() should return an error. > > > > > > > > > + offset += planes[i].length; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > return std::make_unique<FrameBuffer>(std::move(planes)); > > > > > > } > > > > > > > > -- > Regards, > > Laurent Pinchart
On Fri, Aug 27, 2021 at 10:00 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > > > Le jeu. 26 août 2021 10 h 48, Laurent Pinchart <laurent.pinchart@ideasonboard.com> a écrit : >> >> Hi Nicolas, >> >> On Thu, Aug 26, 2021 at 10:31:41AM -0400, Nicolas Dufresne wrote: >> > Le jeudi 26 août 2021 à 14:08 +0300, Laurent Pinchart a écrit : >> > > > We'll probably have to carry a downstream patch to handle them >> > > > initially... Would it be possible to at least make it easy to do that >> > > > until proper V4L2 support gets merged? >> > > > >> > > > (v4l2_ext API is being worked on, but last time I checked it was stuck >> > > > in review, without any comments...) >> > > >> > > If there's significant progress with this development we can carry a >> > > downstream patch to avoid waiting for the API to be merged upstream, but >> > > it indeed seems stuck. What would be your use case for such a downstream >> > > patch though ? We would need drivers using the API to make it useful in >> > > libcamera. >> > >> > I've come across a potential usage of this, but that was too much work to fix >> > back then. The thingy was a very low power camera viewer (in a grid). We wanted >> > to get all the requested frames into the same DMABuf, for that we could have had >> > to be able to import that DMABuf with customized offsets and strides, so that >> > the camera (if capable) would write the frame at the appropriate location in the >> > frame. With 100% of the v4l2 driver not accepting custom strides, and this >> > import limitation, we gave up, and simply reduced the grid size due to memory >> > limitation. >> >> Interesting use case. I was however more wondering if Tomasz had use >> cases in existing drivers, possibly drivers that carry out-of-tree >> patches to support a data offset in V4L2. I fully agree it would be nice >> to have data offsets in V4L2, but what I wonder if whether that would >> only be used in the future once it's available, or if there are devices >> that already use this feature in a downstream kernel. > > > I believe in Xilinx tree, data_offset is used for import too. At least this is how this apparently invalid usage made it into GStreamer. > > If my memory is right, this method was denied by mainline as it would be incompatible with drivers that uses the buffer from the start to data_offset to store some state. > > While unusual, some codec drivers apparently store motion vectors there. Iirc it might be the case for Qualcomm Venus VP8 decoder. > > My memory is a bit rusted, so all this needs to be checked. I never really been completely clear why is this method was invalid, it allow per plane offset, hence allow importing single DMAbuf into drivers to that only support true MPLANE. The reason was that data_offset is currently defined as filled in by userspace for OUTPUT and by drivers for CAPTURE. So there is no way to pass plane offsets for CAPTURE buffers.
Hi Tomasz, On Fri, Aug 27, 2021 at 07:36:34PM +0900, Tomasz Figa wrote: > On Thu, Aug 26, 2021 at 8:09 PM Laurent Pinchart wrote: > > On Thu, Aug 26, 2021 at 02:14:45PM +0900, Tomasz Figa wrote: > > > On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart wrote: > > > > On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote: > > > > > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote: > > > > > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote: > > > > > > > V4L2VideDevice::CreateBuffer() creates the same number of > > > > > > > > > > > > s/CreateBuffer/createBuffer/ > > > > > > > > > > > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > > > > > > format single is single-planar format, the created number of > > > > > > > FrameBuffer::Planes is 1. > > > > > > > It should rather create the same number of FrameBuffer::Planes as the > > > > > > > color format planes. > > > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > --- > > > > > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++- > > > > > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- > > > > > > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > > > > index e767ec84..d0aeb614 100644 > > > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > > > > @@ -136,11 +136,13 @@ private: > > > > > > > private: > > > > > > > struct Plane { > > > > > > > Plane(const FrameBuffer::Plane &plane) > > > > > > > - : fd(plane.fd.fd()), length(plane.length) > > > > > > > + : fd(plane.fd.fd()), offset(plane.offset), > > > > > > > + length(plane.length) > > > > > > > { > > > > > > > } > > > > > > > > > > > > > > int fd; > > > > > > > + unsigned int offset; > > > > > > > unsigned int length; > > > > > > > }; > > > > > > > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > > > index ce60dff6..549418c8 100644 > > > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > > > @@ -25,6 +25,7 @@ > > > > > > > > > > > > > > #include <libcamera/file_descriptor.h> > > > > > > > > > > > > > > +#include "libcamera/internal/formats.h" > > > > > > > #include "libcamera/internal/media_device.h" > > > > > > > #include "libcamera/internal/media_object.h" > > > > > > > > > > > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > > > > > > > > > > for (unsigned int i = 0; i < planes.size(); i++) > > > > > > > if (planes_[i].fd != planes[i].fd.fd() || > > > > > > > + planes_[i].offset != planes_[i].offset || > > > > > > > planes_[i].length != planes[i].length) > > > > > > > return false; > > > > > > > return true; > > > > > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > > > > > > > > > It would be good to capture the fact that the FrameBuffer planes now > > > > > > match the colour planes and not the V4L2 planes. This function is > > > > > > private and isn't documented, so we can't handle it here, and should > > > > > > instead document this in V4L2VideoDevice::allocateBuffers() and > > > > > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that > > > > > > state > > > > > > > > > > > > * The number of planes and the plane sizes for the allocation are determined > > > > > > * by the currently active format on the device as set by setFormat(). > > > > > > > > > > > > It could be extended to > > > > > > > > > > > > * The number of planes and their offsets and sizes are determined by the > > > > > > * currently active format on the device as set by setFormat(). They do not map > > > > > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > > > > > * instance, if the active format is formats::NV12, the allocated FrameBuffer > > > > > > * instances will have two planes, for the luma and chroma components, > > > > > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > > > > > * V4L2_PIX_FMT_NV12M. > > > > > > > > > > > > > > > > > > > > FrameBuffer::Plane plane; > > > > > > > plane.fd = std::move(fd); > > > > > > > - plane.length = multiPlanar ? > > > > > > > - buf.m.planes[nplane].length : buf.length; > > > > > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; > > > > > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > > > > > > > > > The data_offset field defined by V4L2 is, unfortunately, not the offset > > > > > > in the dmabuf, but the offset of the data from the start of the plane. > > > > > > It's used by codec drivers to indicate the size of the header stored at > > > > > > the beginning of the plane. The data_offset is actually contained in the > > > > > > length. > > > > > > > > > > > > I think we can ignore the data offset reported by V4L2, until the V4L2 > > > > > > API is expanded with a "real" data offset. plane.offset can be hardcoded > > > > > > to 0. The change to operator== could also probably be dropped. > > > > > > > > > > Yeah, I have faced the problem in developing chromium video accelerated codecs. > > > > > Although I think setting offset to 0 is not correct either.. but no > > > > > way of avoiding problem here. > > > > > > > > Let's revisit this part of the code once V4L2 gets proper support for > > > > offsets :-) > > > > > > We'll probably have to carry a downstream patch to handle them > > > initially... Would it be possible to at least make it easy to do that > > > until proper V4L2 support gets merged? > > > > > > (v4l2_ext API is being worked on, but last time I checked it was stuck > > > in review, without any comments...) > > > > If there's significant progress with this development we can carry a > > downstream patch to avoid waiting for the API to be merged upstream, but > > it indeed seems stuck. What would be your use case for such a downstream > > patch though ? We would need drivers using the API to make it useful in > > libcamera. > > We carry driver patches to use data_offset for plane offset. > > The background is that for consistency with various graphics APIs, > Chrome OS always passes planes as duped FDs + offset + stride, despite > generally placing all the color planes inside the same DMA-buf. Also, > the offset doesn't necessarily match V4L2 non-M formats to achieve > more efficient line and/or plane alignment and so we often need to use > M formats with a way to pass plane offsets. I suspected so :-) Once the API will be closer to getting merged upstream then I think we can pull it in libcamera. > > > > > > > > > > > > > > planes.push_back(std::move(plane)); > > > > > > > } > > > > > > > > > > > > > > + V4L2DeviceFormat format{}; > > > > > > > + ret = getFormat(&format); > > > > > > > + if (ret < 0) { > > > > > > > + LOG(V4L2, Error) > > > > > > > + << "Failed to get format: " << strerror(-ret); > > > > > > > + return nullptr; > > > > > > > + } > > > > > > > + > > > > > > > > > > > > A comment to explain what's going on would be useful. > > > > > > > > > > > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > > > > > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > > > > > > > > > > > If info isn't valid, should we return nullptr ? > > > > > > > > > > > > > + ASSERT(numPlanes == 1u); > > > > > > > + planes.resize(info.numPlanes()); > > > > > > > + const FileDescriptor &fd = planes[0].fd; > > > > > > > + size_t offset = 0; > > > > > > > + for (size_t i = 0; i < info.numPlanes(); ++i) { > > > > > > > + planes[i].fd = fd; > > > > > > > + planes[i].offset = offset; > > > > > > > + planes[i].length = info.frameSize(format.size); > > > > > > > > > > > > Isn't it the plane size that we need here, not the full frame size ? > > > > > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format, > > > > > > will return the total size of the image, including both colour planes. > > > > > > > > > > > > There's also one change missing. You're addressing the buffer allocation > > > > > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has > > > > > > two planes, we need to translate from two colour planes to one V4L2 > > > > > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used. > > > > > > > > > > I am confused thanks to various "Plane" structures. > > > > > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it? > > > > > FrameMetadata::Plane is used to fill other v4l2_buffer variables. > > > > > What shall I fix in queueBuffer()? > > > > > > > > A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour > > > > planes, with the same dmabuf for both: > > > > > > > > planes[0].fd = dmabuf; > > > > planes[0].offset = 0; > > > > planes[0].length = stride * height; > > > > planes[1].fd = dmabuf; > > > > planes[1].offset = stride * height; > > > > planes[1].length = stride * height / 2; > > > > > > > > This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike > > > > V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice, > > > > V4L2 will expect a single buffer plane, with > > > > > > > > v4l2Planes[0].m.fd = dmabuf; > > > > > > > > Additionally, for output buffers, bytesused and length also need to be > > > > set, but we may not need to handle that right away. > > > > > > > > V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the > > > > two colour planes in the FrameBuffer to the single V4L2 buffer plane. > > > > The translation is fairly easy, as it only requires reducing the number > > > > of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12 > > > > isn't available anywhere, so we will need to add this information > > > > somewhere). We however needs to verify that the FrameBuffer is suitable > > > > for V4L2, and check that > > > > > > > > - planes[0].fd == planes[1].fd > > > > > > planes[0].fd != planes[1].fd doesn't always imply that they refer to a > > > different DMA-buf. I'd skip this check (or just emit a warning) and > > > rely on checking whether the offset matches. > > > > We can check the inode number with fstat() (I've tested that by dupping > > a dmabuf fd, and I get the same inode). It's probably overkill for now, > > but will likely be done at some point. The reason why I'd like this > > check to stay is to catch userspace that would give use different fds > > (whether or not they reference the same dmabuf), and then decide how to > > handle them. > > > > > Generally the two common patterns in V4L2 are: > > > 1) single plane with 1 DMA-buf for all color planes > > > 2) multiplane with 1 DMA-buf for _each_ color plane, with zero offset > > > within each > > > > I'd say that more than this being a common pattern, it's the only thing > > that V4L2 supports upstream :-) > > > > > I haven't seen any other case in practice, so it should be safe to > > > simply check based on the offset. The ext API will solve this by > > > moving the DMA-buf and offset check to the kernel. > > > > > > > - planes[0].offset == 0 > > > > - planes[1].offset == planes[0].length > > > > > > > > and possibly also check the length. If the buffer isn't suitable, > > > > V4L2VideoDevice::queueBuffer() should return an error. > > > > > > > > > > > + offset += planes[i].length; > > > > > > > + } > > > > > > > + } > > > > > > > + > > > > > > > return std::make_unique<FrameBuffer>(std::move(planes)); > > > > > > > } > > > > > > >
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index e767ec84..d0aeb614 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -136,11 +136,13 @@ private: private: struct Plane { Plane(const FrameBuffer::Plane &plane) - : fd(plane.fd.fd()), length(plane.length) + : fd(plane.fd.fd()), offset(plane.offset), + length(plane.length) { } int fd; + unsigned int offset; unsigned int length; }; diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index ce60dff6..549418c8 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -25,6 +25,7 @@ #include <libcamera/file_descriptor.h> +#include "libcamera/internal/formats.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/media_object.h" @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const for (unsigned int i = 0; i < planes.size(); i++) if (planes_[i].fd != planes[i].fd.fd() || + planes_[i].offset != planes_[i].offset || planes_[i].length != planes[i].length) return false; return true; @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) FrameBuffer::Plane plane; plane.fd = std::move(fd); - plane.length = multiPlanar ? - buf.m.planes[nplane].length : buf.length; + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0; + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; planes.push_back(std::move(plane)); } + V4L2DeviceFormat format{}; + ret = getFormat(&format); + if (ret < 0) { + LOG(V4L2, Error) + << "Failed to get format: " << strerror(-ret); + return nullptr; + } + + const auto &info = PixelFormatInfo::info(format.fourcc); + if (info.isValid() && info.numPlanes() != numPlanes) { + ASSERT(numPlanes == 1u); + planes.resize(info.numPlanes()); + const FileDescriptor &fd = planes[0].fd; + size_t offset = 0; + for (size_t i = 0; i < info.numPlanes(); ++i) { + planes[i].fd = fd; + planes[i].offset = offset; + planes[i].length = info.frameSize(format.size); + offset += planes[i].length; + } + } + return std::make_unique<FrameBuffer>(std::move(planes)); }
V4L2VideDevice::CreateBuffer() creates the same number of FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 format single is single-planar format, the created number of FrameBuffer::Planes is 1. It should rather create the same number of FrameBuffer::Planes as the color format planes. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/v4l2_videodevice.h | 4 ++- src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-)