Hi,
On 04/11/2025 17:14, 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]>
> Reviewed-by: Tomi Valkeinen <[email protected]>
> Signed-off-by: Jayesh Choudhary <[email protected]>
> Signed-off-by: Swamil Jain <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 86 +++++++++++------------------
> drivers/gpu/drm/tidss/tidss_dispc.h | 3 -
> 2 files changed, 31 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
> b/drivers/gpu/drm/tidss/tidss_dispc.c
> index d0b191c470ca..b11880178cba 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,54 @@ 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 inline int check_pixel_clock(struct dispc_device *dispc,
> + u32 hw_videoport, unsigned long clock)
> +{
Ah... Sorry, I was quite unclear in my comment to v7. I did not mean
mark it as inline. I meant "move it inline", i.e. move this code into
the dispc_vp_mode_valid function. This is just a few lines, and having
it in a separate function makes it a bit more difficult to understand
what are all the checks done in dispc_vp_mode_valid().
I can do that change when applying the patches, if that's ok for you.
Tomi
> + 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_RANGE;
> +
> /* TODO: add interlace support */
> if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> return MODE_NO_INTERLACE;
> @@ -1423,17 +1410,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;
