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

[Daniel Stone <daniel@fooishbar.org>]

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

Cheers,
Daniel