Re: [PATCH] drm/mediatek: Set sensible cursor width/height values to fix crash

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 19/07/24 10:42, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Thu, 2024-07-18 at 13:23 +0200, AngeloGioacchino Del Regno wrote:
>> Il 18/07/24 13:10, Daniel Stone ha scritto:
>>> Hi all,
>>>
>>> On Thu, 18 Jul 2024 at 11:24, AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>> Il 18/07/24 11:27, Fei Shao ha scritto:
>>>>> This matches my preference in [1], so of course I'd like to see it
>>>>> merged... if maintainers are okay with it.
>>>>> Given I've tested the exact same change before:
>>>>> Reviewed-by: Fei Shao <fshao@chromium.org>
>>>>> Tested-by: Fei Shao <fshao@chromium.org>
>>>>
>>>> Thanks!
>>>
>>> And:
>>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>>>
>>>>>> OOTH, Intel recently added a feature for enumerating "suggested"
>>>>>> cursor sizes. See https://urldefense.com/v3/__https://patchwork.freedesktop.org/patch/583299/__;!!CTRNKA9wMg0ARbw!nRf6mf-9tnE7vLYracLE6Xq_oblRvtENffF73fRzgz_E3zPc3yxeQPE5yPw95sj-ZeoiYJCQSIPWFZ0C3HCXpBkHikWK$
>>>>>>
>>>>>> Not sure if other compositors will end up using it or not.
>>>>
>>>> Yeah, that's good, and we might do that as well in MediaTek DRM... in a slightly
>>>> different way, as it looks like they are simply hinting the same values as the
>>>> mode_config is declaring... while we'd be adding a hint with a sensible size that
>>>> is less than the maximum supported one from the overlay.
>>>>
>>>> In reality, here, the issue is that the most popular compositors do not support
>>>> overlay planes (as in, they don't use them at all)... my first idea was to remove
>>>> the CURSOR plane entirely and declare it as per what it is for real (an OVERLAY),
>>>> but that would only give a performance penalty as that'd become yet another unused
>>>> plane and nothing else.
>>>>
>>>> If at least the most popular compositors did support overlay planes, I'd have done
>>>> that instead... but oh, well!
>>>>
>>>> And anyway I hope that the maintainers are okay with this because, well, otherwise
>>>> MediaTek SoCs won't be usable with any popular WM.
>>>
>>> Every compositor is going to use it, yeah. But until it does, people
>>> are just going to use cursor_width and cursor_size. A lot of older
>>> desktop hardware supports only a single fixed dimension for the cursor
>>> plane (hence the single values), so rather than guess if it needs to
>>> be 32x32 or 64x64 or whatever, people just allocate to the size. Not
>>> to mention that the old pre-atomic cursor ioctls actually require that
>>> you allocate for cursor_width x cursor_height.
>>>
>>> So yeah, this is the right fix - though you could even be more
>>> aggressive and reduce it to 256x256 - and supporting the CURSOR_SIZE
>>> property would be even more useful again.
>>>
>>
>> I thought about being more aggressive, but then I saw that IGT tests for up to 512
>> and that MSM also declares the same, so, making IGT happy because we can indeed
>> support that much (since we can support even more, but doesn't make sense) :-)
>>
>> Regarding CURSOR_SIZE ... right, I can take a look at that a bit later, most
>> probably not for this merge window, though.
> 
> This patch looks acceptable but it could be better.
> It's urgent to fix the crash, if better solution does not come out soon,
> I would apply this patch first.
> 
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> 
> I will remove the Fixes tag because Shawn's patch has no logical problem but the system resource is not enough.
> 
> It's a dilemma that small size has no resource problem but application is limited
> and large size has resource problem but support more application.
> 

Thanks, but the Fixes tag is important, as otherwise v6.11 will be unusable :-)

Regards,
Angelo

> Regards,
> CK
> 
>>
>> Cheers!
>>
>>> Cheers,
>>> Daniel
>>
>>