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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c5ba072f-5a38-4faf-9b8f-906bc896881d@collabora.com>
Date: Fri, 19 Jul 2024 11:20:04 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: CK Hu (胡俊光) <ck.hu@...iatek.com>,
 "daniel@...ishbar.org" <daniel@...ishbar.org>
Cc: "linux-mediatek@...ts.infradead.org"
 <linux-mediatek@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "wenst@...omium.org" <wenst@...omium.org>,
 "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
 "kernel@...labora.com" <kernel@...labora.com>,
 Shawn Sung (宋孝謙) <Shawn.Sung@...iatek.com>,
 "daniel@...ll.ch" <daniel@...ll.ch>,
 "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 "airlied@...il.com" <airlied@...il.com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "fshao@...omium.org" <fshao@...omium.org>
Subject: Re: [PATCH] drm/mediatek: Set sensible cursor width/height values to
 fix crash

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@...labora.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@...omium.org>
>>>>> Tested-by: Fei Shao <fshao@...omium.org>
>>>>
>>>> Thanks!
>>>
>>> And:
>>> Reviewed-by: Daniel Stone <daniels@...labora.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@...iatek.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
>>
>>