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

[Kieran Bingham]

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 ...

--
Kieran

> Best regards,
> Tomasz
> 

-- 
Regards
--
Kieran