[libcamera-devel,00/10] libcamera: Introduce Fence support
mbox series

Message ID 20211028111520.256612-1-jacopo@jmondi.org
Headers show
Series
  • libcamera: Introduce Fence support
Related show

Message

Jacopo Mondi Oct. 28, 2021, 11:15 a.m. UTC 
Fences are a synchronization mechanism that allows to wait for events
on a file descriptor with an optional timeout.

So far the only user of synchronization fencs is the Android HAL, for which
a dedicated class performs fences wait before queueing the Request to the
Camera.

This series move handling of fences to libcamera core to allow generic
application to attach a synchronization fence to a FrameBuffer.

Design principles in the series are:

- Fences are passed and retrieved to/from FrameBuffer by their file-descriptor
  values
- Fences are attacched to a FrameBuffer and their value is valid until the
  Request is queued to the Camera
- When a Request completes the fence file descriptor value will read as -1
  if the core has handled the fence correctly
- If any error occourred waiting on the fence, the fence file descriptor
  is not closed and its value is available from the file descritpro

To realize that an internal Fence class has been introduced and used by the
FrameBuffer::Private and Request::Private classes.

The pipeline handler receives a Request and handles valid fence in the
Request's framebuffers. Once all Fences have been waited on, the Request is
finally queued to the device. If any fence has expired or handling it has
failed, the Request is cancelled and not queued to the device.

Tested on ChromeOS by using CCA (which does not use fences), OpenCamera (which
uses fences) and by several runs of CTS whose results are in the order of 2 to 3
failed tests

Total Run time: 20m 17s
1/1 modules completed
Total Tests       : 231
PASSED            : 228
FAILED            : 3

With a few RecordingTest failures (expected) and some flukes in
android.hardware.camera2.cts.SurfaceViewPreviewTest#testSurfaceSet
and a capture timeout manifestin randomly in other tests
which will be investigated.

Thanks
   j

Jacopo Mondi (9):
  libcamera: event_notifier: Add 'enable' constructor parameter
  libcamera: Introduce Fence class
  test: Add test for the Fence class
  libcamera: request: Add support for fences
  libcamera: framebuffer: Add synchronization Fence
  libcamera: pipeline_handler: Split request queueing
  libcamera: pipeline: Introduce stopDevice()
  libcamera: pipeline_handler: Handle fences
  android: Remove CameraWorker

Laurent Pinchart (1):
  libcamera: request: Make Request class Extensible

 include/libcamera/base/event_notifier.h       |   2 +-
 include/libcamera/framebuffer.h               |   5 +-
 include/libcamera/internal/fence.h            |  64 ++++++
 include/libcamera/internal/framebuffer.h      |   7 +-
 include/libcamera/internal/meson.build        |   2 +
 include/libcamera/internal/pipeline_handler.h |  12 +-
 include/libcamera/internal/request.h          |  55 ++++++
 include/libcamera/request.h                   |   6 +-
 src/android/camera_device.cpp                 |  39 ++--
 src/android/camera_device.h                   |   5 +-
 src/android/camera_request.cpp                |   3 +-
 src/android/camera_request.h                  |   3 +-
 src/android/camera_worker.cpp                 | 129 ------------
 src/android/camera_worker.h                   |  72 -------
 src/android/meson.build                       |   1 -
 src/libcamera/base/event_notifier.cpp         |  12 +-
 src/libcamera/fence.cpp                       | 185 ++++++++++++++++++
 src/libcamera/framebuffer.cpp                 |  46 ++++-
 src/libcamera/meson.build                     |   1 +
 src/libcamera/pipeline/ipu3/ipu3.cpp          |   4 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      |   4 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   4 +-
 src/libcamera/pipeline/simple/simple.cpp      |   4 +-
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |   4 +-
 src/libcamera/pipeline/vimc/vimc.cpp          |   4 +-
 src/libcamera/pipeline_handler.cpp            | 141 ++++++++++++-
 src/libcamera/request.cpp                     | 137 ++++++++++++-
 test/fence.cpp                                | 148 ++++++++++++++
 test/meson.build                              |   1 +
 29 files changed, 824 insertions(+), 276 deletions(-)
 create mode 100644 include/libcamera/internal/fence.h
 create mode 100644 include/libcamera/internal/request.h
 delete mode 100644 src/android/camera_worker.cpp
 delete mode 100644 src/android/camera_worker.h
 create mode 100644 src/libcamera/fence.cpp
 create mode 100644 test/fence.cpp

--
2.33.1

Comments

Kieran Bingham Nov. 9, 2021, 12:08 p.m. UTC | #1 
Quoting Jacopo Mondi (2021-10-28 12:15:11)
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Implement the D-Pointer design pattern in the Request class to allow
> changing internal data without affecting the public ABI.
> 
> Move a few internal fields that are not needed to implement the public
> API to the Request::Private class already. More fields may be moved
> later.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Happy to get this patch merged earlier if it helps too, as it unblocks
other patches.


> ---
>  include/libcamera/internal/meson.build |  1 +
>  include/libcamera/internal/request.h   | 34 ++++++++++++++++++
>  include/libcamera/request.h            |  6 ++--
>  src/libcamera/pipeline_handler.cpp     |  7 ++--
>  src/libcamera/request.cpp              | 50 +++++++++++++++++++++-----
>  5 files changed, 84 insertions(+), 14 deletions(-)
>  create mode 100644 include/libcamera/internal/request.h
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 665fd6de4ed3..cae65b0604ff 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -34,6 +34,7 @@ libcamera_internal_headers = files([
>      'pipeline_handler.h',
>      'process.h',
>      'pub_key.h',
> +    'request.h',
>      'source_paths.h',
>      'sysfs.h',
>      'v4l2_device.h',
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> new file mode 100644
> index 000000000000..df0cc014067e
> --- /dev/null
> +++ b/include/libcamera/internal/request.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * request.h - Request class private data
> + */
> +#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
> +#define __LIBCAMERA_INTERNAL_REQUEST_H__
> +
> +#include <libcamera/request.h>
> +
> +namespace libcamera {
> +
> +class Camera;
> +class FrameBuffer;
> +
> +class Request::Private : public Extensible::Private
> +{
> +       LIBCAMERA_DECLARE_PUBLIC(Request)
> +
> +public:
> +       Private(Camera *camera);
> +       ~Private();
> +
> +       Camera *camera() const { return camera_; }
> +
> +private:
> +       Camera *camera_;
> +       bool cancelled_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index d16904e6b679..d6bd0ba2ac17 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -25,8 +25,10 @@ class CameraControlValidator;
>  class FrameBuffer;
>  class Stream;
>  
> -class Request
> +class Request : public Extensible
>  {
> +       LIBCAMERA_DECLARE_PRIVATE()
> +
>  public:
>         enum Status {
>                 RequestPending,
> @@ -70,7 +72,6 @@ private:
>  
>         bool completeBuffer(FrameBuffer *buffer);
>  
> -       Camera *camera_;
>         ControlList *controls_;
>         ControlList *metadata_;
>         BufferMap bufferMap_;
> @@ -79,7 +80,6 @@ private:
>         uint32_t sequence_;
>         const uint64_t cookie_;
>         Status status_;
> -       bool cancelled_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index f69c4f03b80f..cada864687ff 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -19,6 +19,7 @@
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/request.h"
>  #include "libcamera/internal/tracepoints.h"
>  
>  /**
> @@ -311,7 +312,7 @@ void PipelineHandler::queueRequest(Request *request)
>  {
>         LIBCAMERA_TRACEPOINT(request_queue, request);
>  
> -       Camera *camera = request->camera_;
> +       Camera *camera = request->_d()->camera();
>         Camera::Private *data = camera->_d();
>         data->queuedRequests_.push_back(request);
>  
> @@ -360,7 +361,7 @@ void PipelineHandler::queueRequest(Request *request)
>   */
>  bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>  {
> -       Camera *camera = request->camera_;
> +       Camera *camera = request->_d()->camera();
>         camera->bufferCompleted.emit(request, buffer);
>         return request->completeBuffer(buffer);
>  }
> @@ -381,7 +382,7 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>   */
>  void PipelineHandler::completeRequest(Request *request)
>  {
> -       Camera *camera = request->camera_;
> +       Camera *camera = request->_d()->camera();
>  
>         request->complete();
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 17fefab7ad0e..33fee1ac05ba 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -20,10 +20,11 @@
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
>  #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/request.h"
>  #include "libcamera/internal/tracepoints.h"
>  
>  /**
> - * \file request.h
> + * \file libcamera/request.h
>   * \brief Describes a frame capture request to be processed by a camera
>   */
>  
> @@ -31,6 +32,37 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(Request)
>  
> +/**
> + * \class Request::Private
> + * \brief Request private data
> + *
> + * The Request::Private class stores all private data associated with a
> + * request. It implements the d-pointer design pattern to hide core
> + * Request data from the public API, and exposes utility functions to
> + * internal users of the request (namely the PipelineHandler class and its
> + * subclasses).
> + */
> +
> +/**
> + * \brief Create a Request::Private
> + * \param camera The Camera that creates the request
> + */
> +Request::Private::Private(Camera *camera)
> +       : camera_(camera), cancelled_(false)
> +{
> +}
> +
> +Request::Private::~Private()
> +{
> +}
> +
> +/**
> + * \fn Request::Private::camera()
> + * \brief Retrieve the camera this request has been queued to
> + * \return The Camera this request has been queued to, or nullptr if the
> + * request hasn't been queued
> + */
> +
>  /**
>   * \enum Request::Status
>   * Request completion status
> @@ -75,8 +107,8 @@ LOG_DEFINE_CATEGORY(Request)
>   * completely opaque to libcamera.
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -       : camera_(camera), sequence_(0), cookie_(cookie),
> -         status_(RequestPending), cancelled_(false)
> +       : Extensible(std::make_unique<Private>(camera)), sequence_(0),
> +         cookie_(cookie), status_(RequestPending)
>  {
>         controls_ = new ControlList(controls::controls,
>                                     camera->_d()->validator());
> @@ -126,7 +158,7 @@ void Request::reuse(ReuseFlag flags)
>  
>         sequence_ = 0;
>         status_ = RequestPending;
> -       cancelled_ = false;
> +       _d()->cancelled_ = false;
>  
>         controls_->clear();
>         metadata_->clear();
> @@ -282,7 +314,7 @@ void Request::complete()
>         ASSERT(status_ == RequestPending);
>         ASSERT(!hasPendingBuffers());
>  
> -       status_ = cancelled_ ? RequestCancelled : RequestComplete;
> +       status_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;
>  
>         LOG(Request, Debug) << toString();
>  
> @@ -299,17 +331,19 @@ void Request::complete()
>   */
>  void Request::cancel()
>  {
> +       Private *const d = _d();
> +
>         LIBCAMERA_TRACEPOINT(request_cancel, this);
>  
>         ASSERT(status_ == RequestPending);
>  
>         for (FrameBuffer *buffer : pending_) {
>                 buffer->cancel();
> -               camera_->bufferCompleted.emit(this, buffer);
> +               d->camera_->bufferCompleted.emit(this, buffer);
>         }
>  
>         pending_.clear();
> -       cancelled_ = true;
> +       d->cancelled_ = true;
>  }
>  
>  /**
> @@ -335,7 +369,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)
>         buffer->_d()->setRequest(nullptr);
>  
>         if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> -               cancelled_ = true;
> +               _d()->cancelled_ = true;
>  
>         return !hasPendingBuffers();
>  }
> -- 
> 2.33.1
>
Jacopo Mondi Nov. 11, 2021, 8:30 a.m. UTC | #2 
On Tue, Nov 09, 2021 at 12:08:50PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-10-28 12:15:11)
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Implement the D-Pointer design pattern in the Request class to allow
> > changing internal data without affecting the public ABI.
> >
> > Move a few internal fields that are not needed to implement the public
> > API to the Request::Private class already. More fields may be moved
> > later.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Happy to get this patch merged earlier if it helps too, as it unblocks
> other patches.

FTR I get this warning from checkstyle on this patch.
Warning that I'm afraid I don't understand

--- src/libcamera/request.cpp
+++ src/libcamera/request.cpp
@@ -15,12 +15,12 @@
 #include <libcamera/camera.h>
 #include <libcamera/control_ids.h>
 #include <libcamera/framebuffer.h>
+#include <libcamera/request.h>
 #include <libcamera/stream.h>

 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/camera_controls.h"
 #include "libcamera/internal/framebuffer.h"
-#include "libcamera/internal/request.h"
 #include "libcamera/internal/tracepoints.h"

 /**

>
>
> > ---
> >  include/libcamera/internal/meson.build |  1 +
> >  include/libcamera/internal/request.h   | 34 ++++++++++++++++++
> >  include/libcamera/request.h            |  6 ++--
> >  src/libcamera/pipeline_handler.cpp     |  7 ++--
> >  src/libcamera/request.cpp              | 50 +++++++++++++++++++++-----
> >  5 files changed, 84 insertions(+), 14 deletions(-)
> >  create mode 100644 include/libcamera/internal/request.h
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 665fd6de4ed3..cae65b0604ff 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -34,6 +34,7 @@ libcamera_internal_headers = files([
> >      'pipeline_handler.h',
> >      'process.h',
> >      'pub_key.h',
> > +    'request.h',
> >      'source_paths.h',
> >      'sysfs.h',
> >      'v4l2_device.h',
> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> > new file mode 100644
> > index 000000000000..df0cc014067e
> > --- /dev/null
> > +++ b/include/libcamera/internal/request.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * request.h - Request class private data
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
> > +#define __LIBCAMERA_INTERNAL_REQUEST_H__
> > +
> > +#include <libcamera/request.h>
> > +
> > +namespace libcamera {
> > +
> > +class Camera;
> > +class FrameBuffer;
> > +
> > +class Request::Private : public Extensible::Private
> > +{
> > +       LIBCAMERA_DECLARE_PUBLIC(Request)
> > +
> > +public:
> > +       Private(Camera *camera);
> > +       ~Private();
> > +
> > +       Camera *camera() const { return camera_; }
> > +
> > +private:
> > +       Camera *camera_;
> > +       bool cancelled_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index d16904e6b679..d6bd0ba2ac17 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -25,8 +25,10 @@ class CameraControlValidator;
> >  class FrameBuffer;
> >  class Stream;
> >
> > -class Request
> > +class Request : public Extensible
> >  {
> > +       LIBCAMERA_DECLARE_PRIVATE()
> > +
> >  public:
> >         enum Status {
> >                 RequestPending,
> > @@ -70,7 +72,6 @@ private:
> >
> >         bool completeBuffer(FrameBuffer *buffer);
> >
> > -       Camera *camera_;
> >         ControlList *controls_;
> >         ControlList *metadata_;
> >         BufferMap bufferMap_;
> > @@ -79,7 +80,6 @@ private:
> >         uint32_t sequence_;
> >         const uint64_t cookie_;
> >         Status status_;
> > -       bool cancelled_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index f69c4f03b80f..cada864687ff 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -19,6 +19,7 @@
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/device_enumerator.h"
> >  #include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/request.h"
> >  #include "libcamera/internal/tracepoints.h"
> >
> >  /**
> > @@ -311,7 +312,7 @@ void PipelineHandler::queueRequest(Request *request)
> >  {
> >         LIBCAMERA_TRACEPOINT(request_queue, request);
> >
> > -       Camera *camera = request->camera_;
> > +       Camera *camera = request->_d()->camera();
> >         Camera::Private *data = camera->_d();
> >         data->queuedRequests_.push_back(request);
> >
> > @@ -360,7 +361,7 @@ void PipelineHandler::queueRequest(Request *request)
> >   */
> >  bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> >  {
> > -       Camera *camera = request->camera_;
> > +       Camera *camera = request->_d()->camera();
> >         camera->bufferCompleted.emit(request, buffer);
> >         return request->completeBuffer(buffer);
> >  }
> > @@ -381,7 +382,7 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> >   */
> >  void PipelineHandler::completeRequest(Request *request)
> >  {
> > -       Camera *camera = request->camera_;
> > +       Camera *camera = request->_d()->camera();
> >
> >         request->complete();
> >
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 17fefab7ad0e..33fee1ac05ba 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -20,10 +20,11 @@
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_controls.h"
> >  #include "libcamera/internal/framebuffer.h"
> > +#include "libcamera/internal/request.h"
> >  #include "libcamera/internal/tracepoints.h"
> >
> >  /**
> > - * \file request.h
> > + * \file libcamera/request.h
> >   * \brief Describes a frame capture request to be processed by a camera
> >   */
> >
> > @@ -31,6 +32,37 @@ namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(Request)
> >
> > +/**
> > + * \class Request::Private
> > + * \brief Request private data
> > + *
> > + * The Request::Private class stores all private data associated with a
> > + * request. It implements the d-pointer design pattern to hide core
> > + * Request data from the public API, and exposes utility functions to
> > + * internal users of the request (namely the PipelineHandler class and its
> > + * subclasses).
> > + */
> > +
> > +/**
> > + * \brief Create a Request::Private
> > + * \param camera The Camera that creates the request
> > + */
> > +Request::Private::Private(Camera *camera)
> > +       : camera_(camera), cancelled_(false)
> > +{
> > +}
> > +
> > +Request::Private::~Private()
> > +{
> > +}
> > +
> > +/**
> > + * \fn Request::Private::camera()
> > + * \brief Retrieve the camera this request has been queued to
> > + * \return The Camera this request has been queued to, or nullptr if the
> > + * request hasn't been queued
> > + */
> > +
> >  /**
> >   * \enum Request::Status
> >   * Request completion status
> > @@ -75,8 +107,8 @@ LOG_DEFINE_CATEGORY(Request)
> >   * completely opaque to libcamera.
> >   */
> >  Request::Request(Camera *camera, uint64_t cookie)
> > -       : camera_(camera), sequence_(0), cookie_(cookie),
> > -         status_(RequestPending), cancelled_(false)
> > +       : Extensible(std::make_unique<Private>(camera)), sequence_(0),
> > +         cookie_(cookie), status_(RequestPending)
> >  {
> >         controls_ = new ControlList(controls::controls,
> >                                     camera->_d()->validator());
> > @@ -126,7 +158,7 @@ void Request::reuse(ReuseFlag flags)
> >
> >         sequence_ = 0;
> >         status_ = RequestPending;
> > -       cancelled_ = false;
> > +       _d()->cancelled_ = false;
> >
> >         controls_->clear();
> >         metadata_->clear();
> > @@ -282,7 +314,7 @@ void Request::complete()
> >         ASSERT(status_ == RequestPending);
> >         ASSERT(!hasPendingBuffers());
> >
> > -       status_ = cancelled_ ? RequestCancelled : RequestComplete;
> > +       status_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;
> >
> >         LOG(Request, Debug) << toString();
> >
> > @@ -299,17 +331,19 @@ void Request::complete()
> >   */
> >  void Request::cancel()
> >  {
> > +       Private *const d = _d();
> > +
> >         LIBCAMERA_TRACEPOINT(request_cancel, this);
> >
> >         ASSERT(status_ == RequestPending);
> >
> >         for (FrameBuffer *buffer : pending_) {
> >                 buffer->cancel();
> > -               camera_->bufferCompleted.emit(this, buffer);
> > +               d->camera_->bufferCompleted.emit(this, buffer);
> >         }
> >
> >         pending_.clear();
> > -       cancelled_ = true;
> > +       d->cancelled_ = true;
> >  }
> >
> >  /**
> > @@ -335,7 +369,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)
> >         buffer->_d()->setRequest(nullptr);
> >
> >         if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > -               cancelled_ = true;
> > +               _d()->cancelled_ = true;
> >
> >         return !hasPendingBuffers();
> >  }
> > --
> > 2.33.1
> >
Laurent Pinchart Nov. 12, 2021, 12:37 a.m. UTC | #3 
On Thu, Nov 11, 2021 at 09:30:08AM +0100, Jacopo Mondi wrote:
> On Tue, Nov 09, 2021 at 12:08:50PM +0000, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2021-10-28 12:15:11)
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > Implement the D-Pointer design pattern in the Request class to allow
> > > changing internal data without affecting the public ABI.
> > >
> > > Move a few internal fields that are not needed to implement the public
> > > API to the Request::Private class already. More fields may be moved
> > > later.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Happy to get this patch merged earlier if it helps too, as it unblocks
> > other patches.
> 
> FTR I get this warning from checkstyle on this patch.
> Warning that I'm afraid I don't understand
> 
> --- src/libcamera/request.cpp
> +++ src/libcamera/request.cpp
> @@ -15,12 +15,12 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/framebuffer.h>
> +#include <libcamera/request.h>
>  #include <libcamera/stream.h>
> 
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
>  #include "libcamera/internal/framebuffer.h"
> -#include "libcamera/internal/request.h"
>  #include "libcamera/internal/tracepoints.h"
> 
>  /**

That's because clang-format moved the libcamera/internal/request.h
header to the top (to follow our self-containted compilation check
through top inclusion rule), and moved the licamera/request.h header
that was at the top back in the list of libcamera public headers. Given
that libcamera/internal/request.h includes libcamera/request.h, I think
this is better. Feel free to include this diff in the next version.

> > > ---
> > >  include/libcamera/internal/meson.build |  1 +
> > >  include/libcamera/internal/request.h   | 34 ++++++++++++++++++
> > >  include/libcamera/request.h            |  6 ++--
> > >  src/libcamera/pipeline_handler.cpp     |  7 ++--
> > >  src/libcamera/request.cpp              | 50 +++++++++++++++++++++-----
> > >  5 files changed, 84 insertions(+), 14 deletions(-)
> > >  create mode 100644 include/libcamera/internal/request.h
> > >
> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > index 665fd6de4ed3..cae65b0604ff 100644
> > > --- a/include/libcamera/internal/meson.build
> > > +++ b/include/libcamera/internal/meson.build
> > > @@ -34,6 +34,7 @@ libcamera_internal_headers = files([
> > >      'pipeline_handler.h',
> > >      'process.h',
> > >      'pub_key.h',
> > > +    'request.h',
> > >      'source_paths.h',
> > >      'sysfs.h',
> > >      'v4l2_device.h',
> > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> > > new file mode 100644
> > > index 000000000000..df0cc014067e
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/request.h
> > > @@ -0,0 +1,34 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * request.h - Request class private data
> > > + */
> > > +#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
> > > +#define __LIBCAMERA_INTERNAL_REQUEST_H__
> > > +
> > > +#include <libcamera/request.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class Camera;
> > > +class FrameBuffer;
> > > +
> > > +class Request::Private : public Extensible::Private
> > > +{
> > > +       LIBCAMERA_DECLARE_PUBLIC(Request)
> > > +
> > > +public:
> > > +       Private(Camera *camera);
> > > +       ~Private();
> > > +
> > > +       Camera *camera() const { return camera_; }
> > > +
> > > +private:
> > > +       Camera *camera_;
> > > +       bool cancelled_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index d16904e6b679..d6bd0ba2ac17 100644
> > > --- a/include/libcamera/request.h
> > > +++ b/include/libcamera/request.h
> > > @@ -25,8 +25,10 @@ class CameraControlValidator;
> > >  class FrameBuffer;
> > >  class Stream;
> > >
> > > -class Request
> > > +class Request : public Extensible
> > >  {
> > > +       LIBCAMERA_DECLARE_PRIVATE()
> > > +
> > >  public:
> > >         enum Status {
> > >                 RequestPending,
> > > @@ -70,7 +72,6 @@ private:
> > >
> > >         bool completeBuffer(FrameBuffer *buffer);
> > >
> > > -       Camera *camera_;
> > >         ControlList *controls_;
> > >         ControlList *metadata_;
> > >         BufferMap bufferMap_;
> > > @@ -79,7 +80,6 @@ private:
> > >         uint32_t sequence_;
> > >         const uint64_t cookie_;
> > >         Status status_;
> > > -       bool cancelled_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index f69c4f03b80f..cada864687ff 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -19,6 +19,7 @@
> > >  #include "libcamera/internal/camera.h"
> > >  #include "libcamera/internal/device_enumerator.h"
> > >  #include "libcamera/internal/media_device.h"
> > > +#include "libcamera/internal/request.h"
> > >  #include "libcamera/internal/tracepoints.h"
> > >
> > >  /**
> > > @@ -311,7 +312,7 @@ void PipelineHandler::queueRequest(Request *request)
> > >  {
> > >         LIBCAMERA_TRACEPOINT(request_queue, request);
> > >
> > > -       Camera *camera = request->camera_;
> > > +       Camera *camera = request->_d()->camera();
> > >         Camera::Private *data = camera->_d();
> > >         data->queuedRequests_.push_back(request);
> > >
> > > @@ -360,7 +361,7 @@ void PipelineHandler::queueRequest(Request *request)
> > >   */
> > >  bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> > >  {
> > > -       Camera *camera = request->camera_;
> > > +       Camera *camera = request->_d()->camera();
> > >         camera->bufferCompleted.emit(request, buffer);
> > >         return request->completeBuffer(buffer);
> > >  }
> > > @@ -381,7 +382,7 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> > >   */
> > >  void PipelineHandler::completeRequest(Request *request)
> > >  {
> > > -       Camera *camera = request->camera_;
> > > +       Camera *camera = request->_d()->camera();
> > >
> > >         request->complete();
> > >
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > index 17fefab7ad0e..33fee1ac05ba 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -20,10 +20,11 @@
> > >  #include "libcamera/internal/camera.h"
> > >  #include "libcamera/internal/camera_controls.h"
> > >  #include "libcamera/internal/framebuffer.h"
> > > +#include "libcamera/internal/request.h"
> > >  #include "libcamera/internal/tracepoints.h"
> > >
> > >  /**
> > > - * \file request.h
> > > + * \file libcamera/request.h
> > >   * \brief Describes a frame capture request to be processed by a camera
> > >   */
> > >
> > > @@ -31,6 +32,37 @@ namespace libcamera {
> > >
> > >  LOG_DEFINE_CATEGORY(Request)
> > >
> > > +/**
> > > + * \class Request::Private
> > > + * \brief Request private data
> > > + *
> > > + * The Request::Private class stores all private data associated with a
> > > + * request. It implements the d-pointer design pattern to hide core
> > > + * Request data from the public API, and exposes utility functions to
> > > + * internal users of the request (namely the PipelineHandler class and its
> > > + * subclasses).
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a Request::Private
> > > + * \param camera The Camera that creates the request
> > > + */
> > > +Request::Private::Private(Camera *camera)
> > > +       : camera_(camera), cancelled_(false)
> > > +{
> > > +}
> > > +
> > > +Request::Private::~Private()
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \fn Request::Private::camera()
> > > + * \brief Retrieve the camera this request has been queued to
> > > + * \return The Camera this request has been queued to, or nullptr if the
> > > + * request hasn't been queued
> > > + */
> > > +
> > >  /**
> > >   * \enum Request::Status
> > >   * Request completion status
> > > @@ -75,8 +107,8 @@ LOG_DEFINE_CATEGORY(Request)
> > >   * completely opaque to libcamera.
> > >   */
> > >  Request::Request(Camera *camera, uint64_t cookie)
> > > -       : camera_(camera), sequence_(0), cookie_(cookie),
> > > -         status_(RequestPending), cancelled_(false)
> > > +       : Extensible(std::make_unique<Private>(camera)), sequence_(0),
> > > +         cookie_(cookie), status_(RequestPending)
> > >  {
> > >         controls_ = new ControlList(controls::controls,
> > >                                     camera->_d()->validator());
> > > @@ -126,7 +158,7 @@ void Request::reuse(ReuseFlag flags)
> > >
> > >         sequence_ = 0;
> > >         status_ = RequestPending;
> > > -       cancelled_ = false;
> > > +       _d()->cancelled_ = false;
> > >
> > >         controls_->clear();
> > >         metadata_->clear();
> > > @@ -282,7 +314,7 @@ void Request::complete()
> > >         ASSERT(status_ == RequestPending);
> > >         ASSERT(!hasPendingBuffers());
> > >
> > > -       status_ = cancelled_ ? RequestCancelled : RequestComplete;
> > > +       status_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;
> > >
> > >         LOG(Request, Debug) << toString();
> > >
> > > @@ -299,17 +331,19 @@ void Request::complete()
> > >   */
> > >  void Request::cancel()
> > >  {
> > > +       Private *const d = _d();
> > > +
> > >         LIBCAMERA_TRACEPOINT(request_cancel, this);
> > >
> > >         ASSERT(status_ == RequestPending);
> > >
> > >         for (FrameBuffer *buffer : pending_) {
> > >                 buffer->cancel();
> > > -               camera_->bufferCompleted.emit(this, buffer);
> > > +               d->camera_->bufferCompleted.emit(this, buffer);
> > >         }
> > >
> > >         pending_.clear();
> > > -       cancelled_ = true;
> > > +       d->cancelled_ = true;
> > >  }
> > >
> > >  /**
> > > @@ -335,7 +369,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)
> > >         buffer->_d()->setRequest(nullptr);
> > >
> > >         if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > > -               cancelled_ = true;
> > > +               _d()->cancelled_ = true;
> > >
> > >         return !hasPendingBuffers();
> > >  }