Re: [PATCH v2 2/4] drm/xe/soc_remapper: Use SoC remapper herlper from VSEC code

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Dec 02, 2025 at 10:33:28AM +0530, Nilawar, Badal wrote:
>
>On 18-11-2025 02:23, Umesh Nerlige Ramappa wrote:
>>Since different drivers can use SoC remapper, modify VSEC code to
>>access SoC remapper via a helper that would synchronize such accesses.
>>
>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>---
>>v2: (Lucas)
>>- retain comment
>>- s/BITS/MASK/
>>---
>>  drivers/gpu/drm/xe/regs/xe_pmt.h               |  3 ---
>>  drivers/gpu/drm/xe/regs/xe_soc_remapper_regs.h | 13 +++++++++++++
>>  drivers/gpu/drm/xe/xe_soc_remapper.c           | 18 ++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_soc_remapper.h           |  1 +
>>  drivers/gpu/drm/xe/xe_vsec.c                   |  4 ++--
>>  5 files changed, 34 insertions(+), 5 deletions(-)
>>  create mode 100644 drivers/gpu/drm/xe/regs/xe_soc_remapper_regs.h
>>
>>diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
>>index 0f79c0714454..240d57993ea6 100644
>>--- a/drivers/gpu/drm/xe/regs/xe_pmt.h
>>+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
>>@@ -18,9 +18,6 @@
>>  #define BMG_TELEMETRY_BASE_OFFSET	0xE0000
>>  #define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
>>-#define SG_REMAP_INDEX1			XE_REG(SOC_BASE + 0x08)
>>-#define   SG_REMAP_BITS			REG_GENMASK(31, 24)
>>-
>>  #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>>  #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>>  #define BMG_G6_RESIDENCY_OFFSET		(0x538)
>>diff --git a/drivers/gpu/drm/xe/regs/xe_soc_remapper_regs.h b/drivers/gpu/drm/xe/regs/xe_soc_remapper_regs.h
>>new file mode 100644
>>index 000000000000..9edf234227a9
>>--- /dev/null
>>+++ b/drivers/gpu/drm/xe/regs/xe_soc_remapper_regs.h
>>@@ -0,0 +1,13 @@
>>+/* SPDX-License-Identifier: MIT */
>>+/*
>>+ * Copyright © 2025 Intel Corporation
>>+ */
>>+#ifndef _XE_SOC_REMAPPER_REGS_H_
>>+#define _XE_SOC_REMAPPER_REGS_H_
>>+
>>+#include "xe_regs.h"
>>+
>>+#define SG_REMAP_INDEX1			XE_REG(SOC_BASE + 0x08)
>>+#define   SG_REMAP_TELEM_MASK		REG_GENMASK(31, 24)
>>+
>>+#endif
>>diff --git a/drivers/gpu/drm/xe/xe_soc_remapper.c b/drivers/gpu/drm/xe/xe_soc_remapper.c
>>index f5a02abd6ab1..85d37a86117a 100644
>>--- a/drivers/gpu/drm/xe/xe_soc_remapper.c
>>+++ b/drivers/gpu/drm/xe/xe_soc_remapper.c
>>@@ -5,8 +5,26 @@
>>  #include <linux/spinlock.h>
>>+#include "regs/xe_soc_remapper_regs.h"
>>+#include "xe_mmio.h"
>>  #include "xe_soc_remapper.h"
>>+static void xe_soc_remapper_set_region(struct xe_device *xe, struct xe_reg reg,
>>+				       u32 mask, u32 val)
>>+{
>>+	unsigned long flags;
>>+
>>+	spin_lock_irqsave(&xe->soc_remapper.lock, flags);
>>+	xe_mmio_rmw32(xe_root_tile_mmio(xe), reg, mask, val);
>>+	spin_unlock_irqrestore(&xe->soc_remapper.lock, flags);
>>+}
>>+
>>+void xe_soc_remapper_set_telem_region(struct xe_device *xe, u32 index)
>>+{
>>+	
>I think need to check index is valid or not.

Checking valid index at this level in the call chain would be 
inefficient. Note that we would need to add code to check valid indices 
for each IP. If a new platform has other IPs added, then the checks will 
increase. Also, other IPs may use a different INDEX register for config, 
so that would increase the function footprint for all callers.

I think it's best left to the IP-specific caller to do the checks for 
valid indices for that IP. If the usage of this code grows, we can 
consider refactoring in future.

>>xe_soc_remapper_set_region(xe, SG_REMAP_INDEX1, SG_REMAP_TELEM_MASK,
>>+				   REG_FIELD_PREP(SG_REMAP_TELEM_MASK, index));
>>+}

