On Fri, Nov 11, 2022 at 5:43 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Tomasz, > > On Fri, Nov 11, 2022 at 02:48:48PM +0900, Tomasz Figa wrote: > > On Fri, Nov 11, 2022 at 12:04 PM Hsia-Jun Li wrote: > > > On 11/11/22 01:06, Nicolas Dufresne wrote: > > > > 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 agree, but we will need an API to expose the memory constraints of the > device, or userspace won't be able to allocate memory compatible with > the hardware or driver requirements. Yes, I fully agree and that's why I think we should rather focus our efforts in that direction rather than expanding the existing MMAP capabilities. > > > > 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://registry.khronos.org/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import_modifiers.txt > > > > > > 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? > > > > 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 like how DRM decouples allocation of buffer objects and creation of > frame buffers. Exactly why I proposed so rather than coupling it with S_FMT. (But then it's moot if we decide to focus on DMABUF mode.) > > > > > 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. > > -- > Regards, > > Laurent Pinchart
