From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fllvem-ot04.ext.ti.com (fllvem-ot04.ext.ti.com [198.47.19.246]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D6DCE32BF2E for ; Thu, 28 Aug 2025 16:45:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.47.19.246 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756399548; cv=none; b=SLU5BzyLXaXYjd3+gOdiduqae/wiMHuTfgekqGVrRtjBs9Iv0FZAZu68iBKNT0aVER8Hg4Kk19C8S49/r6JLGLP7E9MNBb7CRosMO383s4srFFG5ut0nRYXm1M5f7MNF6198wwd/O8v/uq8gHBwv49ZKuQfr8Ys9/s9L+96aDMA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756399548; c=relaxed/simple; bh=DkoHGMXPv+Ziz7tic5ZhCq23uuGDr/sdhHtebp3o6vo=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=GZVT6I9fqykmtyIhWalrIk7T8Vfinu2XXb3H7IdDWY7ZXkoLYoULgLyLHMX/gelRa4SAK4OCf94VwwNGHF+PrWPpfIP6Om6u5oTTVlX2ICn0ijg3n/SWEJW/XtUWLWQW8kYrE4bfu/vIgxogZZM4TqDspevumNtOxFGXWkO5H/Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com; spf=pass smtp.mailfrom=ti.com; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b=olV9FDnI; arc=none smtp.client-ip=198.47.19.246 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ti.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b="olV9FDnI" Received: from lelvem-sh02.itg.ti.com ([10.180.78.226]) by fllvem-ot04.ext.ti.com (8.15.2/8.15.2) with ESMTP id 57SGisPw2022902; Thu, 28 Aug 2025 11:44:54 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1756399494; bh=uvOzIUOt4mRyEwxd4Xgg6NBzk4tyhr5ea4GoxBX2rz0=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=olV9FDnIudQJKaDsNmUyDlKi+4Qe8y9KFTAjT4QvBNDEezi4sE9wlkFbsW9PuMYUe 76UWa6Tb/Qv4jnc9pE/mPakWBgZIxu5shDVqVnYwtKoCVLM5+2Iu9jW/mqXpidhv6U TM2sPTvjj6QulzAiRGwkARXu72PpD52E4+MjtsM0= Received: from DFLE105.ent.ti.com (dfle105.ent.ti.com [10.64.6.26]) by lelvem-sh02.itg.ti.com (8.18.1/8.18.1) with ESMTPS id 57SGisS93593614 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA256 bits=128 verify=FAIL); Thu, 28 Aug 2025 11:44:54 -0500 Received: from DFLE102.ent.ti.com (10.64.6.23) by DFLE105.ent.ti.com (10.64.6.26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.55; Thu, 28 Aug 2025 11:44:53 -0500 Received: from lelvem-mr05.itg.ti.com (10.180.75.9) by DFLE102.ent.ti.com (10.64.6.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.55 via Frontend Transport; Thu, 28 Aug 2025 11:44:53 -0500 Received: from [172.24.233.20] (a0512632.dhcp.ti.com [172.24.233.20]) by lelvem-mr05.itg.ti.com (8.18.1/8.18.1) with ESMTP id 57SGinYH928769; Thu, 28 Aug 2025 11:44:50 -0500 Message-ID: <9e09e8d9-d2b4-4c94-955a-903aec4e812e@ti.com> Date: Thu, 28 Aug 2025 22:14:49 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 2/3] drm/tidss: Remove max_pclk_khz from tidss display features To: Tomi Valkeinen CC: , , , , , , , , , , , , , References: <20250819192113.2420396-1-s-jain1@ti.com> <20250819192113.2420396-3-s-jain1@ti.com> Content-Language: en-US From: Swamil Jain In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-C2ProcessedOrg: 333ef613-75bf-4e12-a4b1-8e3623f5dcea On 8/27/25 14:19, Tomi Valkeinen wrote: > Hi, > > On 19/08/2025 22:21, Swamil Jain wrote: >> From: Jayesh Choudhary >> >> TIDSS hardware by itself does not have variable max_pclk for each VP. >> The maximum pixel clock is determined by the limiting factor between >> the functional clock and the PLL (parent to the VP/pixel clock). > > Hmm, this is actually not in the driver, is it? We're not limiting the > pclk based on the fclk. Hi Tomi, We are checking what all pclks can be supported, which is limited by fclk, thats what Jayesh wanted to mention here. > >> The limitation that has been modeled till now comes from the clock >> (PLL can only be programmed to a particular max value). Instead of >> putting it as a constant field in dispc_features, we can query the >> DM to see if requested clock can be set or not and use it in >> mode_valid(). >> >> Replace constant "max_pclk_khz" in dispc_features with >> max_successful_rate and max_attempted_rate, both of these in >> tidss_device structure would be modified in runtime. In mode_valid() >> call, check if a best frequency match for mode clock can be found or >> not using "clk_round_rate()". Based on that, propagate >> max_successful_rate and max_attempted_rate and query DM again only if >> the requested mode clock is greater than max_attempted_rate. (As the >> preferred display mode is usually the max resolution, driver ends up >> checking the highest clock the first time itself which is used in >> subsequent checks). >> >> Since TIDSS display controller provides clock tolerance of 5%, we use >> this while checking the max_successful_rate. Also, move up >> "dispc_pclk_diff()" before it is called. >> >> This will make the existing compatibles reusable if DSS features are >> same across two SoCs with the only difference being the pixel clock. >> >> Fixes: 7246e0929945 ("drm/tidss: Add OLDI bridge support") >> Reviewed-by: Devarsh Thakkar >> Signed-off-by: Jayesh Choudhary >> Signed-off-by: Swamil Jain >> --- >> drivers/gpu/drm/tidss/tidss_dispc.c | 85 +++++++++++++---------------- >> drivers/gpu/drm/tidss/tidss_dispc.h | 1 - >> drivers/gpu/drm/tidss/tidss_drv.h | 11 +++- >> 3 files changed, 47 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c >> index c0277fa36425..c2c0fe0d4a0f 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -58,10 +58,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. >> @@ -144,11 +140,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, >> @@ -244,11 +235,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, >> @@ -315,11 +301,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, >> @@ -376,15 +357,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, >> @@ -441,10 +413,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", >> @@ -1347,25 +1315,57 @@ 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; >> + >> + if (clock <= dispc->tidss->max_successful_rate[hw_videoport]) >> + return 0; >> + >> + if (clock < dispc->tidss->max_attempted_rate[hw_videoport]) >> + return -EINVAL; >> + >> + round_clock = clk_round_rate(dispc->vp_clk[hw_videoport], clock); >> + >> + if (dispc_pclk_diff(clock, round_clock) > 5) >> + return -EINVAL; >> + >> + dispc->tidss->max_successful_rate[hw_videoport] = round_clock; >> + dispc->tidss->max_attempted_rate[hw_videoport] = clock; > > I still don't think this logic is sound. This is trying to find the > maximum clock rate, and optimize by avoiding the calls to > clk_round_rate() if possible. That makes sense. > > But checking for the 5% tolerance breaks it, in my opinion. If we find > out that the PLL can do, say, 100M, but we need pclk of 90M, the current > maximum is still the 100M, isn't it? > > Why can't we replace the "if (mode->clock > max_pclk)" check with a new > check that only looks for the max rate? If we want to add tolerance > checks to mode_valid (which are currently not there), let's add it in a > separate patch. > Yeah, we can drop tolerance check. So, should we drop check_pixel_clock() and also clk_round_rate(), but then how can we know the maximum supported pclk? Regards, Swamil > Tomi > >> + 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) >> + if (check_pixel_clock(dispc, hw_videoport, mode->clock * 1000)) >> return MODE_CLOCK_HIGH; >> >> if (mode->hdisplay > 4096) >> @@ -1437,17 +1437,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 b8614f62186c..45b1a8aa9089 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >> @@ -75,7 +75,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 4e38cfa99e84..667c0d772519 100644 >> --- a/drivers/gpu/drm/tidss/tidss_drv.h >> +++ b/drivers/gpu/drm/tidss/tidss_drv.h >> @@ -23,7 +23,16 @@ struct tidss_device { >> const struct dispc_features *feat; >> struct dispc_device *dispc; >> bool is_ext_vp_clk[TIDSS_MAX_PORTS]; >> - >> + /* >> + * Stores highest pixel clock value found to be valid while checking >> + * supported modes for connected display >> + */ >> + unsigned long max_successful_rate[TIDSS_MAX_PORTS]; >> + /* >> + * Stores the highest attempted pixel clock rate whose validated >> + * clock is within the tolerance range >> + */ >> + unsigned long max_attempted_rate[TIDSS_MAX_PORTS]; >> >> unsigned int num_crtcs; >> struct drm_crtc *crtcs[TIDSS_MAX_PORTS]; >