Re: [PATCH v5 3/3] drm/tidss: oldi: Add atomic_check hook for oldi bridge

[Swamil Jain <s-jain1@ti.com>]

Hi Tomi,

On 9/1/25 14:22, Tomi Valkeinen wrote:
> 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@ti.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@ti.com>
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> Signed-off-by: Swamil Jain <s-jain1@ti.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.

Then for OLDI, we will drop caching the max_pclk.

Regards,
Swamil
> 
>   Tomi
>