[libcamera-devel,v2] android: CameraBuffer: Mark as invalid if cros::CameraBufferManager::Register() fails
diff mbox series

Message ID 20210402040340.1364749-1-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel,v2] android: CameraBuffer: Mark as invalid if cros::CameraBufferManager::Register() fails
Related show

Commit Message

Hirokazu Honda April 2, 2021, 4:03 a.m. UTC 
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@chromium.org>
---
 src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart April 3, 2021, 12:05 a.m. UTC | #1 
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@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@ideasonboard.com>

I'll apply the patch if you're OK with the proposed changes.

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 ?

> +		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;
Hirokazu Honda April 3, 2021, 10:49 a.m. UTC | #2 
On Sat, Apr 3, 2021 at 9:06 AM Laurent Pinchart
<laurent.pinchart@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@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@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
Laurent Pinchart April 3, 2021, 4:03 p.m. UTC | #3 
Hi Hiro,

On Sat, Apr 03, 2021 at 07:49:51PM +0900, Hirokazu Honda wrote:
> On Sat, Apr 3, 2021 at 9:06 AM Laurent Pinchart 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@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@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?

Both will log an error in those cases, so it's best to avoid that. Also,
the API documentation doesn't explicitly state that calling those
functions on buffers that haven't been registered and/or locked is safe,
so even if it is today, the implementation may change later. It's best
to be defensive (or get the CameraBufferManager API to explicitly state
that this is fine, and drop the error messages).

> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc
>
> > > +             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;

Patch
diff mbox series 

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;
+		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;