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 <[email protected]> >>> >>> 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 <[email protected]> >>> Signed-off-by: Jayesh Choudhary <[email protected]> >>> Signed-off-by: Swamil Jain <[email protected]> >>> --- >>> 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
