SWDEV-197284 - drm/amdgpu: Only use the peek function in productor side is not correct

Submitted by Deng, Emily on Aug. 12, 2019, 7:42 a.m.

Details

Message ID 1565595762-8141-1-git-send-email-Emily.Deng@amd.com
State New
Headers show
Series "SWDEV-197284 - drm/amdgpu: Only use the peek function in productor side is not correct" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Deng, Emily Aug. 12, 2019, 7:42 a.m.
For spsc queue, use peek function would cause error in productor side.
As for the last element, when the push job is occurring during pop job, the peek function
will not be updated in time, and it will return NULL.

So use queue count for double confirming the job queue is empty.

Signed-off-by: Emily Deng <Emily.Deng@amd.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
 include/drm/spsc_queue.h                 | 7 +++----
 2 files changed, 5 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 35ddbec..e74894f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -95,7 +95,7 @@  static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity)
 	rmb(); /* for list_empty to work without lock */
 
 	if (list_empty(&entity->list) ||
-	    spsc_queue_peek(&entity->job_queue) == NULL)
+	    ((spsc_queue_peek(&entity->job_queue) == NULL) && !spsc_queue_count(&entity->job_queue)))
 		return true;
 
 	return false;
@@ -281,7 +281,7 @@  void drm_sched_entity_fini(struct drm_sched_entity *entity)
 	/* Consumption of existing IBs wasn't completed. Forcefully
 	 * remove them here.
 	 */
-	if (spsc_queue_peek(&entity->job_queue)) {
+	if ((spsc_queue_peek(&entity->job_queue) == NULL) && !spsc_queue_count(&entity->job_queue)) {
 		if (sched) {
 			/* Park the kernel for a moment to make sure it isn't processing
 			 * our enity.
diff --git a/include/drm/spsc_queue.h b/include/drm/spsc_queue.h
index 125f096..78092b9 100644
--- a/include/drm/spsc_queue.h
+++ b/include/drm/spsc_queue.h
@@ -54,9 +54,8 @@  static inline void spsc_queue_init(struct spsc_queue *queue)
 
 static inline struct spsc_node *spsc_queue_peek(struct spsc_queue *queue)
 {
-	return queue->head;
+	return READ_ONCE(queue->head);
 }
-
 static inline int spsc_queue_count(struct spsc_queue *queue)
 {
 	return atomic_read(&queue->job_count);
@@ -70,9 +69,9 @@  static inline bool spsc_queue_push(struct spsc_queue *queue, struct spsc_node *n
 
 	preempt_disable();
 
+	atomic_inc(&queue->job_count);
 	tail = (struct spsc_node **)atomic_long_xchg(&queue->tail, (long)&node->next);
 	WRITE_ONCE(*tail, node);
-	atomic_inc(&queue->job_count);
 
 	/*
 	 * In case of first element verify new node will be visible to the consumer
@@ -93,6 +92,7 @@  static inline struct spsc_node *spsc_queue_pop(struct spsc_queue *queue)
 	/* Verify reading from memory and not the cache */
 	smp_rmb();
 
+	atomic_dec(&queue->job_count);
 	node = READ_ONCE(queue->head);
 
 	if (!node)
@@ -113,7 +113,6 @@  static inline struct spsc_node *spsc_queue_pop(struct spsc_queue *queue)
 		}
 	}
 
-	atomic_dec(&queue->job_count);
 	return node;
 }
 

Comments

Am 12.08.19 um 09:42 schrieb Emily Deng:
> For spsc queue, use peek function would cause error in productor side.
> As for the last element, when the push job is occurring during pop job, the peek function
> will not be updated in time, and it will return NULL.
>
> So use queue count for double confirming the job queue is empty.

For the upstream branch I'm going to push my patch which is not as 
invasive as this one.

Christian.

>
> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--
>   include/drm/spsc_queue.h                 | 7 +++----
>   2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 35ddbec..e74894f 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -95,7 +95,7 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>   	rmb(); /* for list_empty to work without lock */
>   
>   	if (list_empty(&entity->list) ||
> -	    spsc_queue_peek(&entity->job_queue) == NULL)
> +	    ((spsc_queue_peek(&entity->job_queue) == NULL) && !spsc_queue_count(&entity->job_queue)))
>   		return true;
>   
>   	return false;
> @@ -281,7 +281,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
>   	/* Consumption of existing IBs wasn't completed. Forcefully
>   	 * remove them here.
>   	 */
> -	if (spsc_queue_peek(&entity->job_queue)) {
> +	if ((spsc_queue_peek(&entity->job_queue) == NULL) && !spsc_queue_count(&entity->job_queue)) {
>   		if (sched) {
>   			/* Park the kernel for a moment to make sure it isn't processing
>   			 * our enity.
> diff --git a/include/drm/spsc_queue.h b/include/drm/spsc_queue.h
> index 125f096..78092b9 100644
> --- a/include/drm/spsc_queue.h
> +++ b/include/drm/spsc_queue.h
> @@ -54,9 +54,8 @@ static inline void spsc_queue_init(struct spsc_queue *queue)
>   
>   static inline struct spsc_node *spsc_queue_peek(struct spsc_queue *queue)
>   {
> -	return queue->head;
> +	return READ_ONCE(queue->head);
>   }
> -
>   static inline int spsc_queue_count(struct spsc_queue *queue)
>   {
>   	return atomic_read(&queue->job_count);
> @@ -70,9 +69,9 @@ static inline bool spsc_queue_push(struct spsc_queue *queue, struct spsc_node *n
>   
>   	preempt_disable();
>   
> +	atomic_inc(&queue->job_count);
>   	tail = (struct spsc_node **)atomic_long_xchg(&queue->tail, (long)&node->next);
>   	WRITE_ONCE(*tail, node);
> -	atomic_inc(&queue->job_count);
>   
>   	/*
>   	 * In case of first element verify new node will be visible to the consumer
> @@ -93,6 +92,7 @@ static inline struct spsc_node *spsc_queue_pop(struct spsc_queue *queue)
>   	/* Verify reading from memory and not the cache */
>   	smp_rmb();
>   
> +	atomic_dec(&queue->job_count);
>   	node = READ_ONCE(queue->head);
>   
>   	if (!node)
> @@ -113,7 +113,6 @@ static inline struct spsc_node *spsc_queue_pop(struct spsc_queue *queue)
>   		}
>   	}
>   
> -	atomic_dec(&queue->job_count);
>   	return node;
>   }
>
Ok, please ignore this patch.


Best wishes
Emily Deng

>-----Original Message-----

>From: Christian König <ckoenig.leichtzumerken@gmail.com>

>Sent: Tuesday, August 13, 2019 1:00 AM

>To: Deng, Emily <Emily.Deng@amd.com>; amd-gfx@lists.freedesktop.org

>Subject: Re: [PATCH] SWDEV-197284 - drm/amdgpu: Only use the peek

>function in productor side is not correct

>

>Am 12.08.19 um 09:42 schrieb Emily Deng:

>> For spsc queue, use peek function would cause error in productor side.

>> As for the last element, when the push job is occurring during pop

>> job, the peek function will not be updated in time, and it will return NULL.

>>

>> So use queue count for double confirming the job queue is empty.

>

>For the upstream branch I'm going to push my patch which is not as invasive

>as this one.

>

>Christian.

>

>>

>> Signed-off-by: Emily Deng <Emily.Deng@amd.com>

>> ---

>>   drivers/gpu/drm/scheduler/sched_entity.c | 4 ++--

>>   include/drm/spsc_queue.h                 | 7 +++----

>>   2 files changed, 5 insertions(+), 6 deletions(-)

>>

>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c

>> b/drivers/gpu/drm/scheduler/sched_entity.c

>> index 35ddbec..e74894f 100644

>> --- a/drivers/gpu/drm/scheduler/sched_entity.c

>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c

>> @@ -95,7 +95,7 @@ static bool drm_sched_entity_is_idle(struct

>drm_sched_entity *entity)

>>   	rmb(); /* for list_empty to work without lock */

>>

>>   	if (list_empty(&entity->list) ||

>> -	    spsc_queue_peek(&entity->job_queue) == NULL)

>> +	    ((spsc_queue_peek(&entity->job_queue) == NULL) &&

>> +!spsc_queue_count(&entity->job_queue)))

>>   		return true;

>>

>>   	return false;

>> @@ -281,7 +281,7 @@ void drm_sched_entity_fini(struct drm_sched_entity

>*entity)

>>   	/* Consumption of existing IBs wasn't completed. Forcefully

>>   	 * remove them here.

>>   	 */

>> -	if (spsc_queue_peek(&entity->job_queue)) {

>> +	if ((spsc_queue_peek(&entity->job_queue) == NULL) &&

>> +!spsc_queue_count(&entity->job_queue)) {

>>   		if (sched) {

>>   			/* Park the kernel for a moment to make sure it isn't

>processing

>>   			 * our enity.

>> diff --git a/include/drm/spsc_queue.h b/include/drm/spsc_queue.h index

>> 125f096..78092b9 100644

>> --- a/include/drm/spsc_queue.h

>> +++ b/include/drm/spsc_queue.h

>> @@ -54,9 +54,8 @@ static inline void spsc_queue_init(struct spsc_queue

>> *queue)

>>

>>   static inline struct spsc_node *spsc_queue_peek(struct spsc_queue *queue)

>>   {

>> -	return queue->head;

>> +	return READ_ONCE(queue->head);

>>   }

>> -

>>   static inline int spsc_queue_count(struct spsc_queue *queue)

>>   {

>>   	return atomic_read(&queue->job_count); @@ -70,9 +69,9 @@ static

>> inline bool spsc_queue_push(struct spsc_queue *queue, struct spsc_node

>> *n

>>

>>   	preempt_disable();

>>

>> +	atomic_inc(&queue->job_count);

>>   	tail = (struct spsc_node **)atomic_long_xchg(&queue->tail,

>(long)&node->next);

>>   	WRITE_ONCE(*tail, node);

>> -	atomic_inc(&queue->job_count);

>>

>>   	/*

>>   	 * In case of first element verify new node will be visible to the

>> consumer @@ -93,6 +92,7 @@ static inline struct spsc_node

>*spsc_queue_pop(struct spsc_queue *queue)

>>   	/* Verify reading from memory and not the cache */

>>   	smp_rmb();

>>

>> +	atomic_dec(&queue->job_count);

>>   	node = READ_ONCE(queue->head);

>>

>>   	if (!node)

>> @@ -113,7 +113,6 @@ static inline struct spsc_node

>*spsc_queue_pop(struct spsc_queue *queue)

>>   		}

>>   	}

>>

>> -	atomic_dec(&queue->job_count);

>>   	return node;

>>   }

>>