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

[Esaki Tomohito <etom@igel.co.jp>]

Hello Daniel

Thanks for the summary of the discussion.

On 2021/11/18 22:02, Daniel Stone wrote:
> Hi all,
> Thanks for this Laurent. Esaki-san, could you please CC dri-devel@ on
> discussions like this?

I'm sorry.
I will check using get_maintainer.pl next time.

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

--
Best Regards
Tomohito Esaki