| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1ad79486-b756-4abf-94c7-ab68c2585a7d@ideasonboard.com>
Date: Tue, 16 Sep 2025 14:55:56 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Swamil Jain <s-jain1@...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 v6 2/3] drm/tidss: Remove max_pclk_khz from tidss display
features:
Hi,
On 11/09/2025 14:07, Swamil Jain wrote:
> From: Jayesh Choudhary <j-choudhary@...com>
>
> TIDSS hardware, by itself, does not have variable max pixel clock for
> each VP. The maximum pixel clock is determined by the SoC's clocking
> architecture.
>
> The limitation that has been modeled until now comes from the SoC's
> clocking architecture (PLL can only be programmed to a particular max
> value). Instead of putting it as a constant field in dispc_features,
> we can use clk_round_rate() to see if requested clock can be set or not.
>
> Remove the constant "max_pclk_khz" from dispc_features. In mode_valid()
> call, check if a best frequency match for the mode clock can be found
> or not using clk_round_rate().
>
> Since TIDSS display controller provides clock tolerance of 5%, we use
> this while checking if the requested pixel clock is supported. Also,
> move up dispc_pclk_diff() before it is called.
>
> This will make the existing compatibles reusable if DSS features are
> the same across two SoCs with the only difference being the pixel clock.
>
> Note:
> This uses clk_round_rate() to validate all modes and ensure that the
> driver enumerates only those whose clocking requirements are well
> within the tolerance range. However, this incurs a slight delay, as for
> each mode, clk_round_rate() is called, which takes ~100 us. So, for a
> monitor supporting 30 modes, it takes an extra 3.5 ms to do
> clk_round_rate() to enumerate all modes. If the user wants to bypass
> this validation logic, they can manually modify the driver to bypass
> these calls selectively. For example, they can just do a
> clk_round_rate() check for the highest resolution mode and bypass it
> for the rest of the modes, as done here [1].
>
> [1]: https://lore.kernel.org/all/20250704094851.182131-3-j-choudhary@ti.com/
>
> Fixes: 7246e0929945 ("drm/tidss: Add OLDI bridge support")
> Tested-by: Michael Walle <mwalle@...nel.org>
> 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_dispc.c | 78 +++++++++++------------------
> drivers/gpu/drm/tidss/tidss_dispc.h | 1 -
> drivers/gpu/drm/tidss/tidss_drv.h | 1 -
> 3 files changed, 30 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 7c8c15a5c39b..1cd83a6763ba 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -59,10 +59,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 +141,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 +236,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 +302,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 +358,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 +414,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",
> @@ -1331,25 +1299,50 @@ 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;
> +
> + if (dispc->tidss->is_ext_vp_clk[hw_videoport])
> + return 0;
> + 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;
>
Shouldn't we drop the min_pclk_khz too?
Tomi
> - if (mode->clock > max_pclk)
> + if (check_pixel_clock(dispc, hw_videoport, mode->clock * 1000))
> return MODE_CLOCK_HIGH;
>
> if (mode->hdisplay > 4096)
> @@ -1421,17 +1414,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..fbfe6e304ac8 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -78,7 +78,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;
>
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index e1c1f41d8b4b..f82e282e17a7 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -26,7 +26,6 @@ struct tidss_device {
> struct dispc_device *dispc;
> bool is_ext_vp_clk[TIDSS_MAX_PORTS];
>
> -
> unsigned int num_crtcs;
> struct drm_crtc *crtcs[TIDSS_MAX_PORTS];
>
Powered by blists - more mailing lists