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
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]) {
+ 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;
+ }
+
+ return 0;
+}
+
static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
.attach = tidss_oldi_bridge_attach,
.atomic_pre_enable = tidss_oldi_atomic_pre_enable,
@@ -317,6 +341,7 @@ static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_check = tidss_oldi_atomic_check,
};
static int get_oldi_mode(struct device_node *oldi_tx, int *companion_instance)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?
Tomi
> +
> + return 0;
> +}
> +
> static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
> .attach = tidss_oldi_bridge_attach,
> .atomic_pre_enable = tidss_oldi_atomic_pre_enable,
> @@ -317,6 +341,7 @@ static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> + .atomic_check = tidss_oldi_atomic_check,
> };
>
> static int get_oldi_mode(struct device_node *oldi_tx, int *companion_instance)
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?
Regards,
Swamil
>
> Tomi
>
>> +
>> + return 0;
>> +}
>> +
>> static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
>> .attach = tidss_oldi_bridge_attach,
>> .atomic_pre_enable = tidss_oldi_atomic_pre_enable,
>> @@ -317,6 +341,7 @@ static const struct drm_bridge_funcs tidss_oldi_bridge_funcs = {
>> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>> .atomic_reset = drm_atomic_helper_bridge_reset,
>> + .atomic_check = tidss_oldi_atomic_check,
>> };
>>
>> static int get_oldi_mode(struct device_node *oldi_tx, int *companion_instance)
>
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
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
>
© 2016 - 2025 Red Hat, Inc.