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

[Deng, Emily]

Ok, please ignore this patch.


Best wishes
Emily Deng

>-----Original Message-----
>From: Christian König <ckoenig.leichtzumerken at gmail.com>
>Sent: Tuesday, August 13, 2019 1:00 AM
>To: Deng, Emily <Emily.Deng at amd.com>; amd-gfx at 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 at 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;
>>   }
>>