| Message ID | 20211018152524.78521-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Commit | 375aeafb58efea248f64832b2e529284385bc777 |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Oct 18, 2021 at 05:25:24PM +0200, Jacopo Mondi wrote: > Commit d165f7da34b8 ("android: camera_device: Configure one stream for > identical stream requests") introduced the ability to generate through > post-processing YUV streams of identical size and format. > > Howver the change didn't fully take into account the situation > where only mapped streams are contained in the request submitted by > the camera service to the HAL. In this case the Request will be queued > with no buffers and refused by the Camera. > > Even if this seems a corner case it causes a few CTS to fail, and more > problematically it triggers out-of-order completion of requests, causing > the camera service to abort. > > ERROR Camera camera.cpp:1031 Request contains no buffers > ERROR HAL camera_device.cpp:1109 '\_SB_.PCI0.I2C2.CAM0': Out-of-order completion for request 0x00007a1f1800ccd0 > ERROR cros_camera_service[15706:15711]: [camera_device_adapter.cc(744)] (15711) Notify(): Fatal device error; aborting the camera service > > Revert the commit until a proper solution is implemented. Sounds fine with me, as there's no urgency to support this use case. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 29 ----------------------------- > 1 file changed, 29 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 901867105085..bd34188809da 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -619,35 +619,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > continue; > } > > - /* > - * While gralloc usage flags are supposed to report usage > - * patterns to select a suitable buffer allocation strategy, in > - * practice they're also used to make other decisions, such as > - * selecting the actual format for the IMPLEMENTATION_DEFINED > - * HAL pixel format. To avoid issues, we thus have to set the > - * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for > - * streams that will be produced in software. > - */ > - stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > - > - /* > - * If a CameraStream with the same size and format of the > - * current stream has already been requested, associate the two. > - */ > - auto iter = std::find_if( > - streamConfigs.begin(), streamConfigs.end(), > - [size, format](const Camera3StreamConfig &streamConfig) { > - return streamConfig.config.size == size && > - streamConfig.config.pixelFormat == format; > - }); > - if (iter != streamConfigs.end()) { > - /* Add usage to copy the buffer in streams[0] to stream. */ > - iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN; > - stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN; > - iter->streams.push_back({ stream, CameraStream::Type::Mapped }); > - continue; > - } > - > Camera3StreamConfig streamConfig; > streamConfig.streams = { { stream, CameraStream::Type::Direct } }; > streamConfig.config.size = size;
Hi Jacopo, thank you for the patch. On Tue, Oct 19, 2021 at 3:10 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jacopo, > > Thank you for the patch. > > On Mon, Oct 18, 2021 at 05:25:24PM +0200, Jacopo Mondi wrote: > > Commit d165f7da34b8 ("android: camera_device: Configure one stream for > > identical stream requests") introduced the ability to generate through > > post-processing YUV streams of identical size and format. > > > > Howver the change didn't fully take into account the situation s/Howver/However/ > > where only mapped streams are contained in the request submitted by > > the camera service to the HAL. In this case the Request will be queued > > with no buffers and refused by the Camera. > > > > Even if this seems a corner case it causes a few CTS to fail, and more > > problematically it triggers out-of-order completion of requests, causing > > the camera service to abort. > > > > ERROR Camera camera.cpp:1031 Request contains no buffers > > ERROR HAL camera_device.cpp:1109 '\_SB_.PCI0.I2C2.CAM0': Out-of-order completion for request 0x00007a1f1800ccd0 > > ERROR cros_camera_service[15706:15711]: [camera_device_adapter.cc(744)] (15711) Notify(): Fatal device error; aborting the camera service > > > > Revert the commit until a proper solution is implemented. > > Sounds fine with me, as there's no urgency to support this use case. > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 29 ----------------------------- > > 1 file changed, 29 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 901867105085..bd34188809da 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -619,35 +619,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > continue; > > } > > > > - /* > > - * While gralloc usage flags are supposed to report usage > > - * patterns to select a suitable buffer allocation strategy, in > > - * practice they're also used to make other decisions, such as > > - * selecting the actual format for the IMPLEMENTATION_DEFINED > > - * HAL pixel format. To avoid issues, we thus have to set the > > - * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for > > - * streams that will be produced in software. > > - */ > > - stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > - > > - /* > > - * If a CameraStream with the same size and format of the > > - * current stream has already been requested, associate the two. > > - */ > > - auto iter = std::find_if( > > - streamConfigs.begin(), streamConfigs.end(), > > - [size, format](const Camera3StreamConfig &streamConfig) { > > - return streamConfig.config.size == size && > > - streamConfig.config.pixelFormat == format; > > - }); > > - if (iter != streamConfigs.end()) { > > - /* Add usage to copy the buffer in streams[0] to stream. */ > > - iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN; > > - stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN; > > - iter->streams.push_back({ stream, CameraStream::Type::Mapped }); > > - continue; > > - } > > - > > Camera3StreamConfig streamConfig; > > streamConfig.streams = { { stream, CameraStream::Type::Direct } }; > > streamConfig.config.size = size; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 901867105085..bd34188809da 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -619,35 +619,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) continue; } - /* - * While gralloc usage flags are supposed to report usage - * patterns to select a suitable buffer allocation strategy, in - * practice they're also used to make other decisions, such as - * selecting the actual format for the IMPLEMENTATION_DEFINED - * HAL pixel format. To avoid issues, we thus have to set the - * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for - * streams that will be produced in software. - */ - stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; - - /* - * If a CameraStream with the same size and format of the - * current stream has already been requested, associate the two. - */ - auto iter = std::find_if( - streamConfigs.begin(), streamConfigs.end(), - [size, format](const Camera3StreamConfig &streamConfig) { - return streamConfig.config.size == size && - streamConfig.config.pixelFormat == format; - }); - if (iter != streamConfigs.end()) { - /* Add usage to copy the buffer in streams[0] to stream. */ - iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN; - stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN; - iter->streams.push_back({ stream, CameraStream::Type::Mapped }); - continue; - } - Camera3StreamConfig streamConfig; streamConfig.streams = { { stream, CameraStream::Type::Direct } }; streamConfig.config.size = size;
Commit d165f7da34b8 ("android: camera_device: Configure one stream for identical stream requests") introduced the ability to generate through post-processing YUV streams of identical size and format. Howver the change didn't fully take into account the situation where only mapped streams are contained in the request submitted by the camera service to the HAL. In this case the Request will be queued with no buffers and refused by the Camera. Even if this seems a corner case it causes a few CTS to fail, and more problematically it triggers out-of-order completion of requests, causing the camera service to abort. ERROR Camera camera.cpp:1031 Request contains no buffers ERROR HAL camera_device.cpp:1109 '\_SB_.PCI0.I2C2.CAM0': Out-of-order completion for request 0x00007a1f1800ccd0 ERROR cros_camera_service[15706:15711]: [camera_device_adapter.cc(744)] (15711) Notify(): Fatal device error; aborting the camera service Revert the commit until a proper solution is implemented. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 29 ----------------------------- 1 file changed, 29 deletions(-)