On 2017-09-17 08:03 AM, Oded Gabbay wrote:
> On Sat, Sep 16, 2017 at 2:43 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
>> From: Yong Zhao <yong.zhao at amd.com>
>>
>> Avoid intermediate negative numbers when doing calculations with a mix
>> of signed and unsigned variables where implicit conversions can lead
>> to unexpected results.
>>
>> When kernel queue buffer wraps around to 0, we need to check that rptr
>> won't be overwritten by the new packet.
>>
>> Signed-off-by: Yong Zhao <yong.zhao at amd.com>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> index 9ebb4c1..1c66334 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>> @@ -210,6 +210,11 @@ static int acquire_packet_buffer(struct kernel_queue *kq,
>> uint32_t wptr, rptr;
>> unsigned int *queue_address;
>>
>> + /* When rptr == wptr, the buffer is empty.
> Start comment text in a new line. First line should be just /*
That's a new rule to me, and it seems a bit arbitrary. checkpatch.pl
doesn't complain about this. checkpatch.pl does complain about the
closing */ not being on it's own line, but checkpatch has always been OK
with multiline comments starting on the same line as the opening /*.
There are also plenty of examples of multiline comments starting on the
same line as /*. For example run this grep on the include/linux:
grep -A3 '/\*[^*]\+[^/]$' *.h
Regards,
 Felix
>
>> + * When rptr == wptr + 1, the buffer is full.
>> + * It is always rptr that advances to the position of wptr, rather than
>> + * the opposite. So we can only use up to queue_size_dwords - 1 dwords.
>> + */
>> rptr = *kq->rptr_kernel;
>> wptr = *kq->wptr_kernel;
>> queue_address = (unsigned int *)kq->pq_kernel_addr;
>> @@ -219,11 +224,10 @@ static int acquire_packet_buffer(struct kernel_queue *kq,
>> pr_debug("wptr: %d\n", wptr);
>> pr_debug("queue_address 0x%p\n", queue_address);
>>
>> - available_size = (rptr - 1 - wptr + queue_size_dwords) %
>> + available_size = (rptr + queue_size_dwords - 1 - wptr) %
>> queue_size_dwords;
>>
>> - if (packet_size_in_dwords >= queue_size_dwords ||
>> - packet_size_in_dwords >= available_size) {
>> + if (packet_size_in_dwords > available_size) {
>> /*
>> * make sure calling functions know
>> * acquire_packet_buffer() failed
>> @@ -233,6 +237,14 @@ static int acquire_packet_buffer(struct kernel_queue *kq,
>> }
>>
>> if (wptr + packet_size_in_dwords >= queue_size_dwords) {
>> + /* make sure after rolling back to position 0, there is
>> + * still enough space.
>> + */
>> + if (packet_size_in_dwords >= rptr) {
>> + *buffer_ptr = NULL;
>> + return -ENOMEM;
>> + }
> I don't think the condition is correct.
> Suppose, queue_size_dwords == 100, wptr == rptr == 50 (queue is empty)
> and we have a new packet with size of 70.
> Now, wptr + size is 120, which is >= 100
> However, 70 >= rptr (50) which will give us -ENOMEM, but this is not
> correct condition, because the packet *does* have enough room in the
> queue.
>
> I think the condition should be:
> if (packet_size_in_dwords - (queue_size_dwords - wptr) >= rptr)
> but please check this.
>
>> + /* fill nops, roll back and start at position 0 */
>> while (wptr > 0) {
>> queue_address[wptr] = kq->nop_packet;
>> wptr = (wptr + 1) % queue_size_dwords;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
