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

[Daniele Ceraolo Spurio]

On 11/13/2023 12:23 PM, John Harrison wrote:
> 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 at intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis at 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?

This is post fw fetch, so it checks whether we have a blob or not.

>
> 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.

The fetch code will already print at drm_notice level. This log is not 
about the fetch failure, it is about the GSC engine being disabled as a 
consequence of that (or of the FW not being defined in the first place). 
PXP and HDCP should have their own checks for GSC availability when we 
add support for them (they do in i915).

>
> 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?

I'm not sure what you mean here, this code path is part of the engine 
init flow, so we'll always take it.

Note that the main goal of this check is to allows us to define the GSC 
engine before the GSC FW, which is useful because the GSC FW is usually 
publicly released quite late for a new platform, while engines are one 
of the first things we define.

Daniele

>
> 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 = {
>