linux-kernel - Re: [PATCH v5 3/3] drm/tidss: oldi: Add atomic

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: <327d2ef2-d670-4677-ba1f-b19c2f0b4f3f@ideasonboard.com>
Date: Mon, 1 Sep 2025 11:52:19 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Swamil Jain <s-jain1@...com>
Cc: h-shenoy@...com, devarsht@...com, vigneshr@...com, praneeth@...com,
 u-kumar1@...com, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, jyri.sarha@....fi,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 airlied@...il.com, simona@...ll.ch, aradhya.bhatia@...ux.dev
Subject: Re: [PATCH v5 3/3] drm/tidss: oldi: Add atomic_check hook for oldi
 bridge

Hi,

On 29/08/2025 06:50, Swamil Jain wrote:
> Hi Tomi,
> 
> On 8/27/25 14:35, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 19/08/2025 22:21, Swamil Jain wrote:
>>> From: Jayesh Choudhary <j-choudhary@...com>
>>>
>>> Since OLDI consumes DSS VP clock directly as serial clock, certain
>>> checks cannot be performed in tidss driver which should be checked
>>
>> I think this is a bit misleading. The OLDI input clock doesn't come from
>> DSS, so I wouldn't call it "DSS VP clock". The point here is that the
>> clock from the PLL is used by both OLDI and DSS, and in the current
>> architecture the OLDI driver manages the clock, so the DSS driver can't
>> really do checks, it just has to accept the clock rate. All checks need
>> to be done in the OLDI driver.
>>
>>> in OLDI driver. Add check for mode clock and set max_successful_rate
>>> and max_attempted_rate field for tidss in case the VP is OLDI.
>>>
>>> Fixes: 7246e0929945 ("drm/tidss: Add OLDI bridge support")
>>> Reviewed-by: Devarsh Thakkar <devarsht@...com>
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
>>> Signed-off-by: Swamil Jain <s-jain1@...com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_oldi.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/
>>> tidss/tidss_oldi.c
>>> index ef01ecc17a12..2ed2d0666ccb 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_oldi.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
>>> @@ -309,6 +309,30 @@ static u32
>>> *tidss_oldi_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>>       return input_fmts;
>>>   }
>>>   +static int tidss_oldi_atomic_check(struct drm_bridge *bridge,
>>> +                   struct drm_bridge_state *bridge_state,
>>> +                   struct drm_crtc_state *crtc_state,
>>> +                   struct drm_connector_state *conn_state)
>>> +{
>>> +    struct tidss_oldi *oldi = drm_bridge_to_tidss_oldi(bridge);
>>> +    struct drm_display_mode *adjusted_mode;
>>> +    unsigned long round_clock;
>>> +
>>> +    adjusted_mode = &crtc_state->adjusted_mode;
>>> +
>>> +    if (adjusted_mode->clock > oldi->tidss-
>>> >max_successful_rate[oldi->parent_vp]) {
>>
>> You can change the above check to <=, and return 0 here early.
>>
>>> +        round_clock = clk_round_rate(oldi->serial, adjusted_mode-
>>> >clock * 7 * 1000);
>>> +
>>> +        if (dispc_pclk_diff(adjusted_mode->clock * 7 * 1000,
>>> round_clock) > 5)
>>> +            return -EINVAL;
>>> +
>>> +        oldi->tidss->max_successful_rate[oldi->parent_vp] =
>>> round_clock;
>>> +        oldi->tidss->max_attempted_rate[oldi->parent_vp] =
>>> adjusted_mode->clock * 7 * 1000;
>>> +    }
>>
>> This is not very nice. We should have a function in tidss that we call
>> here, instead of poking into these tidss's variables directly.
>>
>> Actually... Do we even need to use the tidss->max_* fields? The above
>> code is not checking the VP clock maximum, it's actually looking at the
>> serial clock maximum. Currently those two clocks are linked, though, but
>> would it make more sense to have the max_* fields here, in OLDI, for
>> OLDI's serial clock?We don't require tidss->max_* fields here as we
>> only have single mode 
> for OLDI. This is added to make it consistent with check_pixel_clock()
> for validating modes.
> 
> You are right we shouldn't use tidss->* fields directly in OLDI driver.
> 
> As Maxime suggested we only have very few modes for OLDI, can we skip
> caching maximum pixel clock in case of OLDI? What would you suggest Tomi?

I think it's best to at least try the trivial option discussed in this
thread: just use clk_round_rate, without any code to cache or seek out
the max.

 Tomi