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 3A9B61C8605 for ; Fri, 29 Aug 2025 03:37:52 +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=1756438677; cv=none; b=me82TKBhZmeN4y65D0KBmrvU8kgkl4eYQNkLejuf2DYaf/zHyQdwCBlr8tNCQOLHQzLR467cRtXd8EDBzPOUP3bUGlPos4Hus/7EOmgA0lZcaLOI239yJluZlwNC5KtNLYjBTW/k6fQ0HRTiicAz4u0yzJGVW8unhnk6jvxParU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756438677; c=relaxed/simple; bh=nDgnKyzcbYY3p7RC3jwPLcQbnLe8kaHR1CrAjCIbfRc=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=SvoMqmHB3ZKI2RHy5xf0/h5YDuH4S4FmVtq70Jde7gvIlXEDlttDf5n9HMzl8vnibIBt+YYyBvFRbGl8+3D8ZI9nJt3tVYSgPih2gT3lHEfG7H2/CM91fX8gwykcMH0fzGPrSPEqMxr5ncUuOU580Exn6dTR7+5O8Qap10BUIRw= 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=ZILu7fZS; 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="ZILu7fZS" Received: from fllvem-sh04.itg.ti.com ([10.64.41.54]) by fllvem-ot04.ext.ti.com (8.15.2/8.15.2) with ESMTP id 57T3bd502125776; Thu, 28 Aug 2025 22:37:39 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1756438659; bh=UW6m2BOAbKNzRFbZe7Dtf33MMjlhECUi7KY48ZC4FHk=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=ZILu7fZSf8E9C6tI4m3z3YvWvvpYQT9kjwyl35+dcQD5SEC2NjEre8sl6rprakHE2 q4KqW+pkxHYOJv2A9BQpu6j5P9Ng3kHen4h8OGrjuF3qC30uxCdMH5kHbe5OQY/EhS p0QJ63QkgK/nRM5q0JkDaIDEEfl8qXc672AnnajE= Received: from DLEE100.ent.ti.com (dlee100.ent.ti.com [157.170.170.30]) by fllvem-sh04.itg.ti.com (8.18.1/8.18.1) with ESMTPS id 57T3bXbK249296 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA256 bits=128 verify=FAIL); Thu, 28 Aug 2025 22:37:38 -0500 Received: from DLEE107.ent.ti.com (157.170.170.37) by DLEE100.ent.ti.com (157.170.170.30) 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 22:37:33 -0500 Received: from lelvem-mr06.itg.ti.com (10.180.75.8) by DLEE107.ent.ti.com (157.170.170.37) 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 22:37:33 -0500 Received: from [172.24.233.20] (a0512632.dhcp.ti.com [172.24.233.20]) by lelvem-mr06.itg.ti.com (8.18.1/8.18.1) with ESMTP id 57T3bSkd1416777; Thu, 28 Aug 2025 22:37:29 -0500 Message-ID: <22ed865b-86b1-43ee-8bd4-e2e29a712287@ti.com> Date: Fri, 29 Aug 2025 09:07:28 +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 , Maxime Ripard CC: , , , , , , , , , , , , References: <20250819192113.2420396-1-s-jain1@ti.com> <20250819192113.2420396-3-s-jain1@ti.com> <20250827-illegal-splendid-coyote-aff8cc@houat> 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 15:19, Tomi Valkeinen wrote: > Hi, > > On 27/08/2025 12:27, Maxime Ripard wrote: >> On Wed, Aug 27, 2025 at 11:49:22AM +0300, Tomi Valkeinen wrote: >>> 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. >>> >>>> 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? >> >> 5% is pretty large indeed. We've been using .5% in multiple drivers and >> it proved to be pretty ok. I would advise you tu use it too. > > The 5% comes from OMAP DSS, where we had to do pixel clock with a few > dividers and multipliers. The rates were quite coarse, and we ended up > having quite a large tolerance. > > I think with tidss, we always have a PLL we control, so we should always > have very exact clocks. So I'm fine with dropping it to .5%. However, > this patch and series is about removing the a-bit-too-hardcoded VP clk > max rate code in the driver, so I would leave everything else to another > series. > >> It's not clear to me why avoiding a clk_round_rate() call is something >> worth doing though? > > Hard to say if it's worth doing, someone should make some perf tests. > However, afaik, the calls do go to the firmware, so it involves > inter-processor calls. On OMAP DSS checking the clock rates was slow, as > it involved lots of iterating with dividers and multipliers. Perhaps > it's much faster here. > >> Even caching the maximum rate you have been able to reach before is >> pretty fragile: if the PLL changes its rate, or if a sibling clock has >> set some limits on what the PLL can do, your maximum isn't relevant >> anymore. > > You're right, although afaik it should not happen with TI's SoCs. We > would be in trouble anyway if that were the case (e.g. someone starts > the camera, and suddenly we can't support 1080p anymore). > >> in other words, what's wrong with simply calling clk_round_rate() and >> checking if it's within a .5% deviation? > > This started with discussions how to replace the hardcoded max VP clock > rate (used to quickly weed out impossible rates), which in reality was > actually PLL max clock rate. We don't know the PLL max rate, and can't > query it, so this approach was taken. > >> At the very least, this should be explained in comments or the commit >> message. > > I agree. > > Swamil, can you do some perf tests with clk_round_rate()? If it's fast > (enough), it will simplify the driver. Yeah Tomi, will do a perf test. Regards, Swamil > > Tomi >