Re: [PATCH] drm/xe: Store GMD_ID subunit during GT init

[Gustavo Sousa <gustavo.sousa@intel.com>]

Pallavi Mishra <pallavi.mishra@intel.com> writes:

> Read GMD_ID during GT forcewake initialization and store both the
> full GMD_ID value and the subip field in gt->info.
> Keeping the field in gt->info provides a common source for
> future users and avoids additional GMD_ID reads.
>
> Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt.c       | 20 ++++++++++++++------
>  drivers/gpu/drm/xe/xe_gt_types.h |  2 ++
>  drivers/gpu/drm/xe/xe_guc.c      | 12 +++++-------
>  3 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 60d35ada2853..39ae7105aaa0 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -564,6 +564,18 @@ static void dump_pat_on_error(struct xe_gt *gt)
>  	xe_pat_dump(gt, &p);
>  }
>  
> +/*
> + * Store GMD_ID and sub-IP before GuC upload. Main GAMCTRL consumes the stored
> + * subunit field during upload.
> + */
> +static void xe_gt_read_gmdid(struct xe_gt *gt)
> +{
> +	u32 gmdid = xe_mmio_read32(&gt->mmio, GMD_ID);
> +
> +	gt->info.gmdid = gmdid;
> +	gt->info.subip = REG_FIELD_GET(GMD_ID_SUBIP_FLAG_MASK, gmdid);

Why do we want to store a subip field? Shouldn't we simply use
REG_FIELD_GET(GMD_ID_SUBIP_FLAG_MASK, gt->info.gmdid) when needed
instead of adding another field to the struct?

Furthermore, there is some exceptional use case for the "subip" bits on
media version 35, but for other IPs they are tagged as reserved.  As
such, I think we should limit ourselves to treat those bits as "subip"
info only where it makes sense (i.e. the current approach inside
xe_guc_using_main_gamctrl_queues()).

--
Gustavo Sousa

> +}
> +
>  static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>  {
>  	int err;
> @@ -572,6 +584,8 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>  	if (!fw_ref.domains)
>  		return -ETIMEDOUT;
>  
> +	xe_gt_read_gmdid(gt);
> +
>  	err = xe_uc_init(&gt->uc);
>  	if (err)
>  		return err;
> @@ -610,12 +624,6 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>  	if (err)
>  		return err;
>  
> -	/*
> -	 * Stash hardware-reported version.  Since this register does not exist
> -	 * on pre-MTL platforms, reading it there will (correctly) return 0.
> -	 */
> -	gt->info.gmdid = xe_mmio_read32(&gt->mmio, GMD_ID);
> -
>  	if (GRAPHICS_VER(gt_to_xe(gt)) >= 20 && xe_gt_is_main_type(gt))
>  		gt->info.has_wmtp_disabled = !!(xe_mmio_read32(&gt->mmio, XEHP_FUSE4) &
>  			CFEG_WMTP_DISABLE);
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index c00a7c171f80..c67fb0d512b0 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -153,6 +153,8 @@ struct xe_gt {
>  		u64 engine_mask;
>  		/** @info.gmdid: raw GMD_ID value from hardware */
>  		u32 gmdid;
> +		/** @info.subip: GMD_ID subunit field */
> +		u8 subip;
>  		/**
>  		 * @info.multi_queue_engine_class_mask: Bitmask of engine classes with
>  		 * multi queue support enabled.
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index b1f4e132fa0c..37d72ce1d589 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -1864,18 +1864,16 @@ bool xe_guc_using_main_gamctrl_queues(struct xe_guc *guc)
>  	/*
>  	 * For Xe3p media gt (35), the GuC and the CS subunits may be still Xe3
>  	 * that lacks the Main GAMCTRL support. Reserved bits from the GMD_ID
> -	 * inform the IP version of the subunits.
> +	 * inform the IP version of the subunits. That field is captured once
> +	 * at GT init and stored in gt->info.subip.
>  	 */
>  	if (xe_gt_is_media_type(gt) && MEDIA_VER(gt_to_xe(gt)) == 35) {
> -		u32 val = xe_mmio_read32(&gt->mmio, GMD_ID);
> -		u32 subip = REG_FIELD_GET(GMD_ID_SUBIP_FLAG_MASK, val);
> -
> -		if (!subip)
> +		if (!gt->info.subip)
>  			return true;
>  
> -		xe_gt_WARN(gt, subip != 1,
> +		xe_gt_WARN(gt, gt->info.subip != 1,
>  			   "GMD_ID has unknown value in the SUBIP_FLAG field - 0x%x\n",
> -			   subip);
> +			   gt->info.subip);
>  
>  		return false;
>  	}
> -- 
> 2.43.0