[libcamera-devel] [PATCH] libcamera: pipeline: stm32: add pipeline handler for stm32
Benjamin GAIGNARD
benjamin.gaignard at st.com
Fri Apr 19 09:45:45 CEST 2019
On 4/18/19 6:01 PM, Laurent Pinchart wrote:
> Hi Hugues,
>
> On Thu, Apr 18, 2019 at 02:22:07PM +0000, Hugues FRUCHET wrote:
>> On 4/17/19 2:11 PM, Laurent Pinchart wrote:
>>> On Tue, Apr 16, 2019 at 08:39:25AM +0000, Benjamin GAIGNARD wrote:
>>>> On 4/16/19 1:14 AM, Laurent Pinchart wrote:
>>>>> On Tue, Apr 09, 2019 at 09:38:48AM +0000, Benjamin GAIGNARD wrote:
>>>>>> On 4/9/19 11:02 AM, Kieran Bingham wrote:
>>>>>>> On 08/04/2019 13:43, Benjamin Gaignard wrote:
>>>>>>>> Provide a pipeline handler for the stm32 driver.
>>>>>>> Excellent :-) Thank you for your contribution.
>>>>>>>
>>>>>>> Have you been able to test this and successfully list your camera or
>>>>>>> capture frames?
>>>>>> Yes I have been able to list and capture frames with cam
>>>>>>
>>>>>>> To list successfully registered cameras on the device:
>>>>>>> build/src/cam/cam -l
>>>>>>>
>>>>>>> I couldn't see any media-controller support in the stm32-dcmi driver...
>>>>>>> Perhaps I'm missing something.
>>>>>> Hugues has send the patches for media controller support a week ago.
>>>>>>
>>>>>> https://lkml.org/lkml/2019/4/1/298
>>>>>>
>>>>>> I have tested libcamera with those patches.
>>>>> I think you may have missed the other comments from Kieran further down.
>>>> Yes I have completely miss them
>>>>
>>>>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at st.com>
>>>>>>>> ---
>>>>>>>> src/libcamera/pipeline/meson.build | 2 +
>>>>>>>> src/libcamera/pipeline/stm32/meson.build | 3 +
>>>>>>>> src/libcamera/pipeline/stm32/stm32.cpp | 205 +++++++++++++++++++++++++++++++
>>>>>>>> 3 files changed, 210 insertions(+)
>>>>>>>> create mode 100644 src/libcamera/pipeline/stm32/meson.build
>>>>>>>> create mode 100644 src/libcamera/pipeline/stm32/stm32.cpp
>>>>>>>>
>>>>>>>> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
>>>>>>>> index 40bb264..08d6e1c 100644
>>>>>>>> --- a/src/libcamera/pipeline/meson.build
>>>>>>>> +++ b/src/libcamera/pipeline/meson.build
>>>>>>>> @@ -4,3 +4,5 @@ libcamera_sources += files([
>>>>>>>> ])
>>>>>>>>
>>>>>>>> subdir('ipu3')
>>>>>>>> +
>>>>>>>> +subdir('stm32')
>>>>>>>> diff --git a/src/libcamera/pipeline/stm32/meson.build b/src/libcamera/pipeline/stm32/meson.build
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..cb6f16b
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/src/libcamera/pipeline/stm32/meson.build
>>>>>>>> @@ -0,0 +1,3 @@
>>>>>>>> +libcamera_sources += files([
>>>>>>>> + 'stm32.cpp',
>>>>>>>> +])
>>>>>>> Currently the stm32.cpp seems like a fairly simple implementation, which
>>>>>>> fits in its own file, much like pipeline/uvcvideo.cpp.
>>>>>>>
>>>>>>> Do you forsee needing to break the STM32 handler into multiple files
>>>>>>> later, or support other components on the STM32?
>>>>>>>
>>>>>>> I think it's fine if you want to have stm32 as a subdir either way though.
>>>>>>>
>>>>>>> I wonder also, if it is just a 'simple' device if perhaps we should move
>>>>>>> the bulk of the uvcvideo handler to a 'simple.cpp' and allow it to be
>>>>>>> subclassed to simply add the device matching, or provide a table of
>>>>>>> supported match strings.
>>>>>>>
>>>>>>>
>>>>>>> What hardware is available on the STM32 for processing frames?
>>>>>>> Do you have a scaler/resizer? or any further ISP functionality?
>>>> No scaler, no resize and no ISP functionality.
>>>>
>>>> It is a basic device to memory driver.
>>> In that case I wonder if we should really have an stm32-specific
>>> pipeline handler. It seems to me that we could implement a generic
>>> pipeline handler that would support any graph made of a sensor subdev
>>> connected to a video node.
>> We can also have a bridge in between, so graph becomes:
>>
>> root at stm32mp1:~# media-ctl -d /dev/media0 -p
>> Media controller API version 5.0.0
>>
>> Media device information
>> ------------------------
>> driver stm32-dcmi
>> model stm32-dcmi
>> serial
>> bus info platform:stm32-dcmi
>> hw revision 0x0
>> driver version 5.0.0
>>
>> Device topology
>> - entity 1: stm32_dcmi (1 pad, 1 link)
>> type Node subtype V4L flags 1
>> device node name /dev/video0
>> pad0: Sink
>> <- "mipid02 0-0014":1 [ENABLED,IMMUTABLE]
>>
>> - entity 5: mipid02 0-0014 (2 pads, 2 links)
>> type V4L2 subdev subtype Unknown flags 0
>> device node name /dev/v4l-subdev0
>> pad0: Sink
>> <- "ov5640 0-003c":0 [ENABLED,IMMUTABLE]
>> pad1: Source
>> -> "stm32_dcmi":0 [ENABLED,IMMUTABLE]
> This is a chip external to the SoC, right ?
Yes it is.
>
>> - entity 10: ov5640 0-003c (1 pad, 1 link)
>> type V4L2 subdev subtype Sensor flags 0
>> device node name /dev/v4l-subdev1
>> pad0: Source
>> [fmt:JPEG_1X8/320x240 field:none]
>> -> "mipid02 0-0014":0 [ENABLED,IMMUTABLE]
>>
>> We have discussed this on irc here:
>> https://linuxtv.org/irc/irclogger_log/v4l?date=2019-04-02,Tue
>>
>> And in this case, if I have well understood, user has to use media-ctl
>> to set format and resolution on bridge and sensor, just set the
>> resolution and format on video node only will not be sufficient.
> Userspace has to configure the pipeline. The media-ctl tool is one way
> to do so, but not the only one. libcamera aims at solving this issue,
> and calls the media controller API directly from pipeline handlers
> completely transparently for the application.
>
>> Moreover, after having configured format and resolution using media-ctl,
>> the same format and resolution must be asked to GStreamer otherwise a
>> mismatch will occur:
>>
>> 1) media-ctl -d /dev/media0 --set-v4l2 "'ov5640
>> 0-003c':0[fmt:JPEG_1X8/640x480 field:none]"
>> 2) gst-launch-1.0 v4l2src ! image/jpeg, width=320, height=240 !
>> decodebin ! fpsdisplaysink -v
>>
>> /GstPipeline:pipeline0/GstCapsFilter:capsfilter0.GstPad:src: caps =
>> image/jpeg, width=(int)320, height=(int)240,
>> pixel-aspect-ratio=(fraction)1/1
>> => OK, QVGA JPEG is captured
>>
>> but if I just omit the caps right after v4l2src (we don't force any
>> format/resolution), this is broken:
>> 1) media-ctl -d /dev/media0 --set-v4l2 "'ov5640
>> 0-003c':0[fmt:JPEG_1X8/640x480 field:none]"
>> 2) gst-launch-1.0 v4l2src ! decodebin ! fakesink silent=false -v
>>
>> /GstPipeline:pipeline0/GstV4l2Src:v4l2src0.GstPad:src: caps =
>> video/x-raw, format=(string)YUY2, width=(int)2592, height=(int)1944,
>> pixel-aspect-ratio=(fe
>> => KO, we try to capture 5Mp YUYV while ov5640 is configured to send
>> QVGA JPEG...
>>
>> In summary if I have well udnerstood, user must ensure right match of
>> both media-ctl format/resolution and v4l2 S_FMT format/resolution.
> Userspace does, but not the end-user :-)
>
>> So there are 2 cases to handle at least, the simple case without bridge
>> and the case with bridge where MC is involved.
>>
>> I still feel not cumfortable with the fact that just introducing a
>> bridge in the middle (which is just a matter of physical link, no change
>> on functionalities) has impacts up to user side, what do you think ?
> The simplest case of a sensor connected to a camera receiver followed by
> a DMA engine has traditionally been supported in V4L2 by a single video
> device node. At the other extreme, very complex pipelines require usage
> of the media controller API, with possibly complex logic in userspace to
> connect all the pieces together. Then we have all the cases in-between,
> with various degrees of complexity, and we need to draw the line
> somewhere. Now that libcamera is becoming a reality, I think it's best
> for kernelspace to be as simple as possible, and let userspace handle
> the pipeline configuration in as many cases as possible. This will be
> transparent for the end-user, both with native libcamera applications,
> and for gstreamer-based applications when we'll have a gstreamer element
> for libcamera (development hasn't started yet, but that's something we
> want to have). Note that we also plan to offer a V4L2 adaptation layer
> that will allow unmodified pure V4L2 applications to use a camera
> handled by libcamera, and in theory gstreamer could use that, but it
> will be limited to best offert as not all features can be exposed that
> way. A native gstreamer libcamera element will likely be better.
Since the pipeline have to manage all the complexity I will
keep a dedicated file for stm32 pipeline. Sound good for you ?
Benjamin
>
>>>>>>> It would be interesting to see the output of 'media-ctl -p' for your
>>>>>>> platform.
>>>> The ouput of media-ctl -p (with Hugues's patches)
>>>>
>>>> Media controller API version 4.19.24
>>>>
>>>> Media device information
>>>> ------------------------
>>>> driver stm32-dcmi
>>>> model stm32-dcmi
>>>> serial
>>>> bus info platform:stm32-dcmi
>>>> hw revision 0x0
>>>> driver version 4.19.24
>>>>
>>>> Device topology
>>>> - entity 1: stm32_dcmi (1 pad, 1 link)
>>>> type Node subtype V4L flags 1
>>>> device node name /dev/video0
>>>> pad0: Sink
>>>> <- "ov5640 0-003c":0 [ENABLED,IMMUTABLE]
>>>>
>>>> - entity 5: ov5640 0-003c (1 pad, 1 link)
>>>> type V4L2 subdev subtype Sensor flags 0
>>>> device node name /dev/v4l-subdev0
>>>> pad0: Source
>>>> [fmt:JPEG_1X8/320x240 at 1/30 field:none colorspace:jpeg
>>>> xfer:srgb ycbcr:601 quantization:full-range]
>>>> -> "stm32_dcmi":0 [ENABLED,IMMUTABLE]
>>>>
>>>>>>>> diff --git a/src/libcamera/pipeline/stm32/stm32.cpp b/src/libcamera/pipeline/stm32/stm32.cpp
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..301fdfc
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/src/libcamera/pipeline/stm32/stm32.cpp
>>>>>>>> @@ -0,0 +1,205 @@
>>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>>>>> +/*
>>>>>>>> + * stm32.cpp - Pipeline handler for stm32 devices
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include <libcamera/camera.h>
>>>>>>>> +#include <libcamera/request.h>
>>>>>>>> +#include <libcamera/stream.h>
>>>>>>>> +
>>>>>>>> +#include "device_enumerator.h"
>>>>>>>> +#include "log.h"
>>>>>>>> +#include "media_device.h"
>>>>>>>> +#include "pipeline_handler.h"
>>>>>>>> +#include "utils.h"
>>>>>>>> +#include "v4l2_device.h"
>>>>>>>> +
>>>>>>>> +namespace libcamera {
>>>>>>>> +
>>>>>>>> +LOG_DEFINE_CATEGORY(STM32)
>>>>>>>> +
>>>>>>>> +class PipelineHandlerSTM32 : public PipelineHandler {
>>>>>>>> +public:
>>>>>>>> + PipelineHandlerSTM32(CameraManager *manager);
>>>>>>>> + ~PipelineHandlerSTM32();
>>>>>>> Our coding style uses a tab to indent.
>>>>>>>
>>>>>>> When you have committed your code, you can run ./utils/checkstyle.py to
>>>>>>> check the most recent commit (or you can specify a range suitable for git).
>>>>>>>
>>>>>>> This requires installing clang-format ideally, although astyle is also
>>>>>>> supported to some degree.
>>>> I have fix clang-format package version on my side, it will be better in v2.
>>> Thank you.
>>>
>>>>>>>> +
>>>>>>>> + std::map<Stream *, StreamConfiguration>
>>>>>>>> + streamConfiguration(Camera *camera, std::set<Stream *> &streams) override;
>>>>>>>> + int configureStreams(
>>>>>>>> + Camera *camera, std::map<Stream *, StreamConfiguration> &config) override;
>>>>>>>> +
>>>>>>>> + int allocateBuffers(Camera *camera, Stream *stream) override;
>>>>>>>> + int freeBuffers(Camera *camera, Stream *stream) override;
>>>>>>>> +
>>>>>>>> + int start(Camera *camera) override;
>>>>>>>> + void stop(Camera *camera) override;
>>>>>>>> +
>>>>>>>> + int queueRequest(Camera *camera, Request *request) override;
>>>>>>>> +
>>>>>>>> + bool match(DeviceEnumerator *enumerator);
>>>>>>>> +
>>>>>>>> +private:
>>>>>>>> + class STM32CameraData : public CameraData {
>>>>>>>> + public:
>>>>>>>> + STM32CameraData(PipelineHandler *pipe)
>>>>>>>> + : CameraData(pipe), video_(nullptr) {}
>>>>>>>> +
>>>>>>>> + ~STM32CameraData() { delete video_; }
>>>>>>>> +
>>>>>>>> + void bufferReady(Buffer *buffer);
>>>>>>>> +
>>>>>>>> + V4L2Device *video_;
>>>>>>>> + Stream stream_;
>>>>>>>> + };
>>>>>>>> +
>>>>>>>> + STM32CameraData *cameraData(const Camera *camera) {
>>>>>>>> + return static_cast<STM32CameraData *>(PipelineHandler::cameraData(camera));
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + std::shared_ptr<MediaDevice> media_;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +PipelineHandlerSTM32::PipelineHandlerSTM32(CameraManager *manager)
>>>>>>>> + : PipelineHandler(manager), media_(nullptr) {}
>>>>>>>> +
>>>>>>>> +PipelineHandlerSTM32::~PipelineHandlerSTM32() {
>>>>>>>> + if (media_)
>>>>>>>> + media_->release();
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +std::map<Stream *, StreamConfiguration>
>>>>>>>> +PipelineHandlerSTM32::streamConfiguration(Camera *camera,
>>>>>>>> + std::set<Stream *> &streams) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> +
>>>>>>>> + std::map<Stream *, StreamConfiguration> configs;
>>>>>>>> + StreamConfiguration config{};
>>>>>>>> +
>>>>>>>> + LOG(STM32, Debug) << "Retrieving default format";
>>>>>>>> + config.width = 640;
>>>>>>>> + config.height = 480;
>>>>>>>> + config.pixelFormat = V4L2_PIX_FMT_YUYV;
>>>>>>>> + config.bufferCount = 4;
>>>>>>>> +
>>>>>>>> + configs[&data->stream_] = config;
>>>>>>>> +
>>>>>>>> + return configs;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int PipelineHandlerSTM32::configureStreams(
>>>>>>>> + Camera *camera, std::map<Stream *, StreamConfiguration> &config) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> + StreamConfiguration *cfg = &config[&data->stream_];
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + LOG(STM32, Debug) << "Configure the camera for resolution " << cfg->width
>>>>>>>> + << "x" << cfg->height;
>>>>>>>> +
>>>>>>>> + V4L2DeviceFormat format = {};
>>>>>>>> + format.width = cfg->width;
>>>>>>>> + format.height = cfg->height;
>>>>>>>> + format.fourcc = cfg->pixelFormat;
>>>>>>>> +
>>>>>>>> + ret = data->video_->setFormat(&format);
>>>>>>>> + if (ret)
>>>>>>>> + return ret;
>>>>>>>> +
>>>>>>>> + if (format.width != cfg->width || format.height != cfg->height ||
>>>>>>>> + format.fourcc != cfg->pixelFormat)
>>>>>>>> + return -EINVAL;
>>>>>>>> +
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int PipelineHandlerSTM32::allocateBuffers(Camera *camera, Stream *stream) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> + const StreamConfiguration &cfg = stream->configuration();
>>>>>>>> +
>>>>>>>> + LOG(STM32, Debug) << "Requesting " << cfg.bufferCount << " buffers";
>>>>>>>> +
>>>>>>>> + return data->video_->exportBuffers(&stream->bufferPool());
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int PipelineHandlerSTM32::freeBuffers(Camera *camera, Stream *stream) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> + return data->video_->releaseBuffers();
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int PipelineHandlerSTM32::start(Camera *camera) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> + return data->video_->streamOn();
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void PipelineHandlerSTM32::stop(Camera *camera) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> + data->video_->streamOff();
>>>>>>>> + PipelineHandler::stop(camera);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int PipelineHandlerSTM32::queueRequest(Camera *camera, Request *request) {
>>>>>>>> + STM32CameraData *data = cameraData(camera);
>>>>>>>> + Buffer *buffer = request->findBuffer(&data->stream_);
>>>>>>>> + if (!buffer) {
>>>>>>>> + LOG(STM32, Error) << "Attempt to queue request with invalid stream";
>>>>>>>> +
>>>>>>>> + return -ENOENT;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + int ret = data->video_->queueBuffer(buffer);
>>>>>>>> + if (ret < 0)
>>>>>>>> + return ret;
>>>>>>>> +
>>>>>>>> + PipelineHandler::queueRequest(camera, request);
>>>>>>>> +
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +bool PipelineHandlerSTM32::match(DeviceEnumerator *enumerator) {
>>>>>>>> + DeviceMatch dm("stm32-dcmi");
>>>>>>> I see that the stm32-dcmi driver calls down to a subdevice. Is there
>>>>>>> ever a need to interact with this subdevice directly?
>>>>>>>
>>>>>>> Or are all controls handled through the DCMI driver?
>>>>>>>
>>>>>>> I think it looks like everything simply goes through the stm32-dcmi
>>>>>>> V4L2Device interface.
>>>> Until today yes but Hugues is working with v4l for media-controller
>>>> awareness.
>>>>
>>>>>>> That said, I can't see if there is any media-controller device in the
>>>>>>> DCMI driver. Our DeviceMatch currently only looks at media-controller
>>>>>>> enabled devices.
>>>>>>>
>>>>>>>> +
>>>>>>>> + media_ = enumerator->search(dm);
>>>>>>>> + if (!media_)
>>>>>>>> + return false;
>>>>>>>> +
>>>>>>>> + media_->acquire();
>>>>>>>> +
>>>>>>>> + std::unique_ptr<STM32CameraData> data =
>>>>>>>> + utils::make_unique<STM32CameraData>(this);
>>>>>>>> +
>>>>>>>> + /* Locate and open the default video node. */
>>>>>>>> + for (MediaEntity *entity : media_->entities()) {
>>>>>>>> + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
>>>>>>>> + data->video_ = new V4L2Device(entity);
>>>>>>>> + break;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>> I think the MEDIA_ENT_FL_DEFAULT is currently quite specific to the
>>>>>>> UVCVideo (and omap3isp) pipelines, so I'm not sure if this will
>>>>>>> correctly match your device?
>>>> It will because it is in Hugues's patches.
>>>>
>>>> Does an another way of working is posssible ?
>>>>
>>>> Maybe we have misunderstood the mean of MEDIA_ENT_FL_DEFAULT and how to
>>>> use it.
>>> There's a single video node in your media graph, so there's no real need
>>> to use MEDIA_ENT_FL_DEFAULT, you can simply pick the only video device,
>>> can't you ?
>>>
>>>> Do you have some advices ?
>>>>
>>>>>>>> +
>>>>>>>> + if (!data->video_) {
>>>>>>>> + LOG(STM32, Error) << "Could not find a default video device";
>>>>>>>> + return false;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + if (data->video_->open())
>>>>>>>> + return false;
>>>>>>>> +
>>>>>>>> + data->video_->bufferReady.connect(data.get(), &STM32CameraData::bufferReady);
>>>>>>>> +
>>>>>>>> + /* Create and register the camera. */
>>>>>>>> + std::set<Stream *> streams{&data->stream_};
>>>>>>>> + std::shared_ptr<Camera> camera =
>>>>>>>> + Camera::create(this, media_->model(), streams);
>>>>>>>> + registerCamera(std::move(camera), std::move(data));
>>>>>>>> +
>>>>>>>> + return true;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void PipelineHandlerSTM32::STM32CameraData::bufferReady(Buffer *buffer) {
>>>>>>>> + Request *request = queuedRequests_.front();
>>>>>>>> +
>>>>>>>> + pipe_->completeBuffer(camera_, request, buffer);
>>>>>>>> + pipe_->completeRequest(camera_, request);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +REGISTER_PIPELINE_HANDLER(PipelineHandlerSTM32);
>>>>>>>> +
>>>>>>>> +} /* namespace libcamera */
>>>>>>>>
>>>>>>> Thank you for joining the libcamera project! - If there is anything more
>>>>>>> we can do to help you - please let us know.
More information about the libcamera-devel
mailing list