Re: [PATCH] drm/xe/pf: Access VF's register using dedicated MMIO view

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/25/2025 12:27 AM, Matt Roper wrote:
> On Fri, Oct 24, 2025 at 10:58:25PM +0200, Michal Wajdeczko wrote:
>> Instead of creating ad-hoc new register definitions with altered
>> register addresses to mimic the VF's access to these registers,
>> prepare new MMIO instance per required VF, with shifted internal
>> location of the register map.  This will allow to use unmodified
>> register definitions in all calls to xe_mmio() functions.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf.c | 36 +++++++----------------------
>>  drivers/gpu/drm/xe/xe_mmio.c        | 29 +++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_mmio.h        |  4 ++++
>>  3 files changed, 41 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf.c
>> index c4dda87b47cc..0714c758b9c1 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf.c
>> @@ -158,39 +158,19 @@ void xe_gt_sriov_pf_init_hw(struct xe_gt *gt)
>>  	xe_gt_sriov_pf_service_update(gt);
>>  }
>>  
>> -static u32 pf_get_vf_regs_stride(struct xe_device *xe)
>> -{
>> -	return GRAPHICS_VERx100(xe) > 1200 ? 0x400 : 0x1000;
>> -}
>> -
>> -static struct xe_reg xe_reg_vf_to_pf(struct xe_reg vf_reg, unsigned int vfid, u32 stride)
>> -{
>> -	struct xe_reg pf_reg = vf_reg;
>> -
>> -	pf_reg.vf = 0;
>> -	pf_reg.addr += stride * vfid;
>> -
>> -	return pf_reg;
>> -}
>> -
>>  static void pf_clear_vf_scratch_regs(struct xe_gt *gt, unsigned int vfid)
>>  {
>> -	u32 stride = pf_get_vf_regs_stride(gt_to_xe(gt));
>> -	struct xe_reg scratch;
>> -	int n, count;
>> +	struct xe_mmio mmio;
>> +	int n;
>> +
>> +	xe_mmio_init_vf_view(&mmio, &gt->mmio, vfid);
>>  
>>  	if (xe_gt_is_media_type(gt)) {
>> -		count = MED_VF_SW_FLAG_COUNT;
>> -		for (n = 0; n < count; n++) {
>> -			scratch = xe_reg_vf_to_pf(MED_VF_SW_FLAG(n), vfid, stride);
>> -			xe_mmio_write32(&gt->mmio, scratch, 0);
>> -		}
>> +		for (n = 0; n < MED_VF_SW_FLAG_COUNT; n++)
>> +			xe_mmio_write32(&mmio, MED_VF_SW_FLAG(n), 0);
>>  	} else {
>> -		count = VF_SW_FLAG_COUNT;
>> -		for (n = 0; n < count; n++) {
>> -			scratch = xe_reg_vf_to_pf(VF_SW_FLAG(n), vfid, stride);
>> -			xe_mmio_write32(&gt->mmio, scratch, 0);
>> -		}
>> +		for (n = 0; n < VF_SW_FLAG_COUNT; n++)
>> +			xe_mmio_write32(&mmio, VF_SW_FLAG(n), 0);
>>  	}
>>  }
>>  
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> index ef6f3ea573a2..350dca1f0925 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -379,3 +379,32 @@ int xe_mmio_wait32_not(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u32 va
>>  {
>>  	return __xe_mmio_wait32(mmio, reg, mask, val, timeout_us, out_val, atomic, false);
>>  }
>> +
>> +#ifdef CONFIG_PCI_IOV
>> +static size_t vf_regs_stride(struct xe_device *xe)
>> +{
>> +	return GRAPHICS_VERx100(xe) > 1200 ? 0x400 : 0x1000;
>> +}
>> +
>> +/**
>> + * xe_mmio_init_vf_view() - Initialize an MMIO instance for accesses like the VF
>> + * @mmio: the target &xe_mmio to initialize as VF's view
>> + * @base: the source &xe_mmio to initialize from
>> + * @vfid: the VF identifier
>> + */
>> +void xe_mmio_init_vf_view(struct xe_mmio *mmio, const struct xe_mmio *base, unsigned int vfid)
>> +{
>> +	struct xe_tile *tile = base->tile;
>> +	struct xe_device *xe = tile->xe;
>> +	size_t offset = vf_regs_stride(xe) * vfid;
>> +
>> +	xe_assert(xe, IS_SRIOV_PF(xe));
>> +	xe_assert(xe, vfid);
>> +	xe_assert(xe, !base->sriov_vf_gt);
>> +	xe_assert(xe, base->regs_size > offset);
>> +
>> +	*mmio = *base;
>> +	mmio->regs += offset;
>> +	mmio->regs_size -= offset;
> 
> I feel like it might be more intuitive to use adj_offset / adj_limit

I couldn't just update adj_offset as it will not be used on the main
GT because it is 0 there and we have this condition:

	if (addr < mmio->adj_limit)
		addr += mmio->adj_offset;

since I didn't want to touch adj_limit here, it was easier to move regs ;)

but if it is OK to change adj_limit in the xe_gt_mmio_init():

./xe_gt.c-686-
./xe_gt.c-687-  if (gt->info.type == XE_GT_TYPE_MEDIA) {
./xe_gt.c-688-          gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
./xe_gt.c:689:          gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
./xe_gt.c-690-  } else {
./xe_gt.c-691-          gt->mmio.adj_offset = 0;
./xe_gt.c:692: -        gt->mmio.adj_limit = 0;
./xe_gt.c:692: +        gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
./xe_gt.c-693-  }

then I can do:

 +	*mmio = *base;
 +	mmio->adj_offset += offset;

> instead to select the appropriate per-VF instance of the registers and
> leave regs/regs_size unchanged, representing the underlying iomap
> itself.  I see this selection of a specific VF's instance of these
> registers as being similar conceptually to selecting which instance
> (primary or media) of a GT's GSI registers we access.  But this approach
> works just as well, so I'm fine if you want to keep it like this.
> Either way,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> On a slightly related note, I do still have plans to add optional
> allow/deny ranges to xe_mmio structures that debug builds can do runtime
> checks against to ensure an xe_mmio isn't accidentally being used to
> access register ranges it wasn't intended for.  Probably not super
> useful for what you're doing here since the lifetime of your xe_mmio is
> a single function, but I think there are a lot more uses of xe_mmio
> coming in the future, and that will help us catch mistakes in CI.  I'll
> try to get back to working on that soon.
> 
> 
> Matt
> 
>> +}
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
>> index c151ba569003..15362789ab99 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.h
>> +++ b/drivers/gpu/drm/xe/xe_mmio.h
>> @@ -42,4 +42,8 @@ static inline struct xe_mmio *xe_root_tile_mmio(struct xe_device *xe)
>>  	return &xe->tiles[0].mmio;
>>  }
>>  
>> +#ifdef CONFIG_PCI_IOV
>> +void xe_mmio_init_vf_view(struct xe_mmio *mmio, const struct xe_mmio *base, unsigned int vfid);
>> +#endif
>> +
>>  #endif
>> -- 
>> 2.47.1
>>
>