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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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.
> 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;
}

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);