| Message ID | 20200803161816.107113-9-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Mon, Aug 03, 2020 at 05:18:12PM +0100, Kieran Bingham wrote: > Use lseek to query the length of planes where possible rather than leaving > the plane.length as zero, which prevents mapping buffers for software > processing. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > v2: > - no longer support an invalid plane length. > --- > src/android/camera_device.cpp | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 78b0246e40f0..9f3f3315aed4 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1036,12 +1036,13 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer > break; > } > > - /* > - * Setting length to zero here is OK as the length is only used > - * to map the memory of the plane. Libcamera do not need to poke > - * at the memory content queued by the HAL. > - */ > - plane.length = 0; > + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > + if (length == -1) { > + LOG(HAL, Error) << "Failed to query plane length"; > + break; > + } > + > + plane.length = length; > planes.push_back(std::move(plane)); > } >
On Mon, Aug 03, 2020 at 07:30:59PM +0300, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Mon, Aug 03, 2020 at 05:18:12PM +0100, Kieran Bingham wrote: > > Use lseek to query the length of planes where possible rather than leaving > > the plane.length as zero, which prevents mapping buffers for software > > processing. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > v2: > > - no longer support an invalid plane length. > > --- > > src/android/camera_device.cpp | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 78b0246e40f0..9f3f3315aed4 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1036,12 +1036,13 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer > > break; > > } > > > > - /* > > - * Setting length to zero here is OK as the length is only used > > - * to map the memory of the plane. Libcamera do not need to poke > > - * at the memory content queued by the HAL. > > - */ > > - plane.length = 0; > > + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > + if (length == -1) { > > + LOG(HAL, Error) << "Failed to query plane length"; > > + break; I spoke too fast. This will cause the function to return a FrameBuffer with a too small number of planes. You should instead return nullptr here. The Rb tag still holds. > > + } > > + > > + plane.length = length; > > planes.push_back(std::move(plane)); > > } > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 78b0246e40f0..9f3f3315aed4 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1036,12 +1036,13 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer break; } - /* - * Setting length to zero here is OK as the length is only used - * to map the memory of the plane. Libcamera do not need to poke - * at the memory content queued by the HAL. - */ - plane.length = 0; + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); + if (length == -1) { + LOG(HAL, Error) << "Failed to query plane length"; + break; + } + + plane.length = length; planes.push_back(std::move(plane)); }
Use lseek to query the length of planes where possible rather than leaving the plane.length as zero, which prevents mapping buffers for software processing. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- v2: - no longer support an invalid plane length. --- src/android/camera_device.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)