[libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as invalid if cros::CameraBufferManager::Register() fails

[Hirokazu Honda]

On Sat, Apr 3, 2021 at 9:06 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> I'd write the subject line as "android: mm: cros: Handle buffer
> registration failure" to shorten it.
>
> On Fri, Apr 02, 2021 at 01:03:40PM +0900, Hirokazu Honda wrote:
> > cros::CameraBufferManager::Register() fails if a buffer handle
> > is invalid. We should mark CameraBuffer as invalid on the failure
> > of Register().
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > index 7df4f47c..5f98ee44 100644
> > --- a/src/android/mm/cros_camera_buffer.cpp
> > +++ b/src/android/mm/cros_camera_buffer.cpp
> > @@ -53,12 +53,16 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> >  {
> >       bufferManager_ = cros::CameraBufferManager::GetInstance();
> >
> > -     bufferManager_->Register(camera3Buffer);
> > +     int ret = bufferManager_->Register(camera3Buffer);
> > +     if (ret) {
> > +             LOG(HAL, Error) << "Failed registering a buffer: " << ret;
>
> s/a buffer/buffer/
>
> This looks good,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I'll apply the patch if you're OK with the proposed changes.
>

I'm fine indeed.

> One additional question though. The CameraBuffer::Private destructors
> calls
>
>         bufferManager_->Unlock(handle_);
>         bufferManager_->Deregister(handle_);
>
> unconditionally. Is that safe to do if the buffer hasn't been registered
> and locked properly ?
>

I looked the implementation of CameraBufferManagerImpl. [1]
Looks like both functions return -EINVAL if Register() and
Lock/LockYCbCr() are not successful.
Well, it should be safer to check here, but we add the if-condition check here.
Shall I upload the new patch with them?

[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc

Best Regards,
-Hiro
> > +             return;
> > +     }
> >
> >       numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> >       switch (numPlanes_) {
> >       case 1: {
> > -             int ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > +             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> >               if (ret) {
> >                       LOG(HAL, Error) << "Single plane buffer mapping failed";
> >                       return;
> > @@ -67,8 +71,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> >       }
> >       case 2:
> >       case 3: {
> > -             int ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > -                                                 &mem.ycbcr);
> > +             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > +                                             &mem.ycbcr);
> >               if (ret) {
> >                       LOG(HAL, Error) << "YCbCr buffer mapping failed";
> >                       return;
>
> --
> Regards,
>
> Laurent Pinchart