[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7f646e70-73fa-43ea-9fab-c9a0cb64dff6@ti.com>
Date: Mon, 1 Sep 2025 14:29:22 +0530
From: Swamil Jain <s-jain1@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.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 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@...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.
Then for OLDI, we will drop caching the max_pclk.
Regards,
Swamil
>
> Tomi
>
Powered by blists - more mailing lists