Hi, On 28/10/2025 05:39, Swamil Jain wrote: > From: Jayesh Choudhary <[email protected]> > > The TIDSS hardware does not have independent maximum or minimum pixel > clock limits for each video port. Instead, these limits are determined > by the SoC's clock architecture. Previously, this constraint was > modeled using the 'max_pclk_khz' and 'min_pclk_khz' fields in > 'dispc_features', but this approach is static and does not account for > the dynamic behavior of PLLs. > > This patch removes the 'max_pclk_khz' and 'min_pclk_khz' fields from > 'dispc_features'. The correct way to check if a requested mode's pixel > clock is supported is by using 'clk_round_rate()' in the 'mode_valid()' > hook. If the best frequency match for the mode clock falls within the > supported tolerance, it is approved. TIDSS supports a 5% pixel clock > tolerance, which is now reflected in the validation logic. > > This change allows existing DSS-compatible drivers to be reused across > SoCs that only differ in their pixel clock characteristics. The > validation uses 'clk_round_rate()' for each mode, which may introduce > additional delay (about 3.5 ms for 30 modes), but this is generally > negligible. Users desiring faster validation may bypass these calls > selectively, for example, checking only the highest resolution mode, > as shown here[1]. > > [1]: https://lore.kernel.org/all/[email protected]/ > > Tested-by: Michael Walle <[email protected]> > 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_dispc.c | 85 ++++++++++------------------- > drivers/gpu/drm/tidss/tidss_dispc.h | 3 - > 2 files changed, 30 insertions(+), 58 deletions(-) > > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c > b/drivers/gpu/drm/tidss/tidss_dispc.c > index d0b191c470ca..07731b02490f 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.c > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -57,12 +57,6 @@ static const u16 > tidss_k2g_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > }; > > const struct dispc_features dispc_k2g_feats = { > - .min_pclk_khz = 4375, > - > - .max_pclk_khz = { > - [DISPC_VP_DPI] = 150000, > - }, > - > /* > * XXX According TRM the RGB input buffer width up to 2560 should > * work on 3 taps, but in practice it only works up to 1280. > @@ -145,11 +139,6 @@ static const u16 > tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > }; > > const struct dispc_features dispc_am65x_feats = { > - .max_pclk_khz = { > - [DISPC_VP_DPI] = 165000, > - [DISPC_VP_OLDI_AM65X] = 165000, > - }, > - > .scaling = { > .in_width_max_5tap_rgb = 1280, > .in_width_max_3tap_rgb = 2560, > @@ -245,11 +234,6 @@ static const u16 > tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = { > }; > > const struct dispc_features dispc_j721e_feats = { > - .max_pclk_khz = { > - [DISPC_VP_DPI] = 170000, > - [DISPC_VP_INTERNAL] = 600000, > - }, > - > .scaling = { > .in_width_max_5tap_rgb = 2048, > .in_width_max_3tap_rgb = 4096, > @@ -316,11 +300,6 @@ const struct dispc_features dispc_j721e_feats = { > }; > > const struct dispc_features dispc_am625_feats = { > - .max_pclk_khz = { > - [DISPC_VP_DPI] = 165000, > - [DISPC_VP_INTERNAL] = 170000, > - }, > - > .scaling = { > .in_width_max_5tap_rgb = 1280, > .in_width_max_3tap_rgb = 2560, > @@ -377,15 +356,6 @@ const struct dispc_features dispc_am625_feats = { > }; > > const struct dispc_features dispc_am62a7_feats = { > - /* > - * if the code reaches dispc_mode_valid with VP1, > - * it should return MODE_BAD. > - */ > - .max_pclk_khz = { > - [DISPC_VP_TIED_OFF] = 0, > - [DISPC_VP_DPI] = 165000, > - }, > - > .scaling = { > .in_width_max_5tap_rgb = 1280, > .in_width_max_3tap_rgb = 2560, > @@ -442,10 +412,6 @@ const struct dispc_features dispc_am62a7_feats = { > }; > > const struct dispc_features dispc_am62l_feats = { > - .max_pclk_khz = { > - [DISPC_VP_DPI] = 165000, > - }, > - > .subrev = DISPC_AM62L, > > .common = "common", > @@ -1333,33 +1299,53 @@ static void dispc_vp_set_default_color(struct > dispc_device *dispc, > DISPC_OVR_DEFAULT_COLOR2, (v >> 32) & 0xffff); > } > > +/* > + * Calculate the percentage difference between the requested pixel clock rate > + * and the effective rate resulting from calculating the clock divider value. > + */ > +unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate) > +{ > + int r = rate / 100, rr = real_rate / 100; > + > + return (unsigned int)(abs(((rr - r) * 100) / r)); > +} > + > +static int check_pixel_clock(struct dispc_device *dispc, > + u32 hw_videoport, unsigned long clock) > +{ > + unsigned long round_clock; > + > + round_clock = clk_round_rate(dispc->vp_clk[hw_videoport], clock); > + /* > + * To keep the check consistent with dispc_vp_set_clk_rate(), we > + * use the same 5% check here. > + */ > + if (dispc_pclk_diff(clock, round_clock) > 5) > + return -EINVAL; > + return 0; > +} > + > enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc, > u32 hw_videoport, > const struct drm_display_mode *mode) > { > u32 hsw, hfp, hbp, vsw, vfp, vbp; > enum dispc_vp_bus_type bus_type; > - int max_pclk; > > bus_type = dispc->feat->vp_bus_type[hw_videoport]; > > - max_pclk = dispc->feat->max_pclk_khz[bus_type]; > - > - if (WARN_ON(max_pclk == 0)) > + if (WARN_ON(bus_type == DISPC_VP_TIED_OFF)) > return MODE_BAD; > > - if (mode->clock < dispc->feat->min_pclk_khz) > - return MODE_CLOCK_LOW; > - > - if (mode->clock > max_pclk) > - return MODE_CLOCK_HIGH; > - > if (mode->hdisplay > 4096) > return MODE_BAD; > > if (mode->vdisplay > 4096) > return MODE_BAD; > > + if (check_pixel_clock(dispc, hw_videoport, mode->clock * 1000)) > + return MODE_CLOCK_HIGH; > +
I think it's better to just inline check_pixel_clock here, it's just a few lines. Also, returning MODE_CLOCK_HIGH is not correct. I see other drivers using MODE_NOCLOCK or MODE_CLOCK_RANGE. Other than that: Reviewed-by: Tomi Valkeinen <[email protected]> Tomi > /* TODO: add interlace support */ > if (mode->flags & DRM_MODE_FLAG_INTERLACE) > return MODE_NO_INTERLACE; > @@ -1423,17 +1409,6 @@ void dispc_vp_disable_clk(struct dispc_device *dispc, > u32 hw_videoport) > clk_disable_unprepare(dispc->vp_clk[hw_videoport]); > } > > -/* > - * Calculate the percentage difference between the requested pixel clock rate > - * and the effective rate resulting from calculating the clock divider value. > - */ > -unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate) > -{ > - int r = rate / 100, rr = real_rate / 100; > - > - return (unsigned int)(abs(((rr - r) * 100) / r)); > -} > - > int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport, > unsigned long rate) > { > diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h > b/drivers/gpu/drm/tidss/tidss_dispc.h > index 60c1b400eb89..42279312dcc1 100644 > --- a/drivers/gpu/drm/tidss/tidss_dispc.h > +++ b/drivers/gpu/drm/tidss/tidss_dispc.h > @@ -77,9 +77,6 @@ enum dispc_dss_subrevision { > }; > > struct dispc_features { > - int min_pclk_khz; > - int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE]; > - > struct dispc_features_scaling scaling; > > enum dispc_dss_subrevision subrev;
