[PATCH 4/6] drm/xe/vf: Provide early access to GMDID register

[Michal Wajdeczko]

On 23.05.2024 23:17, Matt Roper wrote:
> On Thu, May 23, 2024 at 02:08:16PM -0700, Matt Roper wrote:
>> On Thu, May 23, 2024 at 09:22:38PM +0200, Michal Wajdeczko wrote:
>>> VFs do not have direct access to the GMDID register and must obtain
>>> its value from the GuC. Since we need GMDID value very early in the
>>> driver probe flow, before we even start the full setup of GT and GuC
>>> data structures, we must do some early initializations ourselves.
>>>
>>> Additionally, since we also need GMDID for the media GT, which isn't
>>> created yet, temporarly tweak the root GT type into MEDIA to allow
>>> communication with the correct GuC, as only it can provide the value
>>> of the media GMDID register.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_pci.c | 47 ++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 44 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index ece410289f6c..eefac633dbc1 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -20,6 +20,8 @@
>>>  #include "xe_device.h"
>>>  #include "xe_drv.h"
>>>  #include "xe_gt.h"
>>> +#include "xe_gt_sriov_vf.h"
>>> +#include "xe_guc.h"
>>>  #include "xe_macros.h"
>>>  #include "xe_mmio.h"
>>>  #include "xe_module.h"
>>> @@ -471,10 +473,49 @@ static void read_gmdid(struct xe_device *xe, enum xe_gmdid_type type, u32 *ver,
>>>  
>>>  	KUNIT_STATIC_STUB_REDIRECT(read_gmdid, xe, type, ver, revid);
>>>  
>>> -	if (type == GMDID_MEDIA)
>>> -		gmdid_reg.addr += MEDIA_GT_GSI_OFFSET;
>>> +	if (IS_SRIOV_VF(xe)) {
>>> +		/*
>>> +		 * To get the value of the GMDID register, VFs must obtain it
>>> +		 * from the GuC using MMIO communication.
>>> +		 *
>>> +		 * Note that at this point the xe_gt is not fully uninitialized
>>> +		 * and only basic access to MMIO registers is possible. To use
>>> +		 * our existing GuC communication functions we must perform at
>>> +		 * least basic xe_gt and xe_guc initialization.
>>> +		 *
>>> +		 * Since to obtain the value of GMDID_MEDIA we need to use the
>>> +		 * media GuC, temporarly tweak the gt type.
>>> +		 */
>>> +		xe_gt_assert(gt, gt->info.type == XE_GT_TYPE_UNINITIALIZED);
>>> +
>>> +		if (type == GMDID_MEDIA) {
>>> +			gt->info.id = 1;
>>> +			gt->info.type = XE_GT_TYPE_MEDIA;
>>> +		} else {
>>> +			gt->info.id = 0;
>>> +			gt->info.type = XE_GT_TYPE_MAIN;
>>> +		}
>>> +
>>> +		xe_guc_comm_init_early(&gt->uc.guc);
>>> +
>>> +		/* don't bother with GMDID if failed to negotiate the GuC ABI */
>>> +		val = xe_gt_sriov_vf_bootstrap(gt) ? 0 : xe_gt_sriov_vf_gmdid(gt);
>>> +
>>> +		/* undo */
>>> +		gt->info.id = 0;
>>> +		gt->info.type = XE_GT_TYPE_UNINITIALIZED;
>>
>> The xe_guc_comm_init_early() and xe_gt_sriov_vf_bootstrap() above also
>> make changes to the various GuC0 structures (which might be "wrong" if
>> we were reading the media's GMD_ID, but I think all of those changes
>> wind up getting overwritten later with the correct GuC0 values again
>> later when we initialize the GuC for real.  You might want to add some
>> kind of comment about that, since otherwise it looks at first glance
>> like we might be leaving GuC1 state inside the GuC0 structure here.

yep, I was relying here that all stuff will properly re-initialized
later on, so there is no need (or means) to rollback that

and yes, adding explicit comment about this is a good idea

>>
>> Otherwise,
>>
>>         Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> 
> Oh, one more question --- for non-SRIOV cases we assume that it's always
> safe to read the GMD_ID register offsets, even if the platform has
> fused-off media (like PVC did in the past).  If media is fused off, that
> register read comes back as 0x0 and we move forward recognizing that
> media just isn't available.  I don't think any of the post-MTL platforms
> we have at the moment (i.e., platforms where GMD_ID exists) ever have
> fused off media yet, but I imagine that situation will likely come back
> at some point in the future.
> 
> With the SRIOV-based flows here, are we going to get any GuC errors
> while blindly attempting to read the media GT's GMD_ID if media (and
> thus the media GuC) isn't actually present?

I'm afraid that today VF has no choice but assume that media GuC always
exists and VF can follow with GMDID query

but if there will be real SRIOV cases with fused off media GT then I
guess we will have to ask for some new KLV to be exposed by the root GuC
that VF could use (something similar to existing TILE_MASK)

> 
> 
> Matt
> 
>>
>>> +	} else {
>>> +		/*
>>> +		 * We need to apply the GSI offset explicitly here as at this
>>> +		 * point the xe_gt is not fully uninitialized and only basic
>>> +		 * access to MMIO registers is possible.
>>> +		 */
>>> +		if (type == GMDID_MEDIA)
>>> +			gmdid_reg.addr += MEDIA_GT_GSI_OFFSET;
>>> +
>>> +		val = xe_mmio_read32(gt, gmdid_reg);
>>> +	}
>>>  
>>> -	val = xe_mmio_read32(gt, gmdid_reg);
>>>  	*ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val) * 100 + REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
>>>  	*revid = REG_FIELD_GET(GMD_ID_REVID, val);
>>>  }
>>> -- 
>>> 2.43.0
>>>
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation
>