Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job

to the 6.10-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     drm-sched-always-wake-up-correct-scheduler-in-drm_sched_entity_push_job.patch
and it can be found in the queue-6.10 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


From cbc8764e29c2318229261a679b2aafd0f9072885 Mon Sep 17 00:00:00 2001
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Date: Tue, 24 Sep 2024 11:19:08 +0100
Subject: drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

commit cbc8764e29c2318229261a679b2aafd0f9072885 upstream.

Since drm_sched_entity_modify_sched() can modify the entities run queue,
lets make sure to only dereference the pointer once so both adding and
waking up are guaranteed to be consistent.

Alternative of moving the spin_unlock to after the wake up would for now
be more problematic since the same lock is taken inside
drm_sched_rq_update_fifo().

v2:
 * Improve commit message. (Philipp)
 * Cache the scheduler pointer directly. (Christian)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify sched list")
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Philipp Stanner <pstanner@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.7+
Reviewed-by: Christian König <christian.koenig@amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240924101914.2713-3-tursulin@igalia.com
Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/gpu/drm/scheduler/sched_entity.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -599,6 +599,9 @@ void drm_sched_entity_push_job(struct dr
 
 	/* first job wakes up scheduler */
 	if (first) {
+		struct drm_gpu_scheduler *sched;
+		struct drm_sched_rq *rq;
+
 		/* Add the entity to the run queue */
 		spin_lock(&entity->rq_lock);
 		if (entity->stopped) {
@@ -608,13 +611,16 @@ void drm_sched_entity_push_job(struct dr
 			return;
 		}
 
-		drm_sched_rq_add_entity(entity->rq, entity);
+		rq = entity->rq;
+		sched = rq->sched;
+
+		drm_sched_rq_add_entity(rq, entity);
 		spin_unlock(&entity->rq_lock);
 
 		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
 			drm_sched_rq_update_fifo(entity, submit_ts);
 
-		drm_sched_wakeup(entity->rq->sched);
+		drm_sched_wakeup(sched);
 	}
 }
 EXPORT_SYMBOL(drm_sched_entity_push_job);


Patches currently in stable-queue which might be from tvrtko.ursulin@igalia.com are

queue-6.10/drm-sched-always-increment-correct-scheduler-score.patch
queue-6.10/drm-v3d-prevent-out-of-bounds-access-in-performance-query-extensions.patch
queue-6.10/drm-sched-always-wake-up-correct-scheduler-in-drm_sched_entity_push_job.patch
queue-6.10/drm-sched-add-locking-to-drm_sched_entity_modify_sched.patch

Re: Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

[Christian König]

Hi Greg,

please drop this patch from all backports. It turned out to be broken 
and the old handling has been restored by a revert.

Sorry for the noise. The revert should show up in Linus tree by the end 
of the week.

Regards,
Christian.

Am 07.10.24 um 19:50 schrieb gregkh@linuxfoundation.org:
> This is a note to let you know that I've just added the patch titled
>
>      drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job
>
> to the 6.10-stable tree which can be found at:
>      http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>       drm-sched-always-wake-up-correct-scheduler-in-drm_sched_entity_push_job.patch
> and it can be found in the queue-6.10 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
>  From cbc8764e29c2318229261a679b2aafd0f9072885 Mon Sep 17 00:00:00 2001
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Date: Tue, 24 Sep 2024 11:19:08 +0100
> Subject: drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> commit cbc8764e29c2318229261a679b2aafd0f9072885 upstream.
>
> Since drm_sched_entity_modify_sched() can modify the entities run queue,
> lets make sure to only dereference the pointer once so both adding and
> waking up are guaranteed to be consistent.
>
> Alternative of moving the spin_unlock to after the wake up would for now
> be more problematic since the same lock is taken inside
> drm_sched_rq_update_fifo().
>
> v2:
>   * Improve commit message. (Philipp)
>   * Cache the scheduler pointer directly. (Christian)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify sched list")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Luben Tuikov <ltuikov89@gmail.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Link: https://patchwork.freedesktop.org/patch/msgid/20240924101914.2713-3-tursulin@igalia.com
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c |   10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -599,6 +599,9 @@ void drm_sched_entity_push_job(struct dr
>   
>   	/* first job wakes up scheduler */
>   	if (first) {
> +		struct drm_gpu_scheduler *sched;
> +		struct drm_sched_rq *rq;
> +
>   		/* Add the entity to the run queue */
>   		spin_lock(&entity->rq_lock);
>   		if (entity->stopped) {
> @@ -608,13 +611,16 @@ void drm_sched_entity_push_job(struct dr
>   			return;
>   		}
>   
> -		drm_sched_rq_add_entity(entity->rq, entity);
> +		rq = entity->rq;
> +		sched = rq->sched;
> +
> +		drm_sched_rq_add_entity(rq, entity);
>   		spin_unlock(&entity->rq_lock);
>   
>   		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>   			drm_sched_rq_update_fifo(entity, submit_ts);
>   
> -		drm_sched_wakeup(entity->rq->sched);
> +		drm_sched_wakeup(sched);
>   	}
>   }
>   EXPORT_SYMBOL(drm_sched_entity_push_job);
>
>
> Patches currently in stable-queue which might be from tvrtko.ursulin@igalia.com are
>
> queue-6.10/drm-sched-always-increment-correct-scheduler-score.patch
> queue-6.10/drm-v3d-prevent-out-of-bounds-access-in-performance-query-extensions.patch
> queue-6.10/drm-sched-always-wake-up-correct-scheduler-in-drm_sched_entity_push_job.patch
> queue-6.10/drm-sched-add-locking-to-drm_sched_entity_modify_sched.patch

Re: Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

[Christian König]

Or wait a second with that.

Tvrtko which one was the patch we had to revert?

Regards,
Christian.

Am 07.10.24 um 19:53 schrieb Christian König:
> Hi Greg,
>
> please drop this patch from all backports. It turned out to be broken 
> and the old handling has been restored by a revert.
>
> Sorry for the noise. The revert should show up in Linus tree by the 
> end of the week.
>
> Regards,
> Christian.
>
> Am 07.10.24 um 19:50 schrieb gregkh@linuxfoundation.org:
>> This is a note to let you know that I've just added the patch titled
>>
>>      drm/sched: Always wake up correct scheduler in 
>> drm_sched_entity_push_job
>>
>> to the 6.10-stable tree which can be found at:
>> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>> drm-sched-always-wake-up-correct-scheduler-in-drm_sched_entity_push_job.patch
>> and it can be found in the queue-6.10 subdirectory.
>>
>> If you, or anyone else, feels it should not be added to the stable tree,
>> please let <stable@vger.kernel.org> know about it.
>>
>>
>>  From cbc8764e29c2318229261a679b2aafd0f9072885 Mon Sep 17 00:00:00 2001
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Date: Tue, 24 Sep 2024 11:19:08 +0100
>> Subject: drm/sched: Always wake up correct scheduler in 
>> drm_sched_entity_push_job
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> commit cbc8764e29c2318229261a679b2aafd0f9072885 upstream.
>>
>> Since drm_sched_entity_modify_sched() can modify the entities run queue,
>> lets make sure to only dereference the pointer once so both adding and
>> waking up are guaranteed to be consistent.
>>
>> Alternative of moving the spin_unlock to after the wake up would for now
>> be more problematic since the same lock is taken inside
>> drm_sched_rq_update_fifo().
>>
>> v2:
>>   * Improve commit message. (Philipp)
>>   * Cache the scheduler pointer directly. (Christian)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Fixes: b37aced31eb0 ("drm/scheduler: implement a function to modify 
>> sched list")
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Luben Tuikov <ltuikov89@gmail.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.7+
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Link: 
>> https://patchwork.freedesktop.org/patch/msgid/20240924101914.2713-3-tursulin@igalia.com
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c |   10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -599,6 +599,9 @@ void drm_sched_entity_push_job(struct dr
>>         /* first job wakes up scheduler */
>>       if (first) {
>> +        struct drm_gpu_scheduler *sched;
>> +        struct drm_sched_rq *rq;
>> +
>>           /* Add the entity to the run queue */
>>           spin_lock(&entity->rq_lock);
>>           if (entity->stopped) {
>> @@ -608,13 +611,16 @@ void drm_sched_entity_push_job(struct dr
>>               return;
>>           }
>>   -        drm_sched_rq_add_entity(entity->rq, entity);
>> +        rq = entity->rq;
>> +        sched = rq->sched;
>> +
>> +        drm_sched_rq_add_entity(rq, entity);
>>           spin_unlock(&entity->rq_lock);
>>             if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>>               drm_sched_rq_update_fifo(entity, submit_ts);
>>   -        drm_sched_wakeup(entity->rq->sched);
>> +        drm_sched_wakeup(sched);
>>       }
>>   }
>>   EXPORT_SYMBOL(drm_sched_entity_push_job);
>>
>>
>> Patches currently in stable-queue which might be from 
>> tvrtko.ursulin@igalia.com are
>>
>> queue-6.10/drm-sched-always-increment-correct-scheduler-score.patch
>> queue-6.10/drm-v3d-prevent-out-of-bounds-access-in-performance-query-extensions.patch 
>>
>> queue-6.10/drm-sched-always-wake-up-correct-scheduler-in-drm_sched_entity_push_job.patch 
>>
>> queue-6.10/drm-sched-add-locking-to-drm_sched_entity_modify_sched.patch
>

Re: Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

[Greg KH]

On Mon, Oct 07, 2024 at 07:53:26PM +0200, Christian König wrote:
> Hi Greg,
> 
> please drop this patch from all backports. It turned out to be broken and
> the old handling has been restored by a revert.
> 
> Sorry for the noise. The revert should show up in Linus tree by the end of
> the week.

What is the revert's git id?  I'd rather take that so we don't have any
"why wasn't this patch applied" stuff later on.

thanks,

greg k-h

Re: Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

[Christian König]

Am 07.10.24 um 19:59 schrieb Greg KH:
> On Mon, Oct 07, 2024 at 07:53:26PM +0200, Christian König wrote:
>> Hi Greg,
>>
>> please drop this patch from all backports. It turned out to be broken and
>> the old handling has been restored by a revert.
>>
>> Sorry for the noise. The revert should show up in Linus tree by the end of
>> the week.
> What is the revert's git id?  I'd rather take that so we don't have any
> "why wasn't this patch applied" stuff later on.

Good point. I was also not 100% sure which ones of Tvrtko's patches we 
had to revert.

Here is the git Id from the drm-misc-fixes branch:

commit abf201f6ce14c4ceeccde5471bdf59614b83a3d8
Author: Christian König <christian.koenig@amd.com>
Date:   Mon Sep 30 15:07:49 2024 +0200

     drm/sched: revert "Always increment correct scheduler score"

     This reverts commit 087913e0ba2b3b9d7ccbafb2acf5dab9e35ae1d5.

     It turned out that the original code was correct since the rq can only
     change when there is no armed job for an entity.

     This change here broke the logic since we only incremented the counter
     for the first job, so revert it.

     Signed-off-by: Christian König <christian.koenig@amd.com>
     Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
     Link: 
https://patchwork.freedesktop.org/patch/msgid/20240930131451.536150-1-christian.koenig@amd.com

Sorry for the noise, we should have catched that during the review and 
put the stable tag on the revert as well.

Regards,
Christian.

>
> thanks,
>
> greg k-h

Re: Patch "drm/sched: Always wake up correct scheduler in drm_sched_entity_push_job" has been added to the 6.10-stable tree

[Greg KH]

On Mon, Oct 07, 2024 at 08:23:54PM +0200, Christian König wrote:
> Am 07.10.24 um 19:59 schrieb Greg KH:
> > On Mon, Oct 07, 2024 at 07:53:26PM +0200, Christian König wrote:
> > > Hi Greg,
> > > 
> > > please drop this patch from all backports. It turned out to be broken and
> > > the old handling has been restored by a revert.
> > > 
> > > Sorry for the noise. The revert should show up in Linus tree by the end of
> > > the week.
> > What is the revert's git id?  I'd rather take that so we don't have any
> > "why wasn't this patch applied" stuff later on.
> 
> Good point. I was also not 100% sure which ones of Tvrtko's patches we had
> to revert.
> 
> Here is the git Id from the drm-misc-fixes branch:
> 
> commit abf201f6ce14c4ceeccde5471bdf59614b83a3d8
> Author: Christian König <christian.koenig@amd.com>
> Date:   Mon Sep 30 15:07:49 2024 +0200
> 
>     drm/sched: revert "Always increment correct scheduler score"
> 
>     This reverts commit 087913e0ba2b3b9d7ccbafb2acf5dab9e35ae1d5.
> 
>     It turned out that the original code was correct since the rq can only
>     change when there is no armed job for an entity.
> 
>     This change here broke the logic since we only incremented the counter
>     for the first job, so revert it.
> 
>     Signed-off-by: Christian König <christian.koenig@amd.com>
>     Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>     Link: https://patchwork.freedesktop.org/patch/msgid/20240930131451.536150-1-christian.koenig@amd.com
> 
> Sorry for the noise, we should have catched that during the review and put
> the stable tag on the revert as well.

Thanks for the info, I've picked this revert up now as well.

greg k-h