[libcamera-devel] [PATCH] libcamera: pipeline: simple: enable mplane devices using contiguous memory

[Laurent Pinchart]

Hi Andrey,

On Fri, Oct 09, 2020 at 12:48:14AM +0300, Andrey Konovalov wrote:
> Hi,
> 
> As suggested by Laurent during the discussion on #libcamera irc channel,
> I considered filtering out pixel formats not supported in libcamera in
> SimpleCameraData::init().
> 
> The thing is that in SimpleCameraData::init() we only have the information
> from V4L2VideoDevice::formats() (enum_fmt ioctl) which doesn't include
> the number of memory planes for a given format.
> While in SimplePipelineHandler::configure() we have the data returned by
> s_fmt ioctl, so the number of memory planes for the format set is known,
> and I could easily use it in v1 of the patch.
> 
> As libcamera currently supports only single memory plane (contiguous
> memory) formats, this is enough to filter out unsupported formats
> from what V4L2VideoDevice::formats() returns (easy to implement in
> SimpleCameraData::init()). And checking the number of memory planes
> for a given format isn't necessary.
> 
> But after looking at SimpleCameraData::init(), it turned out that unsupported
> formats are filtered out by the already existing code:
> 
> -----8<-----
>                  /*
>                   * Store the configuration in the formats_ map, mapping the
>                   * PixelFormat to the corresponding configuration. Any
>                   * previously stored value is overwritten, as the pipeline
>                   * handler currently doesn't care about how a particular
>                   * PixelFormat is achieved.
>                   */
>                  for (const auto &videoFormat : videoFormats) {
>                          PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
>                          if (!pixelFormat)
>                                  continue;
> -----8<-----
> 
> V4L2PixelFormat::toPixelFormat() method checks if the V4L2 pixel format is present
> in the vpf2pf map, and if it is not there returns the invalid PixelFormat (with zero fourcc_).
> 
> And indeed with the "if (video->caps().isMultiplanar())" check removed, and the NV61 entry
> commented out from the vpf2pf map (to simulate unsupported format) I've got:
> 
> -----8<-----
> [2:29:09.524492795] [1396] DEBUG SimplePipeline simple.cpp:292 Adding configuration for 2592x1944 in pixel formats [ NV61, NV16]
> [2:29:09.524833004] [1396]  WARN V4L2 v4l2_pixelformat.cpp:180 Unsupported V4L2 pixel format NV61
> [2:29:09.527873323] [1396] DEBUG Camera camera_manager.cpp:152 Pipeline handler "SimplePipelineHandler" matched
> Using camera /base/soc/cci at 1b0c000/i2c-bus at 0/camera_rear at 3c
> [2:29:09.529992233] [1395] DEBUG Camera camera.cpp:754 streams configuration: (0) 2592x1944-NV16
> [2:29:09.530364682] [1395]  INFO Camera camera.cpp:811 configuring streams: (0) 2592x1944-NV16
> -----8<-----
> 
> So the "unsupported" NV61 format was filtered out by the existing code, and NV16 was used for the capture instead.
> 
> This leaves us with the two options for v2 of the patch:
> * v2a
>    Just drop the "if (video->caps().isMultiplanar())" guard.
>    Don't add any new checks. As unsupported formats are filtered out in SimpleCameraData::init(),
>    and all the currently supported formats will just work with the simple pipeline handler.
> * v2b
>    Only change the commit message in the v1 patch.
>    To explain that the added check is no op in the current libcamera, but
>    this "if (captureFormat.planesCount != 1)" will trigger and serve as the reminder to review
>    the simple pipeline handler code when 1) libcamera gets support for non-contiguous formats,
>    and 2) the hardware which uses such formats is enabled in the simple pipeline handler.
> 
> What would be the best option?

I'd go for the second option, a reminder is useful, but I don't have a
very strong preference.

