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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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.

 Tomi