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

[Laurent Pinchart]

Hi Tomasz,

On Mon, Jul 27, 2020 at 04:30:08PM +0200, Tomasz Figa wrote:
> On Mon, Jul 27, 2020 at 4:21 PM Kieran Bingham wrote:
> > On 27/07/2020 14:45, Tomasz Figa wrote:
> >> On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart 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. :)

dmabuf doesn't support read/write, so I think it's safe to leave it
as-is.

-- 
Regards,

Laurent Pinchart