> On 07.10.2020 21:22, Kieran Bingham wrote:
> > On 07/10/2020 18:16, Andrey Konovalov wrote:
> >> On 07.10.2020 16:04, Kieran Bingham wrote:
> >>> On 07/10/2020 14:01, Niklas Söderlund wrote:
> >>>> On 2020-10-07 10:54:57 +0300, Andrey Konovalov wrote:
> >>>>> The current simple pipeline handler refuses to work with capture
> >>>>> devices
> >>>>> which have V4L2_CAP_VIDEO_CAPTURE_MPLANE flag set in the device
> >>>>> capabilities
> >>>>> field. This is too restrictive, as devices supporting the
> >>>>> multi-planar API
> >>>>> can be using contiguous memory for semi-planar and planar formats,
> >>>>> and this
> >>>>> would just work without any changes to libcamera.
> >>>>>
> >>>>> Drop the guard against MPLANE devices, and replace it with the check of
> >>>>> the number of planes in the format the simple pipeline handler is
> >>>>> going to
> >>>>> use for capture. This will let MPLANE devices which don't use
> >>>>> non-contiguous
> >>>>> memory for frame buffers to work with the simple pipeline handler.
> >>>>
> >>>> I wonder if the check should not be moved to SimpleCameraData::init()
> >>>> where the formats_ array is built. The array contains all supported
> >>>> formats of the camera and excluding mplaner formats from it will make it
> >>>> not show up at all for applications. Also validate() would Adjust if any
> >>>> format is asked for that is not in the formats_ array.
> >>
> >> Yes, this is a better option, thanks! I'll send the v2 shortly.
> >>
> >>> That sounds pretty good too. If we go that route, I think it will need a
> >>> highlighting '\todo: support mplane formats' so that it's clear/easy to
> >>> find the code which is mysteriously removing supported formats from a
> >>> device which is capable of using them ;-) (after we really support
> >>> Multiplanar).
> >>
> >> OK, will add the \todo.
> >> BTW, the "mplane format" in this context means the one using non-contiguous
> >> memory for frame buffers. E.g. 'NV16' I've tested is allowed, while 'NM16'
> >> (aka V4L2_PIX_FMT_NV16M) would be excluded.
> > 
> > Yes indeed - Agreed ;-)
> > 
> >>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >>>>> ---
> >>>>>    src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------
> >>>>>    1 file changed, 6 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp
> >>>>> b/src/libcamera/pipeline/simple/simple.cpp
> >>>>> index 10223a9b..8dc23623 100644
> >>>>> --- a/src/libcamera/pipeline/simple/simple.cpp
> >>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >>>>> @@ -592,6 +592,12 @@ int SimplePipelineHandler::configure(Camera
> >>>>> *camera, CameraConfiguration *c)
> >>>>>        if (ret)
> >>>>>            return ret;
> >>>>> +    if (captureFormat.planesCount != 1) {
> >>>>> +        LOG(SimplePipeline, Error)
> >>>>> +            <<  "Planar formats using non-contiguous memory not supported";
> >>>>> +        return -EINVAL;
> >>>>> +    }
> >>>>> +
> >>>>>        if (captureFormat.fourcc != videoFormat ||
> >>>>>            captureFormat.size != pipeConfig.captureSize) {
> >>>>>            LOG(SimplePipeline, Error)
> >>>>> @@ -845,12 +851,6 @@ V4L2VideoDevice
> >>>>> *SimplePipelineHandler::video(const MediaEntity *entity)
> >>>>>        if (video->open() < 0)
> >>>>>            return nullptr;
> >>>>> -    if (video->caps().isMultiplanar()) {
> >>>>> -        LOG(SimplePipeline, Error)
> >>>>> -            << "V4L2 multiplanar devices are not supported";
> >>>>> -        return nullptr;
> >>>>> -    }
> >>>>> -
> >>>>>        video->bufferReady.connect(this, &SimplePipelineHandler::bufferReady);
> >>>>>          auto element = videos_.emplace(entity, std::move(video));

-- 
Regards,

Laurent Pinchart