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

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

Hi Daniel,

On 2021/11/18 23:05, Laurent Pinchart wrote:
> 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 ?

Should I find a driver that has not use drm_plane_funcs?

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

I agree with this way, I'll try to create a patches.

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

-- 
Best Regards
Tomohito Esaki