>
>In next patch  we have created wrapper for SC.  Instead of introducing 
>separate wrappers for each SoC remapper, could we maintain a single 
>function, such as:
>
>int xe_soc_remapper_set_region(struct xe_device *xe, enum 
>soc_remapper_id id, u32 index)
>{
>    switch (id) {
>        case TELEM:
>                 if (xe->info.has_soc_remapper_telem)
>                        return -ENOTSUPP;
>            // Validate index
>            reg = SG_REMAP_INDEX1;
>            // Calculate mask, val
>            break;
>
>        case SC:
>             if (xe->info.has_soc_remapper_sc)
>                return -ENOTSUPP;
>            // Validate index
>            reg = SG_REMAP_INDEX1;
>            // Calculate mask, val
>            break;
>
>        default:
>            return -EINVAL;
>    }
>
>    spin_lock_irqsave(&xe->soc_remapper.lock, flags);
>    xe_mmio_rmw32(xe_root_tile_mmio(xe), reg, mask, val);
>    spin_unlock_irqrestore(&xe->soc_remapper.lock, flags);
>    return 0;
>}

Same logic as above. We should avoid conditional checks at the lowest 
layers and rather have those checks at the topmost layers of callers.

Thanks,
Umesh

>
>Thanks,
>Badal
>
>>+
>>  int xe_soc_remapper_init(struct xe_device *xe)
>>  {
>>  	spin_lock_init(&xe->soc_remapper.lock);
>>diff --git a/drivers/gpu/drm/xe/xe_soc_remapper.h b/drivers/gpu/drm/xe/xe_soc_remapper.h
>>index 3cfd44f1fd74..75431b94e66a 100644
>>--- a/drivers/gpu/drm/xe/xe_soc_remapper.h
>>+++ b/drivers/gpu/drm/xe/xe_soc_remapper.h
>>@@ -11,5 +11,6 @@
>>  #include "xe_device_types.h"
>>  int xe_soc_remapper_init(struct xe_device *xe);
>>+void xe_soc_remapper_set_telem_region(struct xe_device *xe, u32 index);
>>  #endif
>>diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>>index 8f23a27871b6..3e217fb75394 100644
>>--- a/drivers/gpu/drm/xe/xe_vsec.c
>>+++ b/drivers/gpu/drm/xe/xe_vsec.c
>>@@ -16,6 +16,7 @@
>>  #include "xe_mmio.h"
>>  #include "xe_platform_types.h"
>>  #include "xe_pm.h"
>>+#include "xe_soc_remapper.h"
>>  #include "xe_vsec.h"
>>  #include "regs/xe_pmt.h"
>>@@ -163,8 +164,7 @@ int xe_pmt_telem_read(struct pci_dev *pdev, u32 guid, u64 *data, loff_t user_off
>>  		return -ENODATA;
>>  	/* set SoC re-mapper index register based on GUID memory region */
>>-	xe_mmio_rmw32(xe_root_tile_mmio(xe), SG_REMAP_INDEX1, SG_REMAP_BITS,
>>-		      REG_FIELD_PREP(SG_REMAP_BITS, mem_region));
>>+	xe_soc_remapper_set_telem_region(xe, mem_region);
>>  	memcpy_fromio(data, telem_addr, count);
>>  	xe_pm_runtime_put(xe);