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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
Tested-by: Fei Shao <fshao@xxxxxxxxxxxx>
Thanks!
And:
Reviewed-by: Daniel Stone <daniels@xxxxxxxxxxxxx>
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@xxxxxxxxxxxx>
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
