Re: [PATCH v8 02/13] drm/mediatek: Implement Display Stream Compression support

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

On 9/15/26 20:51, Nikolai Burov wrote:
> On 9/15/26 6:05 PM, AngeloGioacchino Del Regno wrote:
> [...]
>>>> +    ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Cannot find DSC RC params\n");
>>>> +        return ret;
>>>> +    }
>>>
>>> The defaults in MediaTek's downstream driver match DRM_DSC_1_2_444, not
>>> DRM_DSC_1_1_PRE_SCR, and the panel on my test device requires that even
>>> though it uses DSC v1.1. This may be a panel-specific problem, but have
>>> you tested this with any actual panels that need 1_1_PRE_SCR?
>>>
>>
>> My downstream uses 1_1_PRE_SCR... that's why it is like this.
>>
>> I can resolve it like
>>
>>       if (dsc->dsc_version_major == 1 && dsc->dsc_version_minor == 1)
>>           type = DRM_DSC_1_1_PRE_SCR;
>>       else
>>           type = DRM_DSC_1_2_444;
>>
>>       ret = drm_dsc_setup_rc_params(dsc, type);
>>
>> ...that will work, granted that the panel declares the correct DSC version.
>>
> 
> Unfortunately not, because DRM_DSC_1_2_444 is also needed for some v1.1
> panels. From drivers/gpu/drm/drm_dsc_helper.c:
> 
> /*
>   * Selected Rate Control Related Parameter Recommended Values from DSC v1.2, v1.2a, v1.2b and
>   * DSC_v1.1_E1 specs.
>   *
>   * Cross-checked against C Model releases: DSC_model_20161212 and 20210623
>   */
> static const struct rc_parameters_data rc_parameters_1_2_444[] = {
> ...
> 
> I'm pretty sure that my device is not using DSC v1.2.
> 
> Since this is definitely panel-specific and there seems to be no good
> way to detect the required parameters at the moment, this is probably
> something to fix later.
> 

Ok, let's just go with 1_2_444 then - makes sense... that's something
that must be accounted at the API level, because I know of some panels
that need the PRE_SCR set... and that's even if they're using v1.2, just
because - for whatever reason - some of the DSC params are stored in and
read from an OTP area of the DriverIC (and that would mismatch then with
the other params that are runtime-programmable from the DrIC view, which
results in pixel-garbage artifacts).

Not sure how many panels are doing that, but this is of course not a MTK
specific thing... I looked around, and apparently there's a mention of
that in the Intel driver as well - yay I'm not the only one who knows, lol.

(And I definitely didn't expect that to be the case anyway).

> [...]
>>> I don't quite understand what is going on here, even in the original
>>> driver before this patch. What is the point of setting VACT_NL twice?
>>> Does mtk_dsi_ps_control() really need to be called in mtk_dsi_poweron()
>>> if mtk_dsi_config_vdo_timing() ends up calling it a second time?
>>
>> I didn't do much research, but MT8173 and 8192 if I remember correctly
>> won't work
>> without this sequence. I saw that 2 years ago or something, so I don't
>> remember
>> all that clearly.
>>
> 
> Okay, it's fine to keep it as-is then to avoid breaking older SoCs. One
> change I would definitely recommend though is to move
> mtk_dsi_set_dsc_params() to the very beginning of the sequence, maybe
> even into mtk_dsi_host_attach().
> 
> Otherwise some parts of the code might end up relying on unpopulated
> parameters by accident, and it's harder to debug if they change in the
> middle of the sequence.
> 

Honestly, since it works right now, I would be happier to push this upstream
as-is, and then shuffle things around (for an optimization, or else) after
the fact.

The point is to have a working baseline upstream, then if needed improve it
later when further testing can be done - point is that there's a huge set
of patches that is restructuring everything, so I want to play safe as much
as I can in this moment - I think (and hope) you understand my feeling here.

> [...]
>> We can always come back to this in the future with a separate patch, and
>> we should
>> do just that, otherwise series versions will never come to an end :-)
>>
> 
> Agreed. My work is currently also based on the restructured mediatek-drm
> driver, but I will try to send follow-up patches for MT6858 support once
> some of this stuff lands upstream.
> 

That's... a w e s o m e !

This is a practical test of the restructured code on yet another platform,
and I'm happier now because it's a different type of device (as in, not a
laptop and not an aiot board).

Thanks for that, highly appreciated.

Cheers,
Angelo