Re: [PATCH] drm/i915/guc: Don't send policy update for child contexts.

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

On 7/27/2022 6:44 PM, John Harrison wrote:
> On 7/27/2022 17:33, Daniele Ceraolo Spurio wrote:
>> The GuC FW applies the parent context policy to all the children,
>> so individual updates to the children are not supported and we
>> should not send them.
>>
>> Note that sending the message did not have any functional consequences,
>> because the GuC just drops it and logs an error; since we were trying
>> to set the child policy to match the parent anyway the message being
>> dropped was not a problem.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: John Harrison <john.c.harrison@intel.com>
> Needs a Fixes tag for the original v70 update patch?

I don't think so. I added the explanation about it not being a 
functional issue to make it clear that everything still works as 
expected without this patch, just with a bit of extra noise in the GuC 
logs. If you think it is still worth applying to older kernels I'll add 
the tag in.

Daniele

>
> John.
>
>> ---
>>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 26 +------------------
>>   1 file changed, 1 insertion(+), 25 deletions(-)
>>
>> 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 76916aed897a..5e31e2540297 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -2420,7 +2420,6 @@ static int guc_context_policy_init_v70(struct 
>> intel_context *ce, bool loop)
>>       struct context_policy policy;
>>       u32 execution_quantum;
>>       u32 preemption_timeout;
>> -    bool missing = false;
>>       unsigned long flags;
>>       int ret;
>>   @@ -2438,32 +2437,9 @@ static int 
>> guc_context_policy_init_v70(struct intel_context *ce, bool loop)
>>           __guc_context_policy_add_preempt_to_idle(&policy, 1);
>>         ret = __guc_context_set_context_policies(guc, &policy, loop);
>> -    missing = ret != 0;
>> -
>> -    if (!missing && intel_context_is_parent(ce)) {
>> -        struct intel_context *child;
>> -
>> -        for_each_child(ce, child) {
>> -            __guc_context_policy_start_klv(&policy, child->guc_id.id);
>> -
>> -            if (engine->flags & I915_ENGINE_WANT_FORCED_PREEMPTION)
>> - __guc_context_policy_add_preempt_to_idle(&policy, 1);
>> -
>> -            child->guc_state.prio = ce->guc_state.prio;
>> -            __guc_context_policy_add_priority(&policy, 
>> ce->guc_state.prio);
>> - __guc_context_policy_add_execution_quantum(&policy, 
>> execution_quantum);
>> - __guc_context_policy_add_preemption_timeout(&policy, 
>> preemption_timeout);
>> -
>> -            ret = __guc_context_set_context_policies(guc, &policy, 
>> loop);
>> -            if (ret) {
>> -                missing = true;
>> -                break;
>> -            }
>> -        }
>> -    }
>>         spin_lock_irqsave(&ce->guc_state.lock, flags);
>> -    if (missing)
>> +    if (ret != 0)
>>           set_context_policy_required(ce);
>>       else
>>           clr_context_policy_required(ce);
>