[libcamera-devel] [PATCH v2 18/27] android: camera_device: Don't assume all planes use the same fd

[Hirokazu Honda]

Hi Laurent,

On Mon, Sep 6, 2021 at 10:53 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Mon, Sep 06, 2021 at 10:35:46PM +0900, Hirokazu Honda wrote:
> > On Mon, Sep 6, 2021 at 4:38 PM Jean-Michel Hautbois wrote:
> > > On 06/09/2021 04:00, Laurent Pinchart wrote:
> > > > Now that libcamera correctly supports frame buffers with different
> > > > dmabuf for each plane, remove the assumption that a single dmabuf is
> > > > used.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > > ---
> > > >  src/android/camera_device.cpp | 25 ++++++-------------------
> > > >  1 file changed, 6 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 8ca76719a50f..c64064106ccc 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -749,25 +749,6 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> > > >                                            libcamera::PixelFormat pixelFormat,
> > > >                                            const libcamera::Size &size)
> > > >  {
> > > > -     FileDescriptor fd;
> > > > -     /*
> > > > -      * This assumes all the planes are in the same dmabuf.
> > > > -      *
> > > > -      * \todo Verify that this assumption holds, fstat() can be used to check
> > > > -      * if two fds refer to the same dmabuf.
> > > > -      */
> > > > -     for (int i = 0; i < camera3buffer->numFds; i++) {
> > > > -             if (camera3buffer->data[i] != -1) {
> > > > -                     fd = FileDescriptor(camera3buffer->data[i]);
> > > > -                     break;
> > > > -             }
> > > > -     }
> > > > -
> > > > -     if (!fd.isValid()) {
> > > > -             LOG(HAL, Fatal) << "No valid fd";
> > > > -             return nullptr;
> > > > -     }
> > > > -
> > > >       CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
> > > >       if (!buf.isValid()) {
> > > >               LOG(HAL, Fatal) << "Failed to create CameraBuffer";
> > > > @@ -776,6 +757,12 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> > > >
> > > >       std::vector<FrameBuffer::Plane> planes(buf.numPlanes());
> > > >       for (size_t i = 0; i < buf.numPlanes(); ++i) {
> >
> > I think it is not guaranteed that numFds is necessarily the same as
> > buf.numPlanes().
>
> Oh ? What's the expected behaviour then ? Is there any documentation
> about all this ?

I could not find the documentation about it.
Maybe they must be all the same or at least larther than num planes.
In chromeos, numFds looks always four and fills the same number of fds
as num planes.
So I think this code is fine.

-Hiro
>
> > > > +             FileDescriptor fd{ camera3buffer->data[i] };
> > > > +             if (!fd.isValid()) {
> > > > +                     LOG(HAL, Fatal) << "No valid fd";
> > > > +                     return nullptr;
> > > > +             }
> > > > +
> > > >               planes[i].fd = fd;
> > > >               planes[i].offset = buf.offset(i);
> > > >               planes[i].length = buf.size(i);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart