Hi all, Thanks for this Laurent. Esaki-san, could you please CC dri-devel@ on discussions like this? On Thu, 18 Nov 2021 at 12:32, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > On Sat, May 11, 2019 at 09:10:27PM +0300, Laurent Pinchart wrote: > > On Thu, May 09, 2019 at 06:25:19PM +0900, Esaki Tomohito wrote: > > > Weston compositor (v5.0.0 or later) uses the DRM API to get the > > > supported modifiers and determines if the sprite plane can be used by > > > comparing the modifiers with the client specified modifier. > > > In currently driver, since the weston doesn't know supported modifiers, > > > that cannot determine if the received dmabuf can be passed through to > > > sprite plane. > > > Since there are R-Car GPU which support linear modifier, the sprite > > > plane cannot be used in a compositor similar to the weston if client > > > specify linear modifier. > > > > I don't think the right solution is to expose the linear modifier from > > all drivers that don't otherwise support modifiers. We should instead > > fix it either in Weston, and treat drivers that don't support the > > modifiers API as supporting the linear modifier only, or in the DRM/KMS > > core by reporting the linear modifier for drivers that don't explicitly > > support modifiers. > > I've been pointed to > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3350#note_1161827, > and we had a discussion on the #dri-devel IRC channel today on this > topic. It turns out I was wrong, not specifying modifiers in userspace > is different than specifying a linear modifier. This is true for some > legacy drivers only (e.g. radeon) that pre-date the modifiers API, and > which select a tiling format internally based on some heuristics. > > I still don't like this patch though, as it would need to be replicated > in most drivers. It would be better if we could handle this in the DRM > core. Daniel kindly offered to summarize the IRC discussion in a reply > to this e-mail. Just quickly, I believe the check for the linear modifier in fb_create is unnecessary, as this should already be checked in the core through format_mod_supported(). There is indeed a difference between LINEAR and INVALID. Linear is an explicit declaration of the layout; INVALID (i.e. no modifier) means 'I don't know what this is, so you should guess'. Guessing is obviously not reliable, so Weston only passes buffers with no modifier to KMS in two cases. The first case is when we allocate a dumb buffer and the driver does not support modifiers; this is safe since it's the same driver. The second case is when either the GPU driver or KMS driver do not support modifiers and we allocate a buffer via GBM with USE_SCANOUT; in this case, it is GBM's responsibility to select the 'right' layout. We will never create a DRM framebuffer with no modifiers when the original buffer comes from a client. If the client does not support modifiers but the KMS driver does, then we do not know that the client has allocated a suitable layout, so this is not safe. If the client does explicitly declare a modifier but the KMS driver does not support modifiers, then we also do not know that this is safe to use. So unless both sides (client/GPU and KMS) support modifiers, we do not do direct scanout from client buffers. This patch would enable this usecase by declaring support for the linear modifier from KMS; when used with a PVR driver which explicitly declares the linear modifier, we know it is safe to pass that client buffer to KMS. Laurent's concern is that the DRM core should handle this rather than open-coding in every driver, which I agree with. Some drivers (e.g. radeon, maybe legacy NV?) do not support modifiers, and _also_ do magic inference of the actual layout of the underlying buffer. However, these drivers are legacy and we do not accept any new addition of inferring layout without modifiers. I think the best way forward here is: - add a new mode_config.cannot_support_modifiers flag, and enable this in radeon (plus any other drivers in the same boat) - change drm_universal_plane_init() to advertise the LINEAR modifier when NULL is passed as the modifier list (including installing a default .format_mod_supported hook) - remove the mode_config.allow_fb_modifiers hook and always advertise modifier support, unless mode_config.cannot_support_modifiers is set FWIW, the effective modifier API and also valid usage is documented here, which should be finished and merged shortly: https://lore.kernel.org/dri-devel/20210905122742.86029-1-daniels@xxxxxxxxxxxxx/ Cheers, Daniel
