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

[Kieran Bingham]

Hi Andrey,

On 07/10/2020 18:16, Andrey Konovalov wrote:
> Hi Niklas, Kieran,
> 
> On 07.10.2020 16:04, Kieran Bingham wrote:
>> Hi Niklas, Andrey,
>>
>> On 07/10/2020 14:01, Niklas Söderlund wrote:
>>> Hello Andrey,
>>>
>>> Thanks for your patch.
>>>
>>> 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 ;-)
--
Kieran


> 
> Thanks,
> Andrey
> 
>> -- 
>> Kieran
>>
>>
>>
>>>
>>>>
>>>> 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));
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> libcamera-devel mailing list
>>>> libcamera-devel at lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>
>>

-- 
Regards
--
Kieran