Re: [PATCH v14 17/17] drm/xe/vsec: Support survivability mode for CRI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/xe/vsec: use xe_survivability_mode_is_boot_enabled() instead
  of pmt cache
--

commit c58d99d8bdca0b8d22ebde50d5dc7b64549279f1
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support survivability mode for CRI

This commit adds a VSEC configuration for Crescent Island in survivability
mode that exposes only the PUNIT crashlog PMT capability.

> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index a35a8b6e733e5..5d4548d863761 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -492,6 +492,8 @@ struct xe_device {
>  		u32 retry_count;
>  		/** @pmt.punit_guid_cache: cache of the PUNIT GUID */
>  		u32 punit_guid_cache;
> +		/** @pmt.survivability_mode: cache of survivability mode state */
> +		bool survivability_mode;
>  	} pmt;
>  
>  	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index f27bfe8218d78..ffaed9ef43b25 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c

[ ... ]

> @@ -319,7 +339,7 @@ static bool wait_for_fw(struct xe_device *xe)
>  {
>  	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
>  
> -	if (xe->info.platform != XE_CRESCENTISLAND)
> +	if (xe->info.platform != XE_CRESCENTISLAND || xe->pmt.survivability_mode)
>  		return true;
>  
>  	while (retries--) {
> @@ -538,6 +558,8 @@ static const int vsec_platforms[] = {
>  
>  static enum xe_vsec get_platform_info(struct xe_device *xe)
>  {
> +	struct xe_survivability *survivability = &xe->survivability;
> +
>  	if (xe->info.platform > XE_CRESCENTISLAND)
>  		return XE_VSEC_UNKNOWN;
>  
> @@ -546,6 +568,9 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
>  	     xe->info.platform == XE_CRESCENTISLAND))
>  		return XE_VSEC_UNKNOWN;
>  
> +	if (survivability->mode && xe->info.platform == XE_CRESCENTISLAND)
> +		return XE_VSEC_CRI_SURVIVABILITY;
> +
>  	return vsec_platforms[xe->info.platform];
>  }
>  
> @@ -686,6 +711,13 @@ int xe_vsec_init(struct xe_device *xe)
>  		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>  		break;
>  
> +	case XE_VSEC_CRI_SURVIVABILITY:
> +		xe->pmt.base_offset = CRI_PMT_OFFSET;
> +		xe->pmt.survivability_mode = true;
> +		info->priv_data = &xe_cri_pmt_cb;
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +		break;

[Severity: Low]
Could wait_for_fw() and get_platform_info() call
xe_survivability_mode_is_boot_enabled(xe) directly instead of reaching into
xe->survivability.mode and caching it in xe->pmt.survivability_mode?  Other
subsystems (such as xe_pci.c, xe_heci_gsc.c and xe_i2c.c) already use that
helper, which would also avoid adding the duplicate boolean to struct
xe_device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110183505.694124-19-michael.j.ruhl@intel.com?part=17