Re: [RFC PATCH v6 02/11] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more)

[Hsia-Jun Li <Randy.Li@synaptics.com>]

On 11/11/22 13:48, Tomasz Figa wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Fri, Nov 11, 2022 at 12:04 PM Hsia-Jun Li <Randy.Li@synaptics.com> wrote:
>>
>>
>>
>> On 11/11/22 01:06, Nicolas Dufresne wrote:
>>> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>
>>>
>>> Le samedi 05 novembre 2022 à 23:19 +0800, Hsia-Jun Li a écrit :
>>>>>> VIDIOC_ENUM_EXT_PIX_FMT would report NV12 and NV12M, while
>>>>>> VIDIOC_ENUM_FMT
>>>>>> would just report NV12M.
>>>>>
>>>>> If NV12 and NV12M are equivalent in Ext API, I don't see why we would
>>>>> report both (unless I'm missing something, which is probably the case).
>>>>>
>>>>> The idea was to deprecate the M-variants one day.
>>>> I was thinking the way in DRM API is better, always assuming it would
>>>> always in a multiple planes. The only problem is we don't have a way to
>>>> let the allocator that allocate contiguous memory for planes when we
>>>> need to do that.
>>>
>>> Its not too late to allow this to be negotiated, but I would move this out of
>>> the pixel format definition to stop the explosion of duplicate pixel formats,
>>> which is a nightmare to deal with.
>> I wonder whether we need to keep the pixel formats in videodev2.h
>> anymore. If we would like to use the modifiers from drm_fourcc.h, why
>> don't we use their pixel formats, they should be the same values of
>> non-M variant pixel formats of v4l2.
>>
>> Let videodev2.h only maintain the those codecs or motion based
>> compressed (pixel) formats.
>>
>> If I simplify the discussion, we want to
>>> negotiate contiguity with the driver. The new FMT structure should have a
>>> CONTIGUOUS flag. So if userpace sets:
>>>
>>>     S_FMT(NV12, CONTIGUOUS)
>> I wonder whether we would allow some planes being contiguous while some
>> would not. For example, the graphics planes could be in a contiguous
>> memory address while its compression metadata are not.
>> Although that is not the case of our platform. I believe it sounds like
>> reasonable case for improving the performance, two meta planes could
>> resident in a different memory bank.
> 
> I feel like this would be only useful in the MMAP mode. Looking at how
> the other UAPIs are evolving, things are going towards
> userspace-managed allocations, using, for example, DMA-buf heaps. I
> think we should follow the trend and keep the MMAP mode just at the
> same level of functionality as is today and focus on improvements and
> new functionality for the DMABUF mode.
> 
I know there are still some devices(encoder) which only have one 
register for storing the address of a graphics buffer.
>>
>> That lead to another question which I forgot whether I mention it before.
>>
>> There are four modifiers in DRM while we would only one in these patches.
>>   From the EGL
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__registry.khronos.org_EGL_extensions_EXT_EGL-5FEXT-5Fimage-5Fdma-5Fbuf-5Fimport-5Fmodifiers.txt&d=DwIFaQ&c=7dfBJ8cXbWjhc0BhImu8wVIoUFmBzj1s88r8EGyM0UY&r=P4xb2_7biqBxD4LGGPrSV6j-jf3C3xlR7PXU-mLTeZE&m=mCebYOAiZK6pbpH1MrZGq-ZkDW-OqORCSwsCEX9ScgdXk_yfWZFJPC5aC93CUg5F&s=rtmW_t2LYoJ6g3Y5wgyICmABu-2Npw3JCOlvUVIYH2o&e=
>>
>> The modifier for echo plane could be different. I wish it would be
>> better to create a framebuffer being aware of which planes are graphics
>> or metadata.
> 
> What's an echo plane?
> 
They could be
DRM_FORMAT_MOD_SYNA_V4H1_128L128_COMPRESSED
DRM_FORMAT_MOD_SYNA_V4H1_128L128_COMPRESSED
DRM_FORMAT_MOD_SYNA_MTR
DRM_FORMAT_MOD_SYNA_MTR
Or
DRM_FORMAT_MOD_SYNA_V4H3P8_64L4
DRM_FORMAT_MOD_SYNA_V4H3P8_64L4

in our platform. It could give a better idea on what is stored in a plane.
> That said, it indeed looks like we may want to be consistent with DRM
> here and allow per-plane modifiers.
> 
>>
>> I wonder whether it would be better that convincing the DRM maintainer
>> adding a non vendor flag for contiguous memory allocation here(DRM
>> itself don't need it).
>> While whether the memory could be contiguous for these vendor pixel
>> formats, it is complex vendor defined.
> 
> Memory allocation doesn't sound to me like it is related to formats or
> modifiers in any way. I agree with Nicolas that if we want to allow
> the userspace to specify if the memory should be contiguous or not,
> that should be a separate flag and actually I'd probably see it in
> REQBUF_EXT and CREATE_BUFS_EXT, rather than as a part of the format.
> 
I agree with that. But here is a problem, if there was a display 
device(DRM) that only supports contiguous planes in a frame buffer.
How do we be aware of that?
>>
>>>
>>> The driver can accepts, and return the unmodified structure, or may drop the
>>> CONTIGUOUS flag, which would mean its not supported. Could be the other way
>>> around too. As for allocation, if you have CONTIGUOUS flag set, userspace does
>>> not have to export or map memory for each planes, as they are the same. We
>>> simply need to define the offset as relative to their allocation, which I think
>>> is the most sensible thing.
>>>
>>> Nicolas
>>>
>>
>> --
>> Hsia-Jun(Randy) Li

-- 
Hsia-Jun(Randy) Li