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

[Michal Wajdeczko]

On 02.07.2021 15:12, Martin Peres wrote:
> On 02/07/2021 16:06, Michal Wajdeczko wrote:
>>
>>
>> On 02.07.2021 10:13, Martin Peres wrote:
>>> On 01/07/2021 21:24, Martin Peres wrote:
>>> [...]
>>>>>
>>>>>>
>>>>>>> +        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?
>>>
>>> In case you missed this crucial part of the review. Please answer the
>>> above question.
>>
>> I guess there is some misunderstanding here, and I must admit I had
>> similar doubt, but if you look beyond patch diff and check function code
>> you will find that the very condition is:
>>
>>     /* Don't enable GuC/HuC on pre-Gen12 */
>>     if (GRAPHICS_VER(i915) < 12) {
>>         i915->params.enable_guc = 0;
>>         return;
>>     }
>>
>> so all pre-Gen12 platforms will continue to have GuC/HuC disabled.
> 
> Thanks Michal, but then the problem is the other way: how can one enable
> it on gen11?

this code here converts default GuC auto mode (enable_guc=-1) into per
platform desired (tested) GuC/HuC enables.

to override that default, you may still use enable_guc=1 to explicitly
enable GuC submission and since we also have this code:

+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;
+}

it should work on any Gen11+.

Michal

> 
> I like what Daniele was going for here: separating the capability from
> the user-requested value, but then it seems the patch stopped half way.
> How about never touching the parameter, and having a AND between the two
> values to get the effective enable_guc?
> 
> Right now, the code is really confusing :s
> 
> Thanks,
> Martin
> 
>>
>> Thanks,
>> Michal
>>