[PATCH] drm/i915/guc: Fix flag query to not modify state

[PATCH] drm/i915/guc: Fix flag query to not modify state

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

A flag query helper was actually writing to the flags word rather than
just reading. Fix that. Also update the function's comment as it was
out of date.

Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking")
Signed-off-by: John Harrison <john.c.harrison@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 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 b3a429a92c0d..d9f4218f5ef4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -174,11 +174,8 @@ static inline void init_sched_state(struct intel_context *ce)
 __maybe_unused
 static bool sched_state_is_init(struct intel_context *ce)
 {
-	/*
-	 * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED after
-	 * suspend.
-	 */
-	return !(ce->guc_state.sched_state &=
+	/* Kernel contexts can have SCHED_STATE_REGISTERED after suspend. */
+	return !(ce->guc_state.sched_state &
 		 ~(SCHED_STATE_BLOCKED_MASK | SCHED_STATE_REGISTERED));
 }
 
-- 
2.25.1

Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix flag query to not modify state

[Tvrtko Ursulin]

On 08/02/2022 02:07, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> A flag query helper was actually writing to the flags word rather than
> just reading. Fix that. Also update the function's comment as it was
> out of date.
> 
> Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking")
> Signed-off-by: John Harrison <john.c.harrison@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 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 b3a429a92c0d..d9f4218f5ef4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -174,11 +174,8 @@ static inline void init_sched_state(struct intel_context *ce)
>   __maybe_unused
>   static bool sched_state_is_init(struct intel_context *ce)
>   {
> -	/*
> -	 * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED after
> -	 * suspend.
> -	 */
> -	return !(ce->guc_state.sched_state &=
> +	/* Kernel contexts can have SCHED_STATE_REGISTERED after suspend. */
> +	return !(ce->guc_state.sched_state &
>   		 ~(SCHED_STATE_BLOCKED_MASK | SCHED_STATE_REGISTERED));
>   }
>   

Looks important - what are the consequences?

Needs Cc: stable for 5.16?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix flag query to not modify state

[John Harrison]

On 2/8/2022 01:39, Tvrtko Ursulin wrote:
> On 08/02/2022 02:07, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> A flag query helper was actually writing to the flags word rather than
>> just reading. Fix that. Also update the function's comment as it was
>> out of date.
>>
>> Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking")
>> Signed-off-by: John Harrison <john.c.harrison@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 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 b3a429a92c0d..d9f4218f5ef4 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -174,11 +174,8 @@ static inline void init_sched_state(struct 
>> intel_context *ce)
>>   __maybe_unused
>>   static bool sched_state_is_init(struct intel_context *ce)
>>   {
>> -    /*
>> -     * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED 
>> after
>> -     * suspend.
>> -     */
>> -    return !(ce->guc_state.sched_state &=
>> +    /* Kernel contexts can have SCHED_STATE_REGISTERED after 
>> suspend. */
>> +    return !(ce->guc_state.sched_state &
>>            ~(SCHED_STATE_BLOCKED_MASK | SCHED_STATE_REGISTERED));
>>   }
>
> Looks important - what are the consequences?
Supposedly nothing.

The test was only ever used inside a BUG_ON during context registration. 
Rather than asserting that the condition was true, it was making the 
condition true. So, in theory, there was no consequence because we 
should never have hit a BUG_ON anyway. Which means the write should 
always have been a no-op.

>
> Needs Cc: stable for 5.16?
Meaning "Cc: <stable@vger.kernel.org>"? Or is there anything required to 
specify 5.16?

John.


>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH] drm/i915/guc: Fix flag query to not modify state

[Tvrtko Ursulin]

On 08/02/2022 18:53, John Harrison wrote:
> On 2/8/2022 01:39, Tvrtko Ursulin wrote:
>> On 08/02/2022 02:07, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> A flag query helper was actually writing to the flags word rather than
>>> just reading. Fix that. Also update the function's comment as it was
>>> out of date.
>>>
>>> Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking")
>>> Signed-off-by: John Harrison <john.c.harrison@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 ++-----
>>>   1 file changed, 2 insertions(+), 5 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 b3a429a92c0d..d9f4218f5ef4 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> @@ -174,11 +174,8 @@ static inline void init_sched_state(struct 
>>> intel_context *ce)
>>>   __maybe_unused
>>>   static bool sched_state_is_init(struct intel_context *ce)
>>>   {
>>> -    /*
>>> -     * XXX: Kernel contexts can have SCHED_STATE_NO_LOCK_REGISTERED 
>>> after
>>> -     * suspend.
>>> -     */
>>> -    return !(ce->guc_state.sched_state &=
>>> +    /* Kernel contexts can have SCHED_STATE_REGISTERED after 
>>> suspend. */
>>> +    return !(ce->guc_state.sched_state &
>>>            ~(SCHED_STATE_BLOCKED_MASK | SCHED_STATE_REGISTERED));
>>>   }
>>
>> Looks important - what are the consequences?
> Supposedly nothing.
> 
> The test was only ever used inside a BUG_ON during context registration. 
> Rather than asserting that the condition was true, it was making the 
> condition true. So, in theory, there was no consequence because we 
> should never have hit a BUG_ON anyway. Which means the write should 
> always have been a no-op.
> 
>>
>> Needs Cc: stable for 5.16?
> Meaning "Cc: <stable@vger.kernel.org>"? Or is there anything required to 
> specify 5.16?

It would have been:

Cc: <stable@vger.kernel.org> # v5.16+

You can use "dim fixes <sha>" and it will output you the suggested tags.

But given what you say about it having no impact even in debug builds 
then it's not needed. Just note that explaining the impact in the commit 
message when Fixes: tag is present is very desirable in general. Without 
it maintainers have a hard time assessing and highlighting important 
stuff in pull requests. So I would at least ask for respin with updated 
commit message explaining there is no consequence and why cc stable is 
not needed.

Regards,

Tvrtko