linux-kernel - Re: [PATCH v3 13/17] drm/bridge: cdns-dsi: Fix REG_WAKEUP

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <213b2515-95d9-4a26-924f-d959ddc3447c@ideasonboard.com>
Date: Fri, 25 Apr 2025 14:42:09 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org, Francesco Dolcini <francesco@...cini.it>,
 Devarsh Thakkar <devarsht@...com>, Jyri Sarha <jyri.sarha@....fi>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Jayesh Choudhary <j-choudhary@...com>
Subject: Re: [PATCH v3 13/17] drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value

On 15/04/2025 23:10, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 14/04/25 16:41, Tomi Valkeinen wrote:
>> The driver tries to calculate the value for REG_WAKEUP_TIME. However,
>> the calculation itself is not correct, and to add on it, the resulting
>> value is almost always larger than the field's size, so the actual
>> result is more or less random.>
>> According to the docs, figuring out the value for REG_WAKEUP_TIME
>> requires HW characterization and there's no way to have a generic
>> algorithm to come up with the value. That doesn't help at all...
>>
>> However, we know that the value must be smaller than the line time, and,
>> at least in my understanding, the proper value for it is quite small.
>> Testing shows that setting it to 1/10 of the line time seems to work
>> well. All video modes from my HDMI monitor work with this algorithm.
>>
>> Hopefully we'll get more information on how to calculate the value, and
>> we can then update this.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 182845c54c3d..fb0623d3f854 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -786,7 +786,13 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>>   
>>   	tx_byte_period = DIV_ROUND_DOWN_ULL((u64)NSEC_PER_SEC * 8,
>>   					    phy_cfg->hs_clk_rate);
>> -	reg_wakeup = (phy_cfg->hs_prepare + phy_cfg->hs_zero) / tx_byte_period;
> 
> I think the primary point of failure in the original calculation is due
> to fact that the hs_prepare and hs_zero are defined in picoseconds (ps),
> and the tx_byte_period is in nanoseconds (ns) as evident by the usage of
> NSEC_PER_SEC macro.
> 
> The resulting tx_by_period is 1000 times smaller, and the reg_wakeup - a
> 1000 times larger. =)

That is true. It didn't work with that fixed either =).

> Further, the TRM does indeed mention that some characterization is
> required to fine tune the exact reg_wakeup value, but it ends up giving
> a vague-ish formula -
> 
> -> reg_wakeup_time = wakeup_time_dsi + wakeup_time_cl + wakeup_time_dl +
> 		     (hs_host_eot × 4 / lane_nb)
> 
> I think the characterization may only be required for the
> wakeup_time_dsi component. The existing formula in the driver (after
> corrected for time unit) is the wakeup_time_dl component. wakeup_time_cl
> seems to be a range of constants, which the phy-core is auto-settling on
> defaults. The document never specifically mentions "hs_host_eot" other
> than the equation, but on the off-chance it is same as phy_cfg->eot,
> then that's 0 and avoidable.

Yep, I tried to decipher how to calculate it using the TRM and the MIPI 
spec, but I just couldn't figure it out. In any case we need to 
characterization (or a known safe value that will be enough for all 
cases) to use a formula. Unfortunately I haven't gotten any details 
about this.

  Tomi