[Intel-xe] [PATCH 2/4] drm/xe: Use dedicated function to read engine fuse registers

[Michal Wajdeczko]

On 30.11.2023 23:56, Matt Roper wrote:
> On Wed, Nov 29, 2023 at 09:02:51AM -0600, Lucas De Marchi wrote:
>> On Wed, Nov 15, 2023 at 03:38:36PM +0100, Michal Wajdeczko wrote:
>>> These registers are not directly exposed to VFs, use function that
>>> will provide required data also when running as a VF.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> ---
>>> drivers/gpu/drm/xe/xe_hw_engine.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
>>> index e831e63c5e48..1cf26c3c15c5 100644
>>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>>> @@ -496,7 +496,7 @@ static void read_media_fuses(struct xe_gt *gt)
>>>
>>> 	xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
>>>
>>> -	media_fuse = xe_mmio_read32(gt, GT_VEBOX_VDBOX_DISABLE);
>>> +	media_fuse = xe_mmio_fuse_read32(gt, GT_VEBOX_VDBOX_DISABLE);
>>
>> I'm not sure about this approach as it's not really clear which register
>> should or should not use this variant. For someone reading the code it
>> will be very confusing. We already have the mcr variant... what if one
>> of those registers is also MCR?
>>
>> As an alternative, we could annotate the registers on their definition
>> like we do for masked registers, and just tread them differently on the
>> normal functions when in VF mode. See the first struct inside struct
>> xe_reg. On register definition we would pass it as XE_REG_OPTION_XXXXXX
>>
>> +Matt Roper
>>
>> I'm unsure what's the best approach here without seeing the VF
>> implementation. Just want to kickstart a discussion.

something like here [1] where VF will receive set of register values
from PF that we will use if VF asks for such register

[1]
https://github.com/intel/linux-intel-lts/blob/5.10/yocto/drivers/gpu/drm/i915/intel_uncore.c#L1758

> 
> I don't think fuse registers are something that would ever be multicast
> since they're "higher level" in the hardware design and not down inside
> of specific hardware subunits.
> 
> I do like the idea of XE_REG_OPTION_XXXXXX since once you mark the
> register that way once, any future code that accesses the same register
> will automatically do the right thing, even if the person adding new
> code isn't aware of the SRIOV-specific details.

I've started tagging registers with

	/**
	 * XE_REG_OPTION_VF - Register is "VF" accessible.
	 *
	 * The register with this option could be accessed by
	 * the VF directly over MMIO BAR.
	 *
	 * To be used with XE_REG() and XE_REG_INITIALIZER().
	 */
	#define XE_REG_OPTION_VF		.vf = 1

but then we must hope that HW team will not change their minds for
existing register offsets in future platforms - now it should be OK.

For registers that VFs need but can't access directly (like fuses & co)
I wanted to add other flag:

	/**
	 * XE_REG_OPTION_VF_RUNTIME - Register is "VF" accessible.
	 *
	 * The register with this option could be accessed by
	 * the VF but only over dedicated mechanism.
	 * Implies XE_REG_OPTION_VF.
	 *
	 * To be used with XE_REG() and XE_REG_INITIALIZER().
	 */
	#define XE_REG_OPTION_VF_RUNTIME	.vf = 1, .vf_runtime = 1

but then xe_reg grows over single u32 - can we make addr:27 bit?

On related note, to actually benefit from this new xe_reg flags, extra
code will be added to generic xe_mmio_read32|write32 functions, likely
making them non-inline, is this OK?

Michal

> 
> 
> Matt
> 
>>
>> Lucas De Marchi
>>
>>
>>>
>>> 	/*
>>> 	 * Pre-Xe_HP platforms had register bits representing absent engines,
>>> @@ -541,7 +541,7 @@ static void read_copy_fuses(struct xe_gt *gt)
>>>
>>> 	xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
>>>
>>> -	bcs_mask = xe_mmio_read32(gt, MIRROR_FUSE3);
>>> +	bcs_mask = xe_mmio_fuse_read32(gt, MIRROR_FUSE3);
>>> 	bcs_mask = REG_FIELD_GET(MEML3_EN_MASK, bcs_mask);
>>>
>>> 	/* BCS0 is always present; only BCS1-BCS8 may be fused off */
>>> @@ -588,7 +588,7 @@ static void read_compute_fuses_from_reg(struct xe_gt *gt)
>>> 	struct xe_device *xe = gt_to_xe(gt);
>>> 	u32 ccs_mask;
>>>
>>> -	ccs_mask = xe_mmio_read32(gt, XEHP_FUSE4);
>>> +	ccs_mask = xe_mmio_fuse_read32(gt, XEHP_FUSE4);
>>> 	ccs_mask = REG_FIELD_GET(CCS_EN_MASK, ccs_mask);
>>>
>>> 	for (int i = XE_HW_ENGINE_CCS0, j = 0; i <= XE_HW_ENGINE_CCS3; ++i, ++j) {
>>> -- 
>>> 2.25.1
>>>
>