Re: [Intel-gfx] [PATCH 1/7] drm/i915/huc: only load HuC on GTs that have VCS engines

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 9/26/2022 9:15 AM, Tvrtko Ursulin wrote:
>
> On 23/09/2022 16:41, Ceraolo Spurio, Daniele wrote:
>> On 9/23/2022 3:53 AM, Tvrtko Ursulin wrote:
>>>
>>> On 22/09/2022 23:11, Daniele Ceraolo Spurio wrote:
>>>> On MTL the primary GT doesn't have any media capabilities, so no video
>>>> engines and no HuC. We must therefore skip the HuC fetch and load on
>>>> that specific case. Given that other multi-GT platforms might have HuC
>>>> on the primary GT, we can't just check for that and it is easier to
>>>> instead check for the lack of VCS engines.
>>>>
>>>> Based on code from Aravind Iddamsetty
>>>>
>>>> Signed-off-by: Daniele Ceraolo Spurio 
>>>> <daniele.ceraolospurio@intel.com>
>>>> Cc: Aravind Iddamsetty <aravind.iddamsetty@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_huc.c | 21 +++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/i915_drv.h        |  9 ++++++---
>>>>   2 files changed, 27 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> index 3bb8838e325a..d4e2b252f16c 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
>>>> @@ -42,12 +42,33 @@
>>>>    * HuC-specific commands.
>>>>    */
>>>>   +static bool vcs_supported(struct intel_gt *gt)
>>>> +{
>>>> +    intel_engine_mask_t mask = gt->info.engine_mask;
>>>> +
>>>> +    /*
>>>> +     * we can reach here from i915_driver_early_probe for primary
>>>> +     * GT with it being not fully setup hence fall back to the 
>>>> device info's
>>>> +     * engine mask
>>>> +     */
>>>> +    if (!mask && gt_is_root(gt))
>>>> +        mask = RUNTIME_INFO(gt->i915)->platform_engine_mask;
>>>
>>> Is it possible for all instances to be fused off? Wondering if the 
>>> function shouldn't just use platform_engine_mask.
>>
>> The spec says that there is always going to be at least 1 VCS (bspec 
>> 55417 in case you want to double-check). I don't see that changing in 
>> the future, because what's the point of having a media GT if you 
>> don't have any enabled VCS engines on it?
>
> That was my gut feeling as well, however..
>
>> Also, platform_engine_mask only contains the entries of the primary 
>> GT, for the other GTs we'd have to navigate the array in the device 
>> info structure and I don't think we want to do that from here when 
>> we've already copied the mask inside gt->info.engine_mask.
>
> ... this is very annoying. Because function is now a bit dodgy, no? 
> Maybe gets the caller a real answer for a _specific_ gt, or maybe gets 
> a fake-ish answer for a root gt. Or if not a root gt and called too 
> early maybe it returns a false zero?
>
> Hm would GEM_BUG_ON(!mask && !gt_is_root(gt)) be correct?
>
> And not even bother to implement is as fallback?
>
> if (gt_is_root)
>     return platform_mask;
> else
>     return gt_mask;
>
> Would that be clearer? Coupled with the comment from the patch, maybe 
> expanded with the statement that if there are some vcs engines, at 
> least one must remain post fusing?

This works for me. I'll wait a bit to see if there are comments on the 
other patches and then send an update.

Thanks,
Daniele

>
> Regards,
>
> Tvrtko
>
>>>> +
>>>> +    return __ENGINE_INSTANCES_MASK(mask, VCS0, I915_MAX_VCS);
>>>> +}
>>>> +
>>>>   void intel_huc_init_early(struct intel_huc *huc)
>>>>   {
>>>>       struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
>>>> +    struct intel_gt *gt = huc_to_gt(huc);
>>>>         intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC);
>>>>   +    if (!vcs_supported(gt)) {
>>>> +        intel_uc_fw_change_status(&huc->fw, 
>>>> INTEL_UC_FIRMWARE_NOT_SUPPORTED);
>>>> +        return;
>>>> +    }
>>>> +
>>>>       if (GRAPHICS_VER(i915) >= 11) {
>>>>           huc->status.reg = GEN11_HUC_KERNEL_LOAD_INFO;
>>>>           huc->status.mask = HUC_LOAD_SUCCESSFUL;
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 134fc1621821..8ca575202e5d 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -777,12 +777,15 @@ IS_SUBPLATFORM(const struct drm_i915_private 
>>>> *i915,
>>>>   #define __HAS_ENGINE(engine_mask, id) ((engine_mask) & BIT(id))
>>>>   #define HAS_ENGINE(gt, id) __HAS_ENGINE((gt)->info.engine_mask, id)
>>>>   -#define ENGINE_INSTANCES_MASK(gt, first, count) ({ \
>>>> +#define __ENGINE_INSTANCES_MASK(mask, first, count) ({            \
>>>>       unsigned int first__ = (first);                    \
>>>>       unsigned int count__ = (count);                    \
>>>> -    ((gt)->info.engine_mask & \
>>>> -     GENMASK(first__ + count__ - 1, first__)) >> first__;        \
>>>> +    ((mask) & GENMASK(first__ + count__ - 1, first__)) >> 
>>>> first__;    \
>>>>   })
>>>> +
>>>> +#define ENGINE_INSTANCES_MASK(gt, first, count) \
>>>> +    __ENGINE_INSTANCES_MASK((gt)->info.engine_mask, first, count)
>>>> +
>>>>   #define RCS_MASK(gt) \
>>>>       ENGINE_INSTANCES_MASK(gt, RCS0, I915_MAX_RCS)
>>>>   #define BCS_MASK(gt) \
>>