Re: [Intel-gfx] [PATCH] drm/i915/wopcm: Handle pre-programmed WOPCM registers

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jan 20, 2022 at 06:13:47PM +0000, Teres Alexis, Alan Previn wrote:
> Just one nit below, (assuming that igt CI failure isnt related - kms flip not completing)
> Reviewed-by Alan Previn <alan.previn.teres.alexis@intel.com>
> 
> -----Original Message-----
> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> 
> Sent: Friday, January 14, 2022 11:33 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com>; Summers, Stuart <stuart.summers@intel.com>; Harrison, John C <john.c.harrison@intel.com>; Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>
> Subject: [PATCH] drm/i915/wopcm: Handle pre-programmed WOPCM registers
> 
> Starting from DG2, some of the programming previously done by i915 and the GuC has been moved to the GSC and the relevant registers are no longer writable by either CPU or GuC. This is also referred to as GuC deprivilege.
> On the i915 side, this affects the WOPCM registers: these are no longer programmed by the driver and we do instead expect to find them already set. This can lead to verification failures because in i915 we cheat a bit with the WOPCM size defines, to keep the code common across platforms, by sometimes using a smaller WOPCM size that the actual HW support (which isn't a problem because the extra size is not needed if the FW fits in the smaller chunk), while the pre-programmed values can use the actual size.
> Given tha the new programming entity is trusted, relax the amount of the checks done on the pre-programmed values by not limiting the max programmed size. In the extremely unlikely scenario that the registers have been misprogrammed, we will still fail later at DMA time.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: John Harrison <john.c.harrison@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_reg.h |  3 ++
>  drivers/gpu/drm/i915/i915_drv.h            |  3 ++
>  drivers/gpu/drm/i915/i915_pci.c            |  1 +
>  drivers/gpu/drm/i915/intel_device_info.c   |  8 +++++
>  drivers/gpu/drm/i915/intel_device_info.h   |  1 +
>  drivers/gpu/drm/i915/intel_wopcm.c         | 42 ++++++++++++++++++----
>  6 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 93b251b25aba..88aad892a0fc 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -394,6 +394,14 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>  		memset(runtime->num_sprites, 0, sizeof(runtime->num_sprites));
>  		memset(runtime->num_scalers, 0, sizeof(runtime->num_scalers));
>  	}
> +
> +	/*
> +	 * Early DG2 steppings don't have the GuC depriv feature. We can't
> +	 * rely on the fuse on those platforms because the meaning of the fuse
> +	 * bit is inverted on platforms that do have the feature.
> +	 */
> +	if (IS_DG2_GRAPHICS_STEP(dev_priv, G10, STEP_A0, STEP_A1))
> +		info->has_guc_deprivilege = 0;
> 
> Nit: not sure if it matters if this stepping is not-public (although I am not 100% sure I am correct in my assumption this is not-public).

Agree with Alan.

Are we ever going to let A0 / A1 stepping for DG2 be publicly available?
If the answer is no, I think this can be removed.

Matt

>  }