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

[John Harrison]

On 11/13/2023 13:32, Daniele Ceraolo Spurio wrote:
> 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).
Okay. That makes sense.

>
>>
>> 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.
Hmm. Meaning this is essentially pre-release only code? We generally try 
not to include internal only hacks in the public tree. Having said that, 
presumably this is not actually pre-release only. Exactly the same 
situation exists if the end user just does not have the firmware 
installed for any reason. In which case it seems reasonable given the 
above more specific checks in the firmware loading and PXP/HDCP code.

Reviewed-by: John Harrison <John.C.Harrison at Intel.com>


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