Messages in this thread | | | Date | Fri, 11 Nov 2022 17:13:50 +0800 | Subject | Re: [RFC PATCH v6 02/11] media: v4l2: Extend pixel formats to unify single/multi-planar handling (and more) | From | Hsia-Jun Li <> |
| |
On 11/11/22 16:52, 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 3:31 PM Hsia-Jun Li <Randy.Li@synaptics.com> wrote: >> >> >> >> 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. > > For those, the legacy MMAP mode (with existing functionality) can be > successfully used, we wouldn't be removing it any time soon. Just > don't want to design new functionality specifically for the legacy > mode. > But it prevents the encoder using the buffer from the outside. For example, there was an PCI-e interface camera which would write to the system memory where is configured to its register, then we would like to encode those buffers. >>>> >>>> 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. > > Yes, that's what I was thinking, but my question is more about what > those planes hold. DRM_FORMAT_MOD_SYNA_V4H1* or DRM_FORMAT_MOD_SYNA_V4H3P8* would be the luma and chroma (un)compressed data here. They are modifiers to NV12 and NV15. Are you sure that they should be planes of the same > buffer rather than separate buffers? I am not sure about your question here. I prefer they are in a different memory plane. But not all Android APIs support that. If I just think about our platform and GNU Linux, I won't care about those limitations. > >>> 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? > > That's why I think the MMAP mode is not scalable and shouldn't be > expanded anymore. Both V4L2 and DRM devices should describe their > constraints to the userspace and then the userspace should allocate > accordingly from the right DMA-buf heap. (Or as Android and ChromeOS > do, just have a central allocator library that understands the > constraints, so there is no need to query the drivers.) > Because we are working for embedded platforms which don't have memory beyond the system memory. I believe those GPU vendors would hate idea of DMAheap only. >>>> >>>>> >>>>> 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
-- Hsia-Jun(Randy) Li
| |