diff mbox series

[3/4] drm/xe: Drop usage of TIMESTAMP_OVERRIDE

Message ID 20250221003843.443559-9-matthew.d.roper@intel.com (mailing list archive)
State New
Headers show
Series Stop accessing display TIMESTAMP_OVERRIDE in GT code | expand

Commit Message

Matt Roper Feb. 21, 2025, 12:38 a.m. UTC 
On pre-Xe2 platforms, one of the approaches to initialize the GT command
streamer frequency is to use the display reference clock.  That's no
longer relevant from Xe2 onward (i.e., all of the platforms where Xe is
officially supported).  Furthermore, use of TIMESTAMP_OVERRIDE to obtain
the display reference clock is unnecessarily roundabout; the display
driver already has a more reliable approach to obtain this value.  Let's
use the display driver's existing logic to determine the proper display
reference clock in the rare cases where the hardware indicates we should
do this.

The one problem that arises here is if the Xe driver is run on an
unsupported platform (i.e., pre-Xe2), CONFIG_DRM_XE_DISPLAY disabled
(meaning we're not expecting to touch display hardware at all), and the
platform has the rare CTC_MODE[1] setting indicating that display
reference clock should be used as the GT CS refclk.  If all of these
conditions are true, the hardcoded 38.4 MHz value will still be correct
for DG2 and MTL platforms, but there's a chance that we might not have
the right value on the older Xe_LP platforms if we're trying not to
touch the display hardware at all.

Bspec: 62395
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/display/xe_display.c |  6 ++++++
 drivers/gpu/drm/xe/display/xe_display.h |  4 ++++
 drivers/gpu/drm/xe/xe_gt_clock.c        | 28 ++++++++-----------------
 3 files changed, 19 insertions(+), 19 deletions(-)

Comments

Lucas De Marchi Feb. 21, 2025, 3:35 a.m. UTC | #1 
On Thu, Feb 20, 2025 at 04:38:47PM -0800, Matt Roper wrote:
>On pre-Xe2 platforms, one of the approaches to initialize the GT command
>streamer frequency is to use the display reference clock.  That's no
>longer relevant from Xe2 onward (i.e., all of the platforms where Xe is
>officially supported).  Furthermore, use of TIMESTAMP_OVERRIDE to obtain
>the display reference clock is unnecessarily roundabout; the display
>driver already has a more reliable approach to obtain this value.  Let's
>use the display driver's existing logic to determine the proper display
>reference clock in the rare cases where the hardware indicates we should
>do this.
>
>The one problem that arises here is if the Xe driver is run on an
>unsupported platform (i.e., pre-Xe2), CONFIG_DRM_XE_DISPLAY disabled
>(meaning we're not expecting to touch display hardware at all), and the
>platform has the rare CTC_MODE[1] setting indicating that display
>reference clock should be used as the GT CS refclk.  If all of these
>conditions are true, the hardcoded 38.4 MHz value will still be correct
>for DG2 and MTL platforms, but there's a chance that we might not have
>the right value on the older Xe_LP platforms if we're trying not to
>touch the display hardware at all.
>
>Bspec: 62395
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/display/xe_display.c |  6 ++++++
> drivers/gpu/drm/xe/display/xe_display.h |  4 ++++
> drivers/gpu/drm/xe/xe_gt_clock.c        | 28 ++++++++-----------------
> 3 files changed, 19 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
>index 02a413a07382..e35d58079f0d 100644
>--- a/drivers/gpu/drm/xe/display/xe_display.c
>+++ b/drivers/gpu/drm/xe/display/xe_display.c
>@@ -17,6 +17,7 @@
> #include "intel_acpi.h"
> #include "intel_audio.h"
> #include "intel_bw.h"
>+#include "intel_cdclk.h"
> #include "intel_display.h"
> #include "intel_display_driver.h"
> #include "intel_display_irq.h"
>@@ -548,3 +549,8 @@ int xe_display_probe(struct xe_device *xe)
> 	unset_display_features(xe);
> 	return 0;
> }
>+
>+u32 xe_display_get_refclk(struct xe_device *xe)
>+{
>+	return intel_display_get_refclk(&xe->display);
>+}
>diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
>index 685dc74402fb..b918f5d6b53a 100644
>--- a/drivers/gpu/drm/xe/display/xe_display.h
>+++ b/drivers/gpu/drm/xe/display/xe_display.h
>@@ -41,6 +41,8 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe);
> void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
> void xe_display_pm_runtime_resume(struct xe_device *xe);
>
>+u32 xe_display_get_refclk(struct xe_device *xe);
>+
> #else
>
> static inline int xe_display_driver_probe_defer(struct pci_dev *pdev) { return 0; }
>@@ -72,5 +74,7 @@ static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
> static inline void xe_display_pm_runtime_suspend_late(struct xe_device *xe) {}
> static inline void xe_display_pm_runtime_resume(struct xe_device *xe) {}
>
>+static u32 xe_display_get_refclk(struct xe_device *xe) { return 38400; }
>+
> #endif /* CONFIG_DRM_XE_DISPLAY */
> #endif /* _XE_DISPLAY_H_ */
>diff --git a/drivers/gpu/drm/xe/xe_gt_clock.c b/drivers/gpu/drm/xe/xe_gt_clock.c
>index cc2ae159298e..b61f944a7b03 100644
>--- a/drivers/gpu/drm/xe/xe_gt_clock.c
>+++ b/drivers/gpu/drm/xe/xe_gt_clock.c
>@@ -7,6 +7,7 @@
>
> #include "xe_gt_clock.h"
>
>+#include "display/xe_display.h"
> #include "regs/xe_gt_regs.h"
> #include "regs/xe_regs.h"
> #include "xe_assert.h"
>@@ -15,22 +16,6 @@
> #include "xe_macros.h"
> #include "xe_mmio.h"
>
>-static u32 read_reference_ts_freq(struct xe_gt *gt)
>-{
>-	u32 ts_override = xe_mmio_read32(&gt->mmio, TIMESTAMP_OVERRIDE);
>-	u32 base_freq, frac_freq;
>-
>-	base_freq = REG_FIELD_GET(TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK,
>-				  ts_override) + 1;
>-	base_freq *= 1000000;
>-
>-	frac_freq = REG_FIELD_GET(TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK,
>-				  ts_override);
>-	frac_freq = 1000000 / (frac_freq + 1);
>-
>-	return base_freq + frac_freq;
>-}
>-
> static u32 get_crystal_clock_freq(u32 rpm_config_reg)
> {
> 	const u32 f19_2_mhz = 19200000;
>@@ -57,14 +42,19 @@ static u32 get_crystal_clock_freq(u32 rpm_config_reg)
>
> int xe_gt_clock_init(struct xe_gt *gt)
> {
>+	struct xe_device *xe = gt_to_xe(gt);
> 	u32 ctc_reg = xe_mmio_read32(&gt->mmio, CTC_MODE);
> 	u32 freq = 0;
>
> 	/* Assuming gen11+ so assert this assumption is correct */
>-	xe_gt_assert(gt, GRAPHICS_VER(gt_to_xe(gt)) >= 11);
>+	xe_gt_assert(gt, GRAPHICS_VER(xe) >= 11);

this was probably missed in cleanups... we don't support anything lower
than 12. Can update this while at it? Or simply remove...

>
>-	if (ctc_reg & CTC_SOURCE_DIVIDE_LOGIC) {
>-		freq = read_reference_ts_freq(gt);
>+	/*
>+	 * Use of the display reference clock to determine GT CS frequency
>+	 * is only relevant to pre-Xe2 platforms.
>+	 */
>+	if (GRAPHICS_VER(xe) < 20 && ctc_reg & CTC_SOURCE_DIVIDE_LOGIC) {
>+		freq = xe_display_get_refclk(xe);

can we go a step further and completely drop using the display refclck?
For any platform we currently use (officially or unofficially, so at
most gen12), do we actually have any where CTC_SOURCE_DIVIDE_LOGIC is
actually set?  What would happen if we had display compiled out?

Lucas De Marchi

> 	} else {
> 		u32 c0 = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
>
>-- 
>2.48.1
>
kernel test robot Feb. 21, 2025, 4:25 a.m. UTC | #2 
Hi Matt,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20250220]
[cannot apply to drm-xe/drm-xe-next v6.14-rc3 v6.14-rc2 v6.14-rc1 linus/master v6.14-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matt-Roper/drm-i915-display-Make-refclk-fetching-logic-reusable/20250221-084058
base:   next-20250220
patch link:    https://lore.kernel.org/r/20250221003843.443559-9-matthew.d.roper%40intel.com
patch subject: [PATCH 3/4] drm/xe: Drop usage of TIMESTAMP_OVERRIDE
config: i386-buildonly-randconfig-003-20250221 (https://download.01.org/0day-ci/archive/20250221/202502211245.wEClyBRb-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250221/202502211245.wEClyBRb-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502211245.wEClyBRb-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/xe/xe_irq.c:12:
>> drivers/gpu/drm/xe/display/xe_display.h:77:12: warning: 'xe_display_get_refclk' defined but not used [-Wunused-function]
      77 | static u32 xe_display_get_refclk(struct xe_device *xe) { return 38400; }
         |            ^~~~~~~~~~~~~~~~~~~~~


vim +/xe_display_get_refclk +77 drivers/gpu/drm/xe/display/xe_display.h

    76	
  > 77	static u32 xe_display_get_refclk(struct xe_device *xe) { return 38400; }
    78
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 02a413a07382..e35d58079f0d 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -17,6 +17,7 @@ 
 #include "intel_acpi.h"
 #include "intel_audio.h"
 #include "intel_bw.h"
+#include "intel_cdclk.h"
 #include "intel_display.h"
 #include "intel_display_driver.h"
 #include "intel_display_irq.h"
@@ -548,3 +549,8 @@  int xe_display_probe(struct xe_device *xe)
 	unset_display_features(xe);
 	return 0;
 }
+
+u32 xe_display_get_refclk(struct xe_device *xe)
+{
+	return intel_display_get_refclk(&xe->display);
+}
diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
index 685dc74402fb..b918f5d6b53a 100644
--- a/drivers/gpu/drm/xe/display/xe_display.h
+++ b/drivers/gpu/drm/xe/display/xe_display.h
@@ -41,6 +41,8 @@  void xe_display_pm_runtime_suspend(struct xe_device *xe);
 void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
 void xe_display_pm_runtime_resume(struct xe_device *xe);
 
+u32 xe_display_get_refclk(struct xe_device *xe);
+
 #else
 
 static inline int xe_display_driver_probe_defer(struct pci_dev *pdev) { return 0; }
@@ -72,5 +74,7 @@  static inline void xe_display_pm_runtime_suspend(struct xe_device *xe) {}
 static inline void xe_display_pm_runtime_suspend_late(struct xe_device *xe) {}
 static inline void xe_display_pm_runtime_resume(struct xe_device *xe) {}
 
+static u32 xe_display_get_refclk(struct xe_device *xe) { return 38400; }
+
 #endif /* CONFIG_DRM_XE_DISPLAY */
 #endif /* _XE_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/xe/xe_gt_clock.c b/drivers/gpu/drm/xe/xe_gt_clock.c
index cc2ae159298e..b61f944a7b03 100644
--- a/drivers/gpu/drm/xe/xe_gt_clock.c
+++ b/drivers/gpu/drm/xe/xe_gt_clock.c
@@ -7,6 +7,7 @@ 
 
 #include "xe_gt_clock.h"
 
+#include "display/xe_display.h"
 #include "regs/xe_gt_regs.h"
 #include "regs/xe_regs.h"
 #include "xe_assert.h"
@@ -15,22 +16,6 @@ 
 #include "xe_macros.h"
 #include "xe_mmio.h"
 
-static u32 read_reference_ts_freq(struct xe_gt *gt)
-{
-	u32 ts_override = xe_mmio_read32(&gt->mmio, TIMESTAMP_OVERRIDE);
-	u32 base_freq, frac_freq;
-
-	base_freq = REG_FIELD_GET(TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK,
-				  ts_override) + 1;
-	base_freq *= 1000000;
-
-	frac_freq = REG_FIELD_GET(TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK,
-				  ts_override);
-	frac_freq = 1000000 / (frac_freq + 1);
-
-	return base_freq + frac_freq;
-}
-
 static u32 get_crystal_clock_freq(u32 rpm_config_reg)
 {
 	const u32 f19_2_mhz = 19200000;
@@ -57,14 +42,19 @@  static u32 get_crystal_clock_freq(u32 rpm_config_reg)
 
 int xe_gt_clock_init(struct xe_gt *gt)
 {
+	struct xe_device *xe = gt_to_xe(gt);
 	u32 ctc_reg = xe_mmio_read32(&gt->mmio, CTC_MODE);
 	u32 freq = 0;
 
 	/* Assuming gen11+ so assert this assumption is correct */
-	xe_gt_assert(gt, GRAPHICS_VER(gt_to_xe(gt)) >= 11);
+	xe_gt_assert(gt, GRAPHICS_VER(xe) >= 11);
 
-	if (ctc_reg & CTC_SOURCE_DIVIDE_LOGIC) {
-		freq = read_reference_ts_freq(gt);
+	/*
+	 * Use of the display reference clock to determine GT CS frequency
+	 * is only relevant to pre-Xe2 platforms.
+	 */
+	if (GRAPHICS_VER(xe) < 20 && ctc_reg & CTC_SOURCE_DIVIDE_LOGIC) {
+		freq = xe_display_get_refclk(xe);
 	} else {
 		u32 c0 = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);