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

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

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.

Regards,
CK

> 
> Cheers!
> 
> > Cheers,
> > Daniel
> 
>