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 /*
> + * 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
