Re: [Intel-xe] [PATCH 11/12] drm/xe/gsc: Define GSCCS for MTL

[John Harrison <john.c.harrison@intel.com>]

On 10/27/2023 15:29, Daniele Ceraolo Spurio wrote:
> Add the GSCCS to the media_xelpmp engine list. Note that since the
> GSCCS is only used with the GSC FW, we can consider it disabled if we
> don't have the FW available.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_hw_engine.c | 20 ++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_pci.c       |  2 +-
>   2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index b5b084590888..142783177e45 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -26,6 +26,7 @@
>   #include "xe_rtp.h"
>   #include "xe_sched_job.h"
>   #include "xe_tuning.h"
> +#include "xe_uc_fw.h"
>   #include "xe_wa.h"
>   
>   #define MAX_MMIO_BASES 3
> @@ -610,6 +611,24 @@ static void read_compute_fuses(struct xe_gt *gt)
>   		read_compute_fuses_from_dss(gt);
>   }
>   
> +static void check_gsc_availability(struct xe_gt *gt)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	if (!(gt->info.engine_mask & BIT(XE_HW_ENGINE_GSCCS0)))
> +		return;
> +
> +	/*
> +	 * The GSCCS is only used to communicate with the GSC FW, so if we don't
> +	 * have the FW there is nothing we need the engine for and can therefore
> +	 * skip its initialization.
Is that correct? Is this testing against a firmware image being 
available on the system or against a firmware blob being defined for the 
platform?

If the firmware is not available but was defined then presumably that 
means that any feature that is reliant upon GSC is going to be broken - 
HDCP, PXP, etc. It seems like that should warrant more than a drm_info 
level message.

On the other hand, if the check is about whether a firmware blob is 
defined for this platform then surely this code path wouldn't even be 
taken in the first place? This can only be reached if actually trying to 
load a firmware file at all, can't it?

John.

> +	 */
> +	if (!xe_uc_fw_is_available(&gt->uc.gsc.fw)) {
> +		gt->info.engine_mask &= ~BIT(XE_HW_ENGINE_GSCCS0);
> +		drm_info(&xe->drm, "gsccs disabled due to lack of FW\n");
> +	}
> +}
> +
>   int xe_hw_engines_init_early(struct xe_gt *gt)
>   {
>   	int i;
> @@ -617,6 +636,7 @@ int xe_hw_engines_init_early(struct xe_gt *gt)
>   	read_media_fuses(gt);
>   	read_copy_fuses(gt);
>   	read_compute_fuses(gt);
> +	check_gsc_availability(gt);
>   
>   	BUILD_BUG_ON(XE_HW_ENGINE_PREEMPT_TIMEOUT < XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN);
>   	BUILD_BUG_ON(XE_HW_ENGINE_PREEMPT_TIMEOUT > XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX);
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 2fae45b9d88e..c09b51735ee4 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -197,7 +197,7 @@ static const struct xe_media_desc media_xelpmp = {
>   	.name = "Xe_LPM+",
>   	.hw_engine_mask =
>   		BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VCS2) |
> -		BIT(XE_HW_ENGINE_VECS0),	/* TODO: add GSC0 */
> +		BIT(XE_HW_ENGINE_VECS0) | BIT(XE_HW_ENGINE_GSCCS0)
>   };
>   
>   static const struct xe_media_desc media_xe2 = {