[libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query plane length

[Tomasz Figa]

On Mon, Jul 27, 2020 at 4:21 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi All,
>
> On 27/07/2020 14:45, Tomasz Figa wrote:
> > On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart
> > <laurent.pinchart at ideasonboard.com> wrote:
> >>
> >> Hi Kieran,
> >>
> >> (CC'ing Tomasz to get a review from a Chrome OS point of view)
> >>
> >> Thank you for the patch.
> >>
> >> On Mon, Jul 20, 2020 at 11:42:31PM +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 at ideasonboard.com>
> >>> ---
> >>>
> >>> I like this one ;-) We need to know the plane lengths for software
> >>> streams now, but the correct way to get this would be to talk to the
> >>> gralloc allocator. This would mean pulling in various extra
> >>> cros-specific libraries, where instead we can query the size of the
> >>> dmabuf by getting the offset when we SEEK_END.
> >>>
> >>>
> >>>  src/android/camera_device.cpp | 18 ++++++++++++------
> >>>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>> index 538b8ab5da03..6212ccdd61ec 100644
> >>> --- a/src/android/camera_device.cpp
> >>> +++ b/src/android/camera_device.cpp
> >>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> >>>                       continue;
> >>>               }
> >>>
> >>> -             /*
> >>> -              * 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) {
> >>> +                     /*
> >>> +                      * A zero length plane is not fatal unless the
> >>> +                      * FrameBuffer is used for a software stream, libcamera
> >>> +                      * itself will not access the internal frame content.
> >>> +                      */
> >>> +                     LOG(HAL, Error) << "Failed to query plane length";
> >>> +                     length = 0;
> >>
> >> Do you expect this to happen ? If not, I'd return an error. If yes, an
> >> error message would be printed in a loop, which isn't very nice. My
> >> preference would be to return an error if possible.
> >>
>
> I don't think I expect it to happen.
>
> >> Otherwise, nice hack :-)
> >
> > A zero-length plane would be strange and could suggest that the
> > backing file is not a DMA-buf, so it could indeed make sense to error
> > out.
>
> I suspect I coded this thinking that 0 was currently acceptable, so it
> might still be acceptable to use.
>
> But I agree with you both that if we can't determine the length of the
> buffer then we have an error and should highlight it accordingly.
>
>
> > Otherwise, we tend to do this in a number of places in Chrome OS and
> > seems to be the official way to query a DMA-buf for its size. However
> > I wonder if we shouldn't restore the original position after the seek?
>
> Does CrOS do this? It's an extra couple of calls, (SEEK_CUR, and
> SEEK_SET), but I hope they're not too expensive ...
>
>
>
> I would expect anyone using a DMABuf will mmap the data if needed rather
> than use read()/write(), but that's probably making too big an
> assumption, so indeed if we left it at SEEK_END we might break something ...

The views on this are separated even in Chrome OS. There are places
where we simply set it back to 0 and there are places where we do a
proper save, change and restore. I don't have a strong opinion on this
and I'm pretty much sure we don't have any read/write calls to
DMA-bufs in Chrome OS, but there could be some implications on other
platforms, so I've intentionally made the last sentence a question. :)

Best regards,
Tomasz