[PATCH 1/2] drm/xe: Don't try to derive GT clock freq from display register on Xe2

[Lucas De Marchi]

On Thu, Sep 12, 2024 at 02:56:00PM GMT, Matt Roper wrote:
>Driver initialization has two approaches for calculating the GT clock --
>it either derives it from the crystal clock frequency as read from a GT
>register, or it derives it from the display's TIMESTAMP_OVERRIDE
>register.  The driver is informed of which approach needs to be used by
>a bit setting in the CTC_MODE register.
>
>Use of a display register to initialize the GT frequency seems a bit
>suspicious and doesn't seem to really be documented clearly in the
>bspec.  However there is a note on the CTC_MODE register indicating that
>the setting that corresponds to use of the display register (which the
>bspec simply describes as "Broadwell Divide logic") is no longer
>supported on Xe2 and beyond.  Furthermore, the TIMESTAMP_OVERRIDE
>register itself was removed in display version 20.  Rework the GT clock
>init function to only consider CTC_MODE (and then possibly use display's
>TIMESTAMP_OVERRIDE) on pre-Xe2 platforms.

good catch

>
>Bspec: 62395
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_gt_clock.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt_clock.c b/drivers/gpu/drm/xe/xe_gt_clock.c
>index cc2ae159298e..9afa5415d627 100644
>--- a/drivers/gpu/drm/xe/xe_gt_clock.c
>+++ b/drivers/gpu/drm/xe/xe_gt_clock.c
>@@ -57,27 +57,27 @@ static u32 get_crystal_clock_freq(u32 rpm_config_reg)
>
> int xe_gt_clock_init(struct xe_gt *gt)
> {
>-	u32 ctc_reg = xe_mmio_read32(&gt->mmio, CTC_MODE);
>-	u32 freq = 0;
>+	u32 c0, freq = 0;
>
> 	/* Assuming gen11+ so assert this assumption is correct */
> 	xe_gt_assert(gt, GRAPHICS_VER(gt_to_xe(gt)) >= 11);
>
>-	if (ctc_reg & CTC_SOURCE_DIVIDE_LOGIC) {
>-		freq = read_reference_ts_freq(gt);
>-	} else {
>-		u32 c0 = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
>-
>-		freq = get_crystal_clock_freq(c0);
>-
>-		/*
>-		 * Now figure out how the command stream's timestamp
>-		 * register increments from this frequency (it might
>-		 * increment only every few clock cycle).
>-		 */
>-		freq >>= 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, c0);
>+	if (GRAPHICS_VER(gt_to_xe(gt)) < 20 &&
>+	    xe_mmio_read32(&gt->mmio, CTC_MODE) & CTC_SOURCE_DIVIDE_LOGIC) {
>+		gt->info.reference_clock = read_reference_ts_freq(gt);

should we rename read_reference_ts_freq() to something like
read_display_ref_timestamp_freq()?  Moving it to xe/display would also
be something to consider to avoid future mistakes.

What happens if we don't have display (hopefully CSC_MODE is set
correctly) or display is not initialized (a.k.a. probe_display=0)?
Maybe add a xe_assert() in that function as well that we should only be
using that when we have display.

Lucas De Marchi

>+		return 0;
> 	}
>
>+	c0 = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
>+	freq = get_crystal_clock_freq(c0);
>+
>+	/*
>+	 * Now figure out how the command stream's timestamp
>+	 * register increments from this frequency (it might
>+	 * increment only every few clock cycle).
>+	 */
>+	freq >>= 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, c0);
>+
> 	gt->info.reference_clock = freq;
> 	return 0;
> }
>-- 
>2.45.2
>