| Message ID | 20210901080302.68525-3-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote: > An Android HAL client may request identical stream requests. It is s/identical stream requests/multiple identical streams/ > redundant that a native camera device produces a separate stream for > each of the identical requests. > Configure camera one stream configuration for the identical stream > requests. And here I'd write Configure the camera with a single stream in that case. The other identical HAL streams will be produced by the YUV post-processor. > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 324b997f..51d5370e 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > continue; > } > > + /* > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on > + * ChromeOS because cros::CameraBufferManager imports the stream > + * buffer with the usage. I'm still not sure to follow you. What happens if you don't set GRALLOC_USAGE_HW_CAMERA_WRITE ? > + */ > +#if defined(OS_CHROMEOS) > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > +#endif > + > + /* > + * 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; > streamConfig.config.pixelFormat = format; > streamConfigs.push_back(std::move(streamConfig)); > - This isn't needed. > /* This stream will be produced by hardware. */ > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > }
Hi Laurent, On Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote: > > An Android HAL client may request identical stream requests. It is > > s/identical stream requests/multiple identical streams/ > > > redundant that a native camera device produces a separate stream for > > each of the identical requests. > > Configure camera one stream configuration for the identical stream > > requests. > > And here I'd write > > Configure the camera with a single stream in that case. The other > identical HAL streams will be produced by the YUV post-processor. > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 324b997f..51d5370e 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > continue; > > } > > > > + /* > > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on > > + * ChromeOS because cros::CameraBufferManager imports the stream > > + * buffer with the usage. > > I'm still not sure to follow you. What happens if you don't set > GRALLOC_USAGE_HW_CAMERA_WRITE ? This gmb_bo_import call fails. That is, the buffer cannot be mapped and even constructing CameraBuffer fails. https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e Well, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand they want to add it for safety. -Hiro > > > + */ > > +#if defined(OS_CHROMEOS) > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > +#endif > > + > > + /* > > + * 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; > > streamConfig.config.pixelFormat = format; > > streamConfigs.push_back(std::move(streamConfig)); > > - > > This isn't needed. > > > /* This stream will be produced by hardware. */ > > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > } > > -- > Regards, > > Laurent Pinchart
Hi Hiro, Sorry for the late reply. On Fri, Sep 03, 2021 at 06:11:30PM +0900, Hirokazu Honda wrote: > On Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart wrote: > > On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote: > > > An Android HAL client may request identical stream requests. It is > > > > s/identical stream requests/multiple identical streams/ > > > > > redundant that a native camera device produces a separate stream for > > > each of the identical requests. > > > Configure camera one stream configuration for the identical stream > > > requests. > > > > And here I'd write > > > > Configure the camera with a single stream in that case. The other > > identical HAL streams will be produced by the YUV post-processor. > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 324b997f..51d5370e 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > continue; > > > } > > > > > > + /* > > > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on > > > + * ChromeOS because cros::CameraBufferManager imports the stream > > > + * buffer with the usage. > > > > I'm still not sure to follow you. What happens if you don't set > > GRALLOC_USAGE_HW_CAMERA_WRITE ? > > This gmb_bo_import call fails. That is, the buffer cannot be mapped > and even constructing CameraBuffer fails. > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e > > Well, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand > they want to add it for safety. Right. This bothers me a bit, but it's not your fault :-) The gralloc usage flags are supposed to indicate expected usage of a buffer to allow selection of a suitable memory allocation strategy compatible with all devices involved. In practice, it's also used to pick pixel formats, so we need to specify GRALLOC_USAGE_HW_CAMERA_WRITE for all buffers that will be filled by the camera HAL, or the wrong format will be selected (or allocation could fail completely). That's a design deficiency of the gralloc and camera HAL APIs in my opinion, but not something we can address. I expect the same to be true for other Android system, so I think it would make sense to set GRALLOC_USAGE_HW_CAMERA_WRITE unconditionally. How about this ? /* * While gralloc usage flags are supposed to report usage * patterns to select a suitable buffer allocation strategy, in * practive 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 you're fine with this, I'll push the series with this change. > > > + */ > > > +#if defined(OS_CHROMEOS) > > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > > +#endif > > > + > > > + /* > > > + * 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; > > > streamConfig.config.pixelFormat = format; > > > streamConfigs.push_back(std::move(streamConfig)); > > > - > > > > This isn't needed. > > > > > /* This stream will be produced by hardware. */ > > > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > > }
Hi Laurent, On Wed, Sep 22, 2021 at 9:54 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Sorry for the late reply. > > On Fri, Sep 03, 2021 at 06:11:30PM +0900, Hirokazu Honda wrote: > > On Fri, Sep 3, 2021 at 5:36 PM Laurent Pinchart wrote: > > > On Wed, Sep 01, 2021 at 05:03:02PM +0900, Hirokazu Honda wrote: > > > > An Android HAL client may request identical stream requests. It is > > > > > > s/identical stream requests/multiple identical streams/ > > > > > > > redundant that a native camera device produces a separate stream for > > > > each of the identical requests. > > > > Configure camera one stream configuration for the identical stream > > > > requests. > > > > > > And here I'd write > > > > > > Configure the camera with a single stream in that case. The other > > > identical HAL streams will be produced by the YUV post-processor. > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/android/camera_device.cpp | 28 +++++++++++++++++++++++++++- > > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 324b997f..51d5370e 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > continue; > > > > } > > > > > > > > + /* > > > > + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on > > > > + * ChromeOS because cros::CameraBufferManager imports the stream > > > > + * buffer with the usage. > > > > > > I'm still not sure to follow you. What happens if you don't set > > > GRALLOC_USAGE_HW_CAMERA_WRITE ? > > > > This gmb_bo_import call fails. That is, the buffer cannot be mapped > > and even constructing CameraBuffer fails. > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc;l=669;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e > > > > Well, GBM_BO_USE_CAMERA_WRITE may be unnecessary, but I can understand > > they want to add it for safety. > > Right. This bothers me a bit, but it's not your fault :-) The gralloc > usage flags are supposed to indicate expected usage of a buffer to allow > selection of a suitable memory allocation strategy compatible with all > devices involved. In practice, it's also used to pick pixel formats, so > we need to specify GRALLOC_USAGE_HW_CAMERA_WRITE for all buffers that > will be filled by the camera HAL, or the wrong format will be selected > (or allocation could fail completely). That's a design deficiency of the > gralloc and camera HAL APIs in my opinion, but not something we can > address. > > I expect the same to be true for other Android system, so I think it > would make sense to set GRALLOC_USAGE_HW_CAMERA_WRITE unconditionally. > How about this ? > > /* > * While gralloc usage flags are supposed to report usage > * patterns to select a suitable buffer allocation strategy, in > * practive they're also used to make other decisions, such as s/practive/practice/. > * 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 you're fine with this, I'll push the series with this change. > The comment desciption looks good. Thanks. -Hiro > > > > + */ > > > > +#if defined(OS_CHROMEOS) > > > > + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > > > +#endif > > > > + > > > > + /* > > > > + * 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; > > > > streamConfig.config.pixelFormat = format; > > > > streamConfigs.push_back(std::move(streamConfig)); > > > > - > > > > > > This isn't needed. > > > > > > > /* This stream will be produced by hardware. */ > > > > stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; > > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 324b997f..51d5370e 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -618,12 +618,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) continue; } + /* + * Always add GRALLOC_USAGE_HW_CAMERA_WRITE to the usage on + * ChromeOS because cros::CameraBufferManager imports the stream + * buffer with the usage. + */ +#if defined(OS_CHROMEOS) + stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; +#endif + + /* + * 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; streamConfig.config.pixelFormat = format; streamConfigs.push_back(std::move(streamConfig)); - /* This stream will be produced by hardware. */ stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE; }