| Message ID | 20221120050629.144188-1-nicholas@rothemail.net |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicholas, Thank you for the patch. On Sat, Nov 19, 2022 at 11:06:29PM -0600, Nicholas Roth via libcamera-devel wrote: > Currently, they way camera_device.cpp handles file descriptors in a s/they/the/ > buffer_handle_t is incorrect. This can result in the HAL attempting to > treat uninitialized memory like file descriptors, usually resulting in > a crash. > > This patch brings the behavior of camera_device.cpp in line with how > CameraBuffer handles file descriptors and planes. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=172 I've replied to the bug report in https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to investigate this further, as numFds != buf.numPlanes() doesn't sound right. > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > --- > src/android/camera_device.cpp | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b20e389b..1459d582 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -781,14 +781,18 @@ CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer, > return nullptr; > } > > + if (camera3buffer->numFds > 1) { > + LOG(HAL, Fatal) << "Discontiguous planes are not supported"; > + } This will break operation on Chrome OS. > + > + SharedFD fd{ camera3buffer->data[0] }; > + if (!fd.isValid()) { > + LOG(HAL, Fatal) << "No valid fd"; > + return nullptr; > + } > + > std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); > for (size_t i = 0; i < buf.numPlanes(); ++i) { > - SharedFD fd{ camera3buffer->data[i] }; > - if (!fd.isValid()) { > - LOG(HAL, Fatal) << "No valid fd"; > - return nullptr; > - } > - > planes[i].fd = fd; > planes[i].offset = buf.offset(i); > planes[i].length = buf.size(i);
> I've replied to the bug report in > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to > investigate this further, as numFds != buf.numPlanes() doesn't sound > right. My understanding from the differences between android/mm/generic_camera_buffer.cpp (Android) and android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on Android each handle buffers differently. See also the comment in generic_camera_buffer.cpp: ``` /* * As Android doesn't offer an API to query buffer layouts, assume for * now that the buffer is backed by a single dmabuf, with planes being * stored contiguously. */ ``` I can't easily find "cros-camera/camera_buffer_manager.h" on the public internet so I can't comment about that here. > This will break operation on Chrome OS. In that case, would you be open to the following? pseudocode: ``` if (camera3buffer->numFds == buf.numPlanes()) { // Act as if there's one buffer per plane (old behavior / ChromeOS) } else if (camera3buffer->numFds == 1) { // Act as if planes are stored contiguously in a single dmabuf (Android) } else { // Log Fatal: Unsupported state: number of FDs {camera3buffer->numFds} is not 1 (contiguous planes) and does not match number of planes {buf.numPlanes()}. Failed to infer buffer layout. } ``` Let me know! Thanks, -Nicholas P.S. I'm moving, and the initial plan is short-term rentals. I may not have access to my PC for awhile. I've tried to put the steps required to replicate my development environment in https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit# and will continue to be active here. I'm also trying to use an IP-KVM to maintain access to my desktop but can't guarantee that will work.
Hi Nicholas, (CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo) On Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote: > > I've replied to the bug report in > > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to > > investigate this further, as numFds != buf.numPlanes() doesn't sound > > right. > > My understanding from the differences between > android/mm/generic_camera_buffer.cpp (Android) and > android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on > Android each handle buffers differently. See also the comment in > generic_camera_buffer.cpp: > ``` > /* > * As Android doesn't offer an API to query buffer layouts, assume for > * now that the buffer is backed by a single dmabuf, with planes being > * stored contiguously. > */ > ``` I didn't interpret this to mean that android would give us a buffer with numFds == 1 even for multiplanar formats, but that all the file descriptors would point to the same dmabuf instance. However, looking at the gralloc implementations for Qualcomm platforms available at https://android.googlesource.com/platform/hardware/qcom/display/, for example the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247), the situation is more complicated. numFds is set to 2, with the first fd storing the image buffer file descriptor (allocated from ION), and the second fd storing a metadata buffer file descriptor (not entirely sure what it's used for). The only reasonable conclusion is that Android is a mess. > I can't easily find "cros-camera/camera_buffer_manager.h" on the > public internet so I can't comment about that here. It's available at https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h. > > This will break operation on Chrome OS. > > In that case, would you be open to the following? > pseudocode: > ``` > if (camera3buffer->numFds == buf.numPlanes()) { > // Act as if there's one buffer per plane (old behavior / ChromeOS) > } else if (camera3buffer->numFds == 1) { > // Act as if planes are stored contiguously in a single dmabuf (Android) > } else { > // Log Fatal: Unsupported state: number of FDs > {camera3buffer->numFds} is not 1 (contiguous planes) and does not > match number of planes {buf.numPlanes()}. Failed to infer buffer > layout. > } > ``` This won't work on Qualcomm platforms, but that's likely something we can ignore for now. I'm also worried about Android platforms that would provide one dmabuf instance (and thus one fd) per plane, which is also something that we could ignore for now I suppose. Looking at CameraDevice::createFrameBuffer(), we create a CameraBuffer instance, which abstracts platform-specific buffer management. I would like to use that to provides the fds we need to create the FrameBuffer, instead of implementing platform-specific logic in createFrameBuffer() directly. We could either extend the class to provides the fds, or possibly better, to create a FrameBuffer instance. Any opinion ? > Let me know! > > Thanks, > -Nicholas > > P.S. I'm moving, and the initial plan is short-term rentals. I may not > have access to my PC for awhile. I've tried to put the steps required > to replicate my development environment in > https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit# > and will continue to be active here. I'm also trying to use an IP-KVM > to maintain access to my desktop but can't guarantee that will work. Thank you for putting those notes together and making them public. It's useful.
Hi Laurent, Nicholas On Mon, Dec 19, 2022 at 03:38:05AM +0200, Laurent Pinchart via libcamera-devel wrote: > Hi Nicholas, > > (CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo) > > On Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote: > > > I've replied to the bug report in > > > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to > > > investigate this further, as numFds != buf.numPlanes() doesn't sound > > > right. > > > > My understanding from the differences between > > android/mm/generic_camera_buffer.cpp (Android) and > > android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on > > Android each handle buffers differently. See also the comment in > > generic_camera_buffer.cpp: > > ``` > > /* > > * As Android doesn't offer an API to query buffer layouts, assume for > > * now that the buffer is backed by a single dmabuf, with planes being > > * stored contiguously. > > */ > > ``` > > I didn't interpret this to mean that android would give us a buffer with > numFds == 1 even for multiplanar formats, but that all the file > descriptors would point to the same dmabuf instance. However, looking at > the gralloc implementations for Qualcomm platforms available at > https://android.googlesource.com/platform/hardware/qcom/display/, for > example the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247), > the situation is more complicated. numFds is set to 2, with the first fd > storing the image buffer file descriptor (allocated from ION), and the > second fd storing a metadata buffer file descriptor (not entirely sure > what it's used for). > > The only reasonable conclusion is that Android is a mess. > > > I can't easily find "cros-camera/camera_buffer_manager.h" on the > > public internet so I can't comment about that here. > > It's available at > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h. > > > > This will break operation on Chrome OS. > > > > In that case, would you be open to the following? > > pseudocode: > > ``` > > if (camera3buffer->numFds == buf.numPlanes()) { > > // Act as if there's one buffer per plane (old behavior / ChromeOS) > > } else if (camera3buffer->numFds == 1) { > > // Act as if planes are stored contiguously in a single dmabuf (Android) > > } else { > > // Log Fatal: Unsupported state: number of FDs > > {camera3buffer->numFds} is not 1 (contiguous planes) and does not > > match number of planes {buf.numPlanes()}. Failed to infer buffer > > layout. > > } > > ``` > > This won't work on Qualcomm platforms, but that's likely something we > can ignore for now. I'm also worried about Android platforms that would > provide one dmabuf instance (and thus one fd) per plane, which is also > something that we could ignore for now I suppose. > > Looking at CameraDevice::createFrameBuffer(), we create a CameraBuffer > instance, which abstracts platform-specific buffer management. I would > like to use that to provides the fds we need to create the FrameBuffer, > instead of implementing platform-specific logic in createFrameBuffer() > directly. We could either extend the class to provides the fds, or > possibly better, to create a FrameBuffer instance. > > Any opinion ? > I concur with the observation that CameraBuffer should be abstracting platform-specific implementation details from the rest of the Camera HAL implementation, possibily with something like CameraBuffer::createFrameBuffer(const buffer_handle_t handle,..) I'm now looking at the ChromeOS CameraBufferManager implementation which abstracts away from us the need to set the right usages flags when, in example, importing buffers: https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/camera_buffer_manager_impl.cc#671 Standard Android platforms will not have the luxury of having a generic buffer manager as ChromeOS does, and each implemenation, if my understanding is correct, will need to encode in the Camera HAL knowledge of the platform specificities (such as buffers layout) and specific requirements of the gralloc implementation (usage flags). We right now have a cros buffer manager backend and a generic one which is, well, generic and can't be expected to work on all platforms as it is. I presume the direction to take would be to specialize that class to adapt it the platform the HAL is running on. Let me use the platform Nicholas' working on as an example: We know we could use [minigbm + rockchip kernel] for allocating NV12 buffers on the display. Let's assume the PinephonePro Waydroid build is instrumented to use such component. Should we expect to have an src/android/mm/rockchip_minigbm_camera_buffer.c implementation that adapts to the requirements of the Rockchip's minigbm implementation ? > > Let me know! > > > > Thanks, > > -Nicholas > > > > P.S. I'm moving, and the initial plan is short-term rentals. I may not > > have access to my PC for awhile. I've tried to put the steps required > > to replicate my development environment in > > https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit# > > and will continue to be active here. I'm also trying to use an IP-KVM > > to maintain access to my desktop but can't guarantee that will work. > > Thank you for putting those notes together and making them public. It's > useful. > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Dec 19, 2022 at 10:15:50AM +0100, Jacopo Mondi wrote: > On Mon, Dec 19, 2022 at 03:38:05AM +0200, Laurent Pinchart via libcamera-devel wrote: > > Hi Nicholas, > > > > (CC'ing Han-Lin and Harvey for the Chrome OS side, as well as Jacopo) > > > > On Sun, Dec 18, 2022 at 02:00:02PM -0600, Nicholas Roth wrote: > > > > I've replied to the bug report in > > > > https://bugs.libcamera.org/show_bug.cgi?id=172#c5. I'd like to > > > > investigate this further, as numFds != buf.numPlanes() doesn't sound > > > > right. > > > > > > My understanding from the differences between > > > android/mm/generic_camera_buffer.cpp (Android) and > > > android/mm/cros_camera_buffer.cpp (ChromeOS) is that camera3 on > > > Android each handle buffers differently. See also the comment in > > > generic_camera_buffer.cpp: > > > ``` > > > /* > > > * As Android doesn't offer an API to query buffer layouts, assume for > > > * now that the buffer is backed by a single dmabuf, with planes being > > > * stored contiguously. > > > */ > > > ``` > > > > I didn't interpret this to mean that android would give us a buffer with > > numFds == 1 even for multiplanar formats, but that all the file > > descriptors would point to the same dmabuf instance. However, looking at > > the gralloc implementations for Qualcomm platforms available at > > https://android.googlesource.com/platform/hardware/qcom/display/, for > > example the MSM8996 gralloc (https://android.googlesource.com/platform/hardware/qcom/display/+/refs/heads/master/msm8996/libgralloc/gralloc_priv.h#247), > > the situation is more complicated. numFds is set to 2, with the first fd > > storing the image buffer file descriptor (allocated from ION), and the > > second fd storing a metadata buffer file descriptor (not entirely sure > > what it's used for). > > > > The only reasonable conclusion is that Android is a mess. > > > > > I can't easily find "cros-camera/camera_buffer_manager.h" on the > > > public internet so I can't comment about that here. > > > > It's available at > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/include/cros-camera/camera_buffer_manager.h. > > > > > > This will break operation on Chrome OS. > > > > > > In that case, would you be open to the following? > > > pseudocode: > > > ``` > > > if (camera3buffer->numFds == buf.numPlanes()) { > > > // Act as if there's one buffer per plane (old behavior / ChromeOS) > > > } else if (camera3buffer->numFds == 1) { > > > // Act as if planes are stored contiguously in a single dmabuf (Android) > > > } else { > > > // Log Fatal: Unsupported state: number of FDs > > > {camera3buffer->numFds} is not 1 (contiguous planes) and does not > > > match number of planes {buf.numPlanes()}. Failed to infer buffer > > > layout. > > > } > > > ``` > > > > This won't work on Qualcomm platforms, but that's likely something we > > can ignore for now. I'm also worried about Android platforms that would > > provide one dmabuf instance (and thus one fd) per plane, which is also > > something that we could ignore for now I suppose. > > > > Looking at CameraDevice::createFrameBuffer(), we create a CameraBuffer > > instance, which abstracts platform-specific buffer management. I would > > like to use that to provides the fds we need to create the FrameBuffer, > > instead of implementing platform-specific logic in createFrameBuffer() > > directly. We could either extend the class to provides the fds, or > > possibly better, to create a FrameBuffer instance. > > > > Any opinion ? > > I concur with the observation that CameraBuffer should be abstracting > platform-specific implementation details from the rest of the Camera > HAL implementation, possibily with something like > CameraBuffer::createFrameBuffer(const buffer_handle_t handle,..) > > I'm now looking at the ChromeOS CameraBufferManager implementation > which abstracts away from us the need to set the right usages flags > when, in example, importing buffers: > https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/camera_buffer_manager_impl.cc#671 > > Standard Android platforms will not have the luxury of having a > generic buffer manager as ChromeOS does, and each implemenation, if my > understanding is correct, will need to encode in the Camera HAL > knowledge of the platform specificities (such as buffers layout) and > specific requirements of the gralloc implementation (usage flags). Until Android sort out their mess, I think that's right. I won't hold my breath, one of the big design principles of the operating system is that platform-specific components are meant to be developed together, and share common knowledge through hardcoding assumptions, without any standard API to communicate them. I doubt they will change that anytime soon. > We right now have a cros buffer manager backend and a generic one > which is, well, generic and can't be expected to work on all platforms > as it is. I presume the direction to take would be to specialize that > class to adapt it the platform the HAL is running on. I believe so, yes. We may not have to specialize it for every single Android platform though, there may be some platform-specific data that we could also obtain from the HAL configuration file. > Let me use the platform Nicholas' working on as an example: > > We know we could use [minigbm + rockchip kernel] for allocating NV12 > buffers on the display. Let's assume the PinephonePro Waydroid build > is instrumented to use such component. Should we expect to have an > src/android/mm/rockchip_minigbm_camera_buffer.c implementation that > adapts to the requirements of the Rockchip's minigbm implementation ? It depends what those requirements would be. One thing we'll likely have to specialize CameraBuffer for is the usage of the fds in the native_handle_t structure. If multiple platform use the same layout, possibly with other platform-specific differences that could be encoded in a configuration file (such as, for example, which pixel format IMPLEMENTATION_DEFINED is mapped to, assuming it doesn't depend too much on the usage flags), then a single CameraBuffer implementation could be used for all those platforms. > > > Let me know! > > > > > > Thanks, > > > -Nicholas > > > > > > P.S. I'm moving, and the initial plan is short-term rentals. I may not > > > have access to my PC for awhile. I've tried to put the steps required > > > to replicate my development environment in > > > https://docs.google.com/document/d/1Sly_VH3w6wFIdE72WXijQegoHZh8IxEnJ9m0hH7GodU/edit# > > > and will continue to be active here. I'm also trying to use an IP-KVM > > > to maintain access to my desktop but can't guarantee that will work. > > > > Thank you for putting those notes together and making them public. It's > > useful.
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b20e389b..1459d582 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -781,14 +781,18 @@ CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer, return nullptr; } + if (camera3buffer->numFds > 1) { + LOG(HAL, Fatal) << "Discontiguous planes are not supported"; + } + + SharedFD fd{ camera3buffer->data[0] }; + if (!fd.isValid()) { + LOG(HAL, Fatal) << "No valid fd"; + return nullptr; + } + std::vector<FrameBuffer::Plane> planes(buf.numPlanes()); for (size_t i = 0; i < buf.numPlanes(); ++i) { - SharedFD fd{ camera3buffer->data[i] }; - if (!fd.isValid()) { - LOG(HAL, Fatal) << "No valid fd"; - return nullptr; - } - planes[i].fd = fd; planes[i].offset = buf.offset(i); planes[i].length = buf.size(i);
Currently, they way camera_device.cpp handles file descriptors in a buffer_handle_t is incorrect. This can result in the HAL attempting to treat uninitialized memory like file descriptors, usually resulting in a crash. This patch brings the behavior of camera_device.cpp in line with how CameraBuffer handles file descriptors and planes. Bug: https://bugs.libcamera.org/show_bug.cgi?id=172 Signed-off-by: Nicholas Roth <nicholas@rothemail.net> --- src/android/camera_device.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)