[Intel-gfx] [PATCH 47/47] drm/i915/guc: Unblock GuC submission on Gen11+

Thu Jul 1 18:24:55 UTC 2021

On 30/06/2021 21:00, Matthew Brost wrote:
> On Wed, Jun 30, 2021 at 11:22:38AM +0300, Martin Peres wrote:
>>
>>
>> On 24/06/2021 10:05, Matthew Brost wrote:
>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>>
>>> Unblock GuC submission on Gen11+ platforms.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc.h            |  1 +
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  8 ++++++++
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h |  3 +--
>>>    drivers/gpu/drm/i915/gt/uc/intel_uc.c             | 14 +++++++++-----
>>>    4 files changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> index fae01dc8e1b9..77981788204f 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> @@ -54,6 +54,7 @@ struct intel_guc {
>>>    	struct ida guc_ids;
>>>    	struct list_head guc_id_list;
>>> +	bool submission_supported;
>>>    	bool submission_selected;
>>>    	struct i915_vma *ads_vma;
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> index a427336ce916..405339202280 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct intel_guc *guc)
>>>    	/* Note: By the time we're here, GuC may have already been reset */
>>>    }
>>> +static bool __guc_submission_supported(struct intel_guc *guc)
>>> +{
>>> +	/* GuC submission is unavailable for pre-Gen11 */
>>> +	return intel_guc_is_supported(guc) &&
>>> +	       INTEL_GEN(guc_to_gt(guc)->i915) >= 11;
>>> +}
>>> +
>>>    static bool __guc_submission_selected(struct intel_guc *guc)
>>>    {
>>>    	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>>> @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct intel_guc *guc)
>>>    void intel_guc_submission_init_early(struct intel_guc *guc)
>>>    {
>>> +	guc->submission_supported = __guc_submission_supported(guc);
>>>    	guc->submission_selected = __guc_submission_selected(guc);
>>>    }
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
>>> index a2a3fad72be1..be767eb6ff71 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
>>> @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc,
>>>    static inline bool intel_guc_submission_is_supported(struct intel_guc *guc)
>>>    {
>>> -	/* XXX: GuC submission is unavailable for now */
>>> -	return false;
>>> +	return guc->submission_supported;
>>>    }
>>>    static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc)
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index 7a69c3c027e9..61be0aa81492 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct intel_uc *uc)
>>>    		return;
>>>    	}
>>> -	/* Default: enable HuC authentication only */
>>> -	i915->params.enable_guc = ENABLE_GUC_LOAD_HUC;
>>> +	/* Intermediate platforms are HuC authentication only */
>>> +	if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) {
>>> +		drm_dbg(&i915->drm, "Disabling GuC only due to old platform\n");
>>
>> This comment does not seem accurate, given that DG1 is barely out, and ADL
>> is not out yet. How about:
>>
>> "Disabling GuC on untested platforms"?
> 
> This isn't my comment but it seems right to me. AFAIK this describes the
> current PR but it is subject to change (i.e. we may enable GuC on DG1 by
> default at some point).

Well, it's pretty bad PR to say that DG1 and ADL are old when they are 
not even out ;)

But seriously, fix this sentence, it makes no sense at all unless you 
are really trying to confuse non-native speakers (and annoy language 
purists too).

> 
>>
>>> +		i915->params.enable_guc = ENABLE_GUC_LOAD_HUC;
>>> +		return;
>>> +	}
>>> +
>>> +	/* Default: enable HuC authentication and GuC submission */
>>> +	i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | ENABLE_GUC_SUBMISSION;
>>
>> This seems to be in contradiction with the GuC submission plan which states:
>>
>> "Not enabled by default on any current platforms but can be enabled via
>> modparam enable_guc".
>>
> 
> I don't believe any current platform gets this point where GuC
> submission would be enabled by default. The first would be ADL-P which
> isn't out yet.

Isn't that exactly what the line above does?

> 
>> When you rework the patch, could you please add a warning when the user
>> force-enables the GuC Command Submission? Something like:
>>
>> "WARNING: The user force-enabled the experimental GuC command submission
>> backend using i915.enable_guc. Please disable it if experiencing stability
>> issues. No bug reports will be accepted on this backend".
>>
>> This should allow you to work on the backend, while communicating clearly to
>> users that it is not ready just yet. Once it has matured, the warning can be
>> removed.
> 
> This is a good idea but the only issue I see this message blowing up CI.
> We plan to enable GuC submission, via a modparam, on several platforms
> (e.g. TGL) where TGL isn't the PR in CI. I think if is a debug level
> message CI should be happy but I'll double check on this.

Some taints would be problematic. The only issue you may have is the IGT 
reload tests which could give you a dmesg-warn if you were to use any 
level under level 5. So put is as an info message and you'll be good :)

In case of doubt, just ask Petri / Adrinael, he is your local IGT 
maintainer.

Martin

> 
> Matt
> 
>>
>> Cheers,
>> Martin
>>
>>>    }
>>>    /* Reset GuC providing us with fresh state for both GuC and HuC.
>>> @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc)
>>>    	if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
>>>    		return -ENOMEM;
>>> -	/* XXX: GuC submission is unavailable for now */
>>> -	GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
>>> -
>>>    	ret = intel_guc_init(guc);
>>>    	if (ret)
>>>    		return ret;
>>>