[libcamera-devel] [PATCH v2 00/12] [WIP] libcamera: Add support for Fence

[Jacopo Mondi]

Marked as WIP as I've not completed the documentation yet, as I wish to
validate the API first. Also, I'm sure I've missed pieces here and there but I
want this out to discuss it before getting it into a mergeable state.

Reworked the API as suggested and discussed in v1.

v1->v2:

Major changes:
- removed notifiers and timers from Fence. A Fence is now an wrapper that owns
  a file descriptor
- Add timers and notifiers to Request::Private and add the
  Request::Private::prepare interface

Smaller changes:
- Expand Request::Private to move all internal fields
- Remove Fence move semantic. A Fence lives in a Framebuffer.
- A few minor patches on top

Fence application API:

1 unique_ptr<Fence> fence = make_unique<Fence>(FileDescriptor &fd);
2 request->addBuffer(..., move(fence))
3	frameBuffer.set(fence)
4 camera->queueRequest(request)
5	pipelineHandler.queueRequest()
6 		request.prepare()
7		request.prepared.emit()
8		pipelineHandler.doQueueRequest()
9			if (request.privateState.Ready())
10				doQueueRequestDevice(request)
11			else
12				request->cancel()
13 requestCompleted()
14	if buffer.fence()
15		FileDescriptor fd = std::move(buffer.fence.fd())

What I don't like

- It's a clunky API

	FileDescriptor fenceFd = FileDescriptor(std::move(buffer.fence));
	std::unique_ptr<Fence> fence = std::make_unique<Fence>(fenceFd);
	descriptor->request_->addBuffer(cameraStream->stream(),
					frameBuffer, std::move(fence));

- Fence creation with a FileDescriptor & does not enforce that the
  FileDescriptor should not have duplicated the integer file descriptor

- Fence constructor takes a FileDescriptor &, but moves the FileDescriptor
  internally not to duplicate the fd. The FileDescriptor passed as reference is
  invalidated but nothing in the API suggests so

- Request::addBuffer() moves the Fence to the Framebuffer not to the request
  even if the function prototype suggest differently

- The API to extract the FileDescritptor from a completed request requires
  the user to now that it has to move the FileDescritpro out, nothing suggest
  that

- Request now has Request::Private::Status and Request::Status

- Fence are an empty wrapper around a FileDescriptor. That's ok-ish as it
  allows to support more synchronization methods in future without changing the
  API towards the Fence class users

- Timeouts are per-request not per fence. If we need to have it per-fence the
  core infrastructure need to be changed

Buildbot https://buildbot.libcamera.org/#/builders/1/builds/54

CTS:

=======================================================
=============== Summary ===============
Total Run time: 19m 58s
1/1 modules completed
Total Tests       : 231
PASSED            : 230
FAILED            : 1
============== End of Results ==============
============================================

Thanks
   j

Jacopo Mondi (11):
  libcamera: fence: Introduce Fence
  libcamera: framebuffer: private: Add Fence
  libcamera: request: Add Fence to Request::addBuffer()
  test: fence: Add test for the Fence class
  libcamera: pipeline_handler: Split request queueing
  libcamera: pipeline: Introduce stopDevice()
  libcamera: request: Add Request::Private::prepare()
  libcamera: pipeline_handler: Prepare Request
  android: Remove CameraWorker
  test: camera: Fix trivial spelling mistaken
  libcamera: Add tracing to meson summary

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

 include/libcamera/fence.h                     |  36 ++
 include/libcamera/framebuffer.h               |   3 +
 include/libcamera/internal/fence.h            |  37 ++
 include/libcamera/internal/framebuffer.h      |   9 +
 include/libcamera/internal/meson.build        |   2 +
 include/libcamera/internal/pipeline_handler.h |   9 +-
 include/libcamera/internal/request.h          |  72 +++
 .../libcamera/internal/tracepoints/request.tp |  18 +-
 include/libcamera/meson.build                 |   1 +
 include/libcamera/request.h                   |  22 +-
 meson.build                                   |   1 +
 src/android/camera_device.cpp                 |  43 +-
 src/android/camera_device.h                   |   3 -
 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/fence.cpp                       | 104 ++++
 src/libcamera/framebuffer.cpp                 |  26 +
 src/libcamera/meson.build                     |   4 +
 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            |  97 +++-
 src/libcamera/request.cpp                     | 462 ++++++++++++++----
 test/camera/capture.cpp                       |   2 +-
 test/fence.cpp                                | 339 +++++++++++++
 test/meson.build                              |   1 +
 32 files changed, 1160 insertions(+), 363 deletions(-)
 create mode 100644 include/libcamera/fence.h
 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