Re: [PATCH] drm: rcar-du: add modifiers support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Thu, Nov 18, 2021 at 01:02:11PM +0000, Daniel Stone wrote:
> 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 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)

Is there an easy way to identify the drivers that need this ?

>   - 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

Looks good to me.

> 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@collabora.com/

-- 
Regards,

Laurent Pinchart