| Message ID | 20210816043138.957984-10-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:37PM +0900, Hirokazu Honda wrote: > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > each FrameBuffer::Plane::length. It should rather be the length of > plane. This also changes CreateFrameBuffer() to fill offset of > FrameBuffer::Plane. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a69b687a..1ded5cb1 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -12,6 +12,7 @@ > > #include <algorithm> > #include <fstream> > +#include <sys/mman.h> > #include <unistd.h> > #include <vector> > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > { > - std::vector<FrameBuffer::Plane> planes; > + FileDescriptor fd; > + /* This assumes all the planes are in the same buffer. */ Should we ASSERT() if that's not the case ? Also, does this assumption always hold ? > for (int i = 0; i < camera3buffer->numFds; i++) { > - /* Skip unused planes. */ > - if (camera3buffer->data[i] == -1) > + if (camera3buffer->data[i] != -1) { > + fd = FileDescriptor(camera3buffer->data[i]); > break; > - > - FrameBuffer::Plane plane; > - plane.fd = FileDescriptor(camera3buffer->data[i]); > - if (!plane.fd.isValid()) { > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > - << camera3buffer->data[i] << ") " > - << " on plane " << i; > - return nullptr; > } > + } Blank line. > + if (!fd.isValid()) { > + LOG(HAL, Fatal) << "No valid fd"; > + return nullptr; > + } > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > - if (length == -1) { > - LOG(HAL, Error) << "Failed to query plane length"; > - return nullptr; > - } > + CameraBuffer buf(camera3buffer, PROT_READ); I suppose there's no other way than using the cros::CameraBufferManager to get the necessary information. I'm annoyed that this involves mapping the buffer to the CPU just to retrieve sizes, as that's an expensive operation :-( It's of course not a prerequisite for this series, but I'm wondering if the cros::CameraBufferManager API could be improved to separate retrieving plane information from mapping the buffer. What do you think ? Also, should generic_camera_buffer.cpp be updated to provide the correct information ? > + if (!buf.isValid()) { > + LOG(HAL, Fatal) << "Failed mapping buffer"; > + return nullptr; > + } > > - plane.length = length; > - planes.push_back(std::move(plane)); > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > + planes[i].fd = fd; > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > + planes[i].length = buf.plane(i).size(); > } > > return new FrameBuffer(std::move(planes));
On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > each FrameBuffer::Plane::length. It should rather be the length of > > plane. This also changes CreateFrameBuffer() to fill offset of > > FrameBuffer::Plane. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index a69b687a..1ded5cb1 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -12,6 +12,7 @@ > > > > #include <algorithm> > > #include <fstream> > > +#include <sys/mman.h> > > #include <unistd.h> > > #include <vector> > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > { > > - std::vector<FrameBuffer::Plane> planes; > > + FileDescriptor fd; > > + /* This assumes all the planes are in the same buffer. */ > > Should we ASSERT() if that's not the case ? Also, does this assumption > always hold ? > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > - /* Skip unused planes. */ > > - if (camera3buffer->data[i] == -1) > > + if (camera3buffer->data[i] != -1) { > > + fd = FileDescriptor(camera3buffer->data[i]); > > break; > > - > > - FrameBuffer::Plane plane; > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > - if (!plane.fd.isValid()) { > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > - << camera3buffer->data[i] << ") " > > - << " on plane " << i; > > - return nullptr; > > } > > + } > > Blank line. > > > + if (!fd.isValid()) { > > + LOG(HAL, Fatal) << "No valid fd"; > > + return nullptr; > > + } > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > - if (length == -1) { > > - LOG(HAL, Error) << "Failed to query plane length"; > > - return nullptr; > > - } > > + CameraBuffer buf(camera3buffer, PROT_READ); > > I suppose there's no other way than using the cros::CameraBufferManager > to get the necessary information. I'm annoyed that this involves mapping > the buffer to the CPU just to retrieve sizes, as that's an expensive > operation :-( It's of course not a prerequisite for this series, but I'm > wondering if the cros::CameraBufferManager API could be improved to > separate retrieving plane information from mapping the buffer. What do > you think ? Which mapping are you referring to? Also, there are already methods to retrieve this information, see GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > Also, should generic_camera_buffer.cpp be updated to provide the correct > information ? > > > + if (!buf.isValid()) { > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > + return nullptr; > > + } > > > > - plane.length = length; > > - planes.push_back(std::move(plane)); > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > + planes[i].fd = fd; > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > + planes[i].length = buf.plane(i).size(); > > } > > > > return new FrameBuffer(std::move(planes)); > > -- > Regards, > > Laurent Pinchart
Hi Tomasz, On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote: > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote: > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > > each FrameBuffer::Plane::length. It should rather be the length of > > > plane. This also changes CreateFrameBuffer() to fill offset of > > > FrameBuffer::Plane. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index a69b687a..1ded5cb1 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -12,6 +12,7 @@ > > > > > > #include <algorithm> > > > #include <fstream> > > > +#include <sys/mman.h> > > > #include <unistd.h> > > > #include <vector> > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > > { > > > - std::vector<FrameBuffer::Plane> planes; > > > + FileDescriptor fd; > > > + /* This assumes all the planes are in the same buffer. */ > > > > Should we ASSERT() if that's not the case ? Also, does this assumption > > always hold ? > > > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > > - /* Skip unused planes. */ > > > - if (camera3buffer->data[i] == -1) > > > + if (camera3buffer->data[i] != -1) { > > > + fd = FileDescriptor(camera3buffer->data[i]); > > > break; > > > - > > > - FrameBuffer::Plane plane; > > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > > - if (!plane.fd.isValid()) { > > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > > - << camera3buffer->data[i] << ") " > > > - << " on plane " << i; > > > - return nullptr; > > > } > > > + } > > > > Blank line. > > > > > + if (!fd.isValid()) { > > > + LOG(HAL, Fatal) << "No valid fd"; > > > + return nullptr; > > > + } > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > > - if (length == -1) { > > > - LOG(HAL, Error) << "Failed to query plane length"; > > > - return nullptr; > > > - } > > > + CameraBuffer buf(camera3buffer, PROT_READ); > > > > I suppose there's no other way than using the cros::CameraBufferManager > > to get the necessary information. I'm annoyed that this involves mapping > > the buffer to the CPU just to retrieve sizes, as that's an expensive > > operation :-( It's of course not a prerequisite for this series, but I'm > > wondering if the cros::CameraBufferManager API could be improved to > > separate retrieving plane information from mapping the buffer. What do > > you think ? > > Which mapping are you referring to? > > Also, there are already methods to retrieve this information, see > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. You're right. We currently call Lock or LockYCbCr in the CameraBuffer constructor, but that's not required to get the plane size or offset. We should thus be able to improve the implementation here, without any change on the CrOS side. Thanks for the feedback. > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > > Also, should generic_camera_buffer.cpp be updated to provide the correct > > information ? > > > > > + if (!buf.isValid()) { > > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > > + return nullptr; > > > + } > > > > > > - plane.length = length; > > > - planes.push_back(std::move(plane)); > > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > > + planes[i].fd = fd; > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > > + planes[i].length = buf.plane(i).size(); > > > } > > > > > > return new FrameBuffer(std::move(planes));
Hi Laurent, thank you for reviewing. On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Tomasz, > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote: > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote: > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > > > each FrameBuffer::Plane::length. It should rather be the length of > > > > plane. This also changes CreateFrameBuffer() to fill offset of > > > > FrameBuffer::Plane. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index a69b687a..1ded5cb1 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -12,6 +12,7 @@ > > > > > > > > #include <algorithm> > > > > #include <fstream> > > > > +#include <sys/mman.h> > > > > #include <unistd.h> > > > > #include <vector> > > > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > > > { > > > > - std::vector<FrameBuffer::Plane> planes; > > > > + FileDescriptor fd; > > > > + /* This assumes all the planes are in the same buffer. */ > > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption > > > always hold ? > > > I think this assumption holds at least on ChromeOS. Hmm, ASSERT() is difficult because fds are duplicated and thus different from each other in terms of integer. > > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > > > - /* Skip unused planes. */ > > > > - if (camera3buffer->data[i] == -1) > > > > + if (camera3buffer->data[i] != -1) { > > > > + fd = FileDescriptor(camera3buffer->data[i]); > > > > break; > > > > - > > > > - FrameBuffer::Plane plane; > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > > > - if (!plane.fd.isValid()) { > > > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > > > - << camera3buffer->data[i] << ") " > > > > - << " on plane " << i; > > > > - return nullptr; > > > > } > > > > + } > > > > > > Blank line. > > > > > > > + if (!fd.isValid()) { > > > > + LOG(HAL, Fatal) << "No valid fd"; > > > > + return nullptr; > > > > + } > > > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > > > - if (length == -1) { > > > > - LOG(HAL, Error) << "Failed to query plane length"; > > > > - return nullptr; > > > > - } > > > > + CameraBuffer buf(camera3buffer, PROT_READ); > > > > > > I suppose there's no other way than using the cros::CameraBufferManager > > > to get the necessary information. I'm annoyed that this involves mapping > > > the buffer to the CPU just to retrieve sizes, as that's an expensive > > > operation :-( It's of course not a prerequisite for this series, but I'm > > > wondering if the cros::CameraBufferManager API could be improved to > > > separate retrieving plane information from mapping the buffer. What do > > > you think ? > > > > Which mapping are you referring to? > > > > Also, there are already methods to retrieve this information, see > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer > constructor, but that's not required to get the plane size or offset. We > should thus be able to improve the implementation here, without any > change on the CrOS side. Thanks for the feedback. > I actually tried to add the option to not map in the constructor https://patchwork.libcamera.org/patch/11837/. Would you tell me your preferred way? Best Regards, -Hiro > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct > > > information ? > > > > > > > + if (!buf.isValid()) { > > > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > > > + return nullptr; > > > > + } > > > > > > > > - plane.length = length; > > > > - planes.push_back(std::move(plane)); > > > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > > > + planes[i].fd = fd; > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > > > + planes[i].length = buf.plane(i).size(); > > > > } > > > > > > > > return new FrameBuffer(std::move(planes)); > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote: > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote: > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote: > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote: > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > > > > each FrameBuffer::Plane::length. It should rather be the length of > > > > > plane. This also changes CreateFrameBuffer() to fill offset of > > > > > FrameBuffer::Plane. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > index a69b687a..1ded5cb1 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -12,6 +12,7 @@ > > > > > > > > > > #include <algorithm> > > > > > #include <fstream> > > > > > +#include <sys/mman.h> > > > > > #include <unistd.h> > > > > > #include <vector> > > > > > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > > > > { > > > > > - std::vector<FrameBuffer::Plane> planes; > > > > > + FileDescriptor fd; > > > > > + /* This assumes all the planes are in the same buffer. */ > > > > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption > > > > always hold ? > > > > > > I think this assumption holds at least on ChromeOS. > Hmm, ASSERT() is difficult because fds are duplicated and thus > different from each other in terms of integer. So Chrome OS will give different fds, all referring to the same dmabuf ? > > > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > > > > - /* Skip unused planes. */ > > > > > - if (camera3buffer->data[i] == -1) > > > > > + if (camera3buffer->data[i] != -1) { > > > > > + fd = FileDescriptor(camera3buffer->data[i]); > > > > > break; > > > > > - > > > > > - FrameBuffer::Plane plane; > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > > > > - if (!plane.fd.isValid()) { > > > > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > > > > - << camera3buffer->data[i] << ") " > > > > > - << " on plane " << i; > > > > > - return nullptr; > > > > > } > > > > > + } > > > > > > > > Blank line. > > > > > > > > > + if (!fd.isValid()) { > > > > > + LOG(HAL, Fatal) << "No valid fd"; > > > > > + return nullptr; > > > > > + } > > > > > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > > > > - if (length == -1) { > > > > > - LOG(HAL, Error) << "Failed to query plane length"; > > > > > - return nullptr; > > > > > - } > > > > > + CameraBuffer buf(camera3buffer, PROT_READ); > > > > > > > > I suppose there's no other way than using the cros::CameraBufferManager > > > > to get the necessary information. I'm annoyed that this involves mapping > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive > > > > operation :-( It's of course not a prerequisite for this series, but I'm > > > > wondering if the cros::CameraBufferManager API could be improved to > > > > separate retrieving plane information from mapping the buffer. What do > > > > you think ? > > > > > > Which mapping are you referring to? > > > > > > Also, there are already methods to retrieve this information, see > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. > > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer > > constructor, but that's not required to get the plane size or offset. We > > should thus be able to improve the implementation here, without any > > change on the CrOS side. Thanks for the feedback. > > I actually tried to add the option to not map in the constructor > https://patchwork.libcamera.org/patch/11837/. > Would you tell me your preferred way? I had missed that, sorry. I've now replied to that patch. > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct > > > > information ? > > > > > > > > > + if (!buf.isValid()) { > > > > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > > > > + return nullptr; > > > > > + } > > > > > > > > > > - plane.length = length; > > > > > - planes.push_back(std::move(plane)); > > > > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > > > > + planes[i].fd = fd; > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > > > > + planes[i].length = buf.plane(i).size(); > > > > > } > > > > > > > > > > return new FrameBuffer(std::move(planes));
Hi Laurent, On Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote: > > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote: > > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote: > > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote: > > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > > > > > each FrameBuffer::Plane::length. It should rather be the length of > > > > > > plane. This also changes CreateFrameBuffer() to fill offset of > > > > > > FrameBuffer::Plane. > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > --- > > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > > > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > > index a69b687a..1ded5cb1 100644 > > > > > > --- a/src/android/camera_device.cpp > > > > > > +++ b/src/android/camera_device.cpp > > > > > > @@ -12,6 +12,7 @@ > > > > > > > > > > > > #include <algorithm> > > > > > > #include <fstream> > > > > > > +#include <sys/mman.h> > > > > > > #include <unistd.h> > > > > > > #include <vector> > > > > > > > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > > > > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > > > > > { > > > > > > - std::vector<FrameBuffer::Plane> planes; > > > > > > + FileDescriptor fd; > > > > > > + /* This assumes all the planes are in the same buffer. */ > > > > > > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption > > > > > always hold ? > > > > > > > > > I think this assumption holds at least on ChromeOS. > > Hmm, ASSERT() is difficult because fds are duplicated and thus > > different from each other in terms of integer. > > So Chrome OS will give different fds, all referring to the same dmabuf ? > I didn't know they are always dup()ed. It depends on HAL client implementation. +Ricky, do you know about it? > > > > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > > > > > - /* Skip unused planes. */ > > > > > > - if (camera3buffer->data[i] == -1) > > > > > > + if (camera3buffer->data[i] != -1) { > > > > > > + fd = FileDescriptor(camera3buffer->data[i]); > > > > > > break; > > > > > > - > > > > > > - FrameBuffer::Plane plane; > > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > > > > > - if (!plane.fd.isValid()) { > > > > > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > > > > > - << camera3buffer->data[i] << ") " > > > > > > - << " on plane " << i; > > > > > > - return nullptr; > > > > > > } > > > > > > + } > > > > > > > > > > Blank line. > > > > > > > > > > > + if (!fd.isValid()) { > > > > > > + LOG(HAL, Fatal) << "No valid fd"; > > > > > > + return nullptr; > > > > > > + } > > > > > > > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > > > > > - if (length == -1) { > > > > > > - LOG(HAL, Error) << "Failed to query plane length"; > > > > > > - return nullptr; > > > > > > - } > > > > > > + CameraBuffer buf(camera3buffer, PROT_READ); > > > > > > > > > > I suppose there's no other way than using the cros::CameraBufferManager > > > > > to get the necessary information. I'm annoyed that this involves mapping > > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive > > > > > operation :-( It's of course not a prerequisite for this series, but I'm > > > > > wondering if the cros::CameraBufferManager API could be improved to > > > > > separate retrieving plane information from mapping the buffer. What do > > > > > you think ? > > > > > > > > Which mapping are you referring to? > > > > > > > > Also, there are already methods to retrieve this information, see > > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. > > > > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer > > > constructor, but that's not required to get the plane size or offset. We > > > should thus be able to improve the implementation here, without any > > > change on the CrOS side. Thanks for the feedback. > > > > I actually tried to add the option to not map in the constructor > > https://patchwork.libcamera.org/patch/11837/. > > Would you tell me your preferred way? > > I had missed that, sorry. I've now replied to that patch. > > > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > > > > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct > > > > > information ? > > > > > > > > > > > + if (!buf.isValid()) { > > > > > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > > > > > + return nullptr; > > > > > > + } > > > > > > > > > > > > - plane.length = length; > > > > > > - planes.push_back(std::move(plane)); > > > > > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > > > > > + planes[i].fd = fd; > > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > > > > > + planes[i].length = buf.plane(i).size(); > > > > > > } > > > > > > > > > > > > return new FrameBuffer(std::move(planes)); > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Fri, Aug 20, 2021 at 08:31:59PM +0900, Hirokazu Honda wrote: > On Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart wrote: > > On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote: > > > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote: > > > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote: > > > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote: > > > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > > > > > > each FrameBuffer::Plane::length. It should rather be the length of > > > > > > > plane. This also changes CreateFrameBuffer() to fill offset of > > > > > > > FrameBuffer::Plane. > > > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > --- > > > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > > > > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > > > index a69b687a..1ded5cb1 100644 > > > > > > > --- a/src/android/camera_device.cpp > > > > > > > +++ b/src/android/camera_device.cpp > > > > > > > @@ -12,6 +12,7 @@ > > > > > > > > > > > > > > #include <algorithm> > > > > > > > #include <fstream> > > > > > > > +#include <sys/mman.h> > > > > > > > #include <unistd.h> > > > > > > > #include <vector> > > > > > > > > > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > > > > > > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > > > > > > { > > > > > > > - std::vector<FrameBuffer::Plane> planes; > > > > > > > + FileDescriptor fd; > > > > > > > + /* This assumes all the planes are in the same buffer. */ > > > > > > > > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption > > > > > > always hold ? > > > > > > > > > > > > I think this assumption holds at least on ChromeOS. > > > Hmm, ASSERT() is difficult because fds are duplicated and thus > > > different from each other in terms of integer. > > > > So Chrome OS will give different fds, all referring to the same dmabuf ? > > I didn't know they are always dup()ed. It depends on HAL client implementation. > +Ricky, do you know about it? For what it's worth, I believe you can check if two different fds refer to the same dmabuf by comparing their inode number, as returned by fstat(). This is something that should likely be done in V4L2VideoDevice::queueBuffer(), as from the point of view of the libcamera public API, we want to support different buffers and offsets. > > > > > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > > > > > > - /* Skip unused planes. */ > > > > > > > - if (camera3buffer->data[i] == -1) > > > > > > > + if (camera3buffer->data[i] != -1) { > > > > > > > + fd = FileDescriptor(camera3buffer->data[i]); > > > > > > > break; > > > > > > > - > > > > > > > - FrameBuffer::Plane plane; > > > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > > > > > > - if (!plane.fd.isValid()) { > > > > > > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > > > > > > - << camera3buffer->data[i] << ") " > > > > > > > - << " on plane " << i; > > > > > > > - return nullptr; > > > > > > > } > > > > > > > + } > > > > > > > > > > > > Blank line. > > > > > > > > > > > > > + if (!fd.isValid()) { > > > > > > > + LOG(HAL, Fatal) << "No valid fd"; > > > > > > > + return nullptr; > > > > > > > + } > > > > > > > > > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > > > > > > - if (length == -1) { > > > > > > > - LOG(HAL, Error) << "Failed to query plane length"; > > > > > > > - return nullptr; > > > > > > > - } > > > > > > > + CameraBuffer buf(camera3buffer, PROT_READ); > > > > > > > > > > > > I suppose there's no other way than using the cros::CameraBufferManager > > > > > > to get the necessary information. I'm annoyed that this involves mapping > > > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive > > > > > > operation :-( It's of course not a prerequisite for this series, but I'm > > > > > > wondering if the cros::CameraBufferManager API could be improved to > > > > > > separate retrieving plane information from mapping the buffer. What do > > > > > > you think ? > > > > > > > > > > Which mapping are you referring to? > > > > > > > > > > Also, there are already methods to retrieve this information, see > > > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. > > > > > > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer > > > > constructor, but that's not required to get the plane size or offset. We > > > > should thus be able to improve the implementation here, without any > > > > change on the CrOS side. Thanks for the feedback. > > > > > > I actually tried to add the option to not map in the constructor > > > https://patchwork.libcamera.org/patch/11837/. > > > Would you tell me your preferred way? > > > > I had missed that, sorry. I've now replied to that patch. > > > > > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > > > > > > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct > > > > > > information ? > > > > > > > > > > > > > + if (!buf.isValid()) { > > > > > > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > > > > > > + return nullptr; > > > > > > > + } > > > > > > > > > > > > > > - plane.length = length; > > > > > > > - planes.push_back(std::move(plane)); > > > > > > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > > > > > > + planes[i].fd = fd; > > > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > > > > > > + planes[i].length = buf.plane(i).size(); > > > > > > > } > > > > > > > > > > > > > > return new FrameBuffer(std::move(planes));
On Sat, Aug 21, 2021 at 1:25 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Fri, Aug 20, 2021 at 08:31:59PM +0900, Hirokazu Honda wrote: > > On Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart wrote: > > > On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote: > > > > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote: > > > > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote: > > > > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote: > > > > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote: > > > > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to > > > > > > > > each FrameBuffer::Plane::length. It should rather be the length of > > > > > > > > plane. This also changes CreateFrameBuffer() to fill offset of > > > > > > > > FrameBuffer::Plane. > > > > > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > --- > > > > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- > > > > > > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > > > > index a69b687a..1ded5cb1 100644 > > > > > > > > --- a/src/android/camera_device.cpp > > > > > > > > +++ b/src/android/camera_device.cpp > > > > > > > > @@ -12,6 +12,7 @@ > > > > > > > > > > > > > > > > #include <algorithm> > > > > > > > > #include <fstream> > > > > > > > > +#include <sys/mman.h> > > > > > > > > #include <unistd.h> > > > > > > > > #include <vector> > > > > > > > > > > > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > > > > > > > > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) > > > > > > > > { > > > > > > > > - std::vector<FrameBuffer::Plane> planes; > > > > > > > > + FileDescriptor fd; > > > > > > > > + /* This assumes all the planes are in the same buffer. */ > > > > > > > > > > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption > > > > > > > always hold ? > > > > > > > > > > > > > > > I think this assumption holds at least on ChromeOS. > > > > Hmm, ASSERT() is difficult because fds are duplicated and thus > > > > different from each other in terms of integer. > > > > > > So Chrome OS will give different fds, all referring to the same dmabuf ? > > > > I didn't know they are always dup()ed. It depends on HAL client implementation. > > +Ricky, do you know about it? > > For what it's worth, I believe you can check if two different fds refer > to the same dmabuf by comparing their inode number, as returned by > fstat(). This is something that should likely be done in > V4L2VideoDevice::queueBuffer(), as from the point of view of the > libcamera public API, we want to support different buffers and offsets. > As I mentioned in another reply, there are only 2 patterns that occur in practice - 1 DMA-buf with offsets for color planes OR multiple DMA-bufs with 0 offsets. Chrome OS actually ends up using only the former. Best regards, Tomasz > > > > > > > > for (int i = 0; i < camera3buffer->numFds; i++) { > > > > > > > > - /* Skip unused planes. */ > > > > > > > > - if (camera3buffer->data[i] == -1) > > > > > > > > + if (camera3buffer->data[i] != -1) { > > > > > > > > + fd = FileDescriptor(camera3buffer->data[i]); > > > > > > > > break; > > > > > > > > - > > > > > > > > - FrameBuffer::Plane plane; > > > > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]); > > > > > > > > - if (!plane.fd.isValid()) { > > > > > > > > - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" > > > > > > > > - << camera3buffer->data[i] << ") " > > > > > > > > - << " on plane " << i; > > > > > > > > - return nullptr; > > > > > > > > } > > > > > > > > + } > > > > > > > > > > > > > > Blank line. > > > > > > > > > > > > > > > + if (!fd.isValid()) { > > > > > > > > + LOG(HAL, Fatal) << "No valid fd"; > > > > > > > > + return nullptr; > > > > > > > > + } > > > > > > > > > > > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > > > > > > > - if (length == -1) { > > > > > > > > - LOG(HAL, Error) << "Failed to query plane length"; > > > > > > > > - return nullptr; > > > > > > > > - } > > > > > > > > + CameraBuffer buf(camera3buffer, PROT_READ); > > > > > > > > > > > > > > I suppose there's no other way than using the cros::CameraBufferManager > > > > > > > to get the necessary information. I'm annoyed that this involves mapping > > > > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive > > > > > > > operation :-( It's of course not a prerequisite for this series, but I'm > > > > > > > wondering if the cros::CameraBufferManager API could be improved to > > > > > > > separate retrieving plane information from mapping the buffer. What do > > > > > > > you think ? > > > > > > > > > > > > Which mapping are you referring to? > > > > > > > > > > > > Also, there are already methods to retrieve this information, see > > > > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1]. > > > > > > > > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer > > > > > constructor, but that's not required to get the plane size or offset. We > > > > > should thus be able to improve the implementation here, without any > > > > > change on the CrOS side. Thanks for the feedback. > > > > > > > > I actually tried to add the option to not map in the constructor > > > > https://patchwork.libcamera.org/patch/11837/. > > > > Would you tell me your preferred way? > > > > > > I had missed that, sorry. I've now replied to that patch. > > > > > > > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305 > > > > > > > > > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct > > > > > > > information ? > > > > > > > > > > > > > > > + if (!buf.isValid()) { > > > > > > > > + LOG(HAL, Fatal) << "Failed mapping buffer"; > > > > > > > > + return nullptr; > > > > > > > > + } > > > > > > > > > > > > > > > > - plane.length = length; > > > > > > > > - planes.push_back(std::move(plane)); > > > > > > > > + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > > > > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) { > > > > > > > > + planes[i].fd = fd; > > > > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); > > > > > > > > + planes[i].length = buf.plane(i).size(); > > > > > > > > } > > > > > > > > > > > > > > > > return new FrameBuffer(std::move(planes)); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a69b687a..1ded5cb1 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -12,6 +12,7 @@ #include <algorithm> #include <fstream> +#include <sys/mman.h> #include <unistd.h> #include <vector> @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer) { - std::vector<FrameBuffer::Plane> planes; + FileDescriptor fd; + /* This assumes all the planes are in the same buffer. */ for (int i = 0; i < camera3buffer->numFds; i++) { - /* Skip unused planes. */ - if (camera3buffer->data[i] == -1) + if (camera3buffer->data[i] != -1) { + fd = FileDescriptor(camera3buffer->data[i]); break; - - FrameBuffer::Plane plane; - plane.fd = FileDescriptor(camera3buffer->data[i]); - if (!plane.fd.isValid()) { - LOG(HAL, Error) << "Failed to obtain FileDescriptor (" - << camera3buffer->data[i] << ") " - << " on plane " << i; - return nullptr; } + } + if (!fd.isValid()) { + LOG(HAL, Fatal) << "No valid fd"; + return nullptr; + } - off_t length = lseek(plane.fd.fd(), 0, SEEK_END); - if (length == -1) { - LOG(HAL, Error) << "Failed to query plane length"; - return nullptr; - } + CameraBuffer buf(camera3buffer, PROT_READ); + if (!buf.isValid()) { + LOG(HAL, Fatal) << "Failed mapping buffer"; + return nullptr; + } - plane.length = length; - planes.push_back(std::move(plane)); + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); + for (size_t i = 0; i < buf.numPlanes(); ++i) { + planes[i].fd = fd; + planes[i].offset = buf.plane(i).data() - buf.plane(0).data(); + planes[i].length = buf.plane(i).size(); } return new FrameBuffer(std::move(planes));
CameraDevice::CreateFrameBuffer() fills the length of the buffer to each FrameBuffer::Plane::length. It should rather be the length of plane. This also changes CreateFrameBuffer() to fill offset of FrameBuffer::Plane. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 38 ++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 18 deletions(-)