Re: [PATCH 03/11] drm/amdgpu: convert amdgpu_vm_it to half closed intervals

2019-10-04 Thread Christian König 

Hi Michel,

Am 04.10.19 um 13:36 schrieb Michel Lespinasse:

On Fri, Oct 04, 2019 at 06:54:54AM +, Koenig, Christian wrote:

Am 03.10.19 um 22:18 schrieb Davidlohr Bueso:

The amdgpu_vm interval tree really wants [a, b) intervals,

NAK, we explicitly do need an [a, b[ interval here.

Hi Christian,

Just wanted to confirm where you stand on this patch, since I think
you reconsidered your initial position after first looking at 9/11
from this series.

I do not know the amdgpu code well, but I think the changes should be
fine - in struct amdgpu_bo_va_mapping, the "end" field will hold what
was previously stored in the "last" field, plus one. The expectation
is that overflows should not be an issue there, as "end" is explicitly
declared as an uint64, and as the code was previously computing
"last + 1" in many places.

Does that seem workable to you ?


No, we computed last + 1 in a couple of debug places were it doesn't 
hurt us and IIRC we currently cheat a bit because we use pfn instead of 
addresses on some other places.


But that is only a leftover from radeon and we need to fix that sooner 
or later, cause essentially the physical address space of the device is 
really full 64bits, e.g. 0x0-0x.


So that only fits into a 64bit int when we use half open/closed 
intervals, but would wrap around to zero if we use a closed interval.


I initially thought that the set was changing the interval tree into 
always using a closed interval, but that seems to have been a 
misunderstanding.


Regards,
Christian.



Thanks,



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 03/11] drm/amdgpu: convert amdgpu_vm_it to half closed intervals

2019-10-04 Thread Michel Lespinasse 
On Fri, Oct 04, 2019 at 06:54:54AM +, Koenig, Christian wrote:
> Am 03.10.19 um 22:18 schrieb Davidlohr Bueso:
> > The amdgpu_vm interval tree really wants [a, b) intervals,
> 
> NAK, we explicitly do need an [a, b[ interval here.

Hi Christian,

Just wanted to confirm where you stand on this patch, since I think
you reconsidered your initial position after first looking at 9/11
from this series.

I do not know the amdgpu code well, but I think the changes should be
fine - in struct amdgpu_bo_va_mapping, the "end" field will hold what
was previously stored in the "last" field, plus one. The expectation
is that overflows should not be an issue there, as "end" is explicitly
declared as an uint64, and as the code was previously computing
"last + 1" in many places.

Does that seem workable to you ?

Thanks,

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH 03/11] drm/amdgpu: convert amdgpu_vm_it to half closed intervals

2019-10-04 Thread Koenig, Christian 
Am 03.10.19 um 22:18 schrieb Davidlohr Bueso:
> The amdgpu_vm interval tree really wants [a, b) intervals,

NAK, we explicitly do need an [a, b[ interval here.

Regards,
Christian.

> not fully closed ones. As such convert it to use the new
> interval_tree_gen.h, and also rename the 'last' endpoint
> in the node to 'end', which is both a more suitable name
> for the half closed interval and also reduces the chances
> of missing a conversion when doing insertion or lookup.
>
> Cc: Jerome Glisse 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: Daniel Vetter 
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Davidlohr Bueso 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h  | 18 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c|  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c|  3 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 46 
> +++---
>   6 files changed, 36 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 49b767b7238f..290bfe820890 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -756,7 +756,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser 
> *p)
>   }
>   
>   if ((va_start + chunk_ib->ib_bytes) >
> - (m->last + 1) * AMDGPU_GPU_PAGE_SIZE) {
> + m->end * AMDGPU_GPU_PAGE_SIZE) {
>   DRM_ERROR("IB va_start+ib_bytes is invalid\n");
>   return -EINVAL;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 7e99f6c58c48..60b73bc4d11a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -51,7 +51,7 @@ struct amdgpu_bo_va_mapping {
>   struct list_headlist;
>   struct rb_node  rb;
>   uint64_tstart;
> - uint64_tlast;
> + uint64_tend;
>   uint64_t__subtree_last;
>   uint64_toffset;
>   uint64_tflags;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> index 8227ebd0f511..c5b0e88d019c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> @@ -247,7 +247,7 @@ TRACE_EVENT(amdgpu_vm_bo_map,
>   TP_STRUCT__entry(
>__field(struct amdgpu_bo *, bo)
>__field(long, start)
> -  __field(long, last)
> +  __field(long, end)
>__field(u64, offset)
>__field(u64, flags)
>),
> @@ -255,12 +255,12 @@ TRACE_EVENT(amdgpu_vm_bo_map,
>   TP_fast_assign(
>  __entry->bo = bo_va ? bo_va->base.bo : NULL;
>  __entry->start = mapping->start;
> -__entry->last = mapping->last;
> +__entry->end = mapping->end;
>  __entry->offset = mapping->offset;
>  __entry->flags = mapping->flags;
>  ),
> - TP_printk("bo=%p, start=%lx, last=%lx, offset=%010llx, flags=%llx",
> -   __entry->bo, __entry->start, __entry->last,
> + TP_printk("bo=%p, start=%lx, end=%lx, offset=%010llx, flags=%llx",
> +   __entry->bo, __entry->start, __entry->end,
> __entry->offset, __entry->flags)
>   );
>   
> @@ -271,7 +271,7 @@ TRACE_EVENT(amdgpu_vm_bo_unmap,
>   TP_STRUCT__entry(
>__field(struct amdgpu_bo *, bo)
>__field(long, start)
> -  __field(long, last)
> +  __field(long, end)
>__field(u64, offset)
>__field(u64, flags)
>),
> @@ -279,12 +279,12 @@ TRACE_EVENT(amdgpu_vm_bo_unmap,
>   TP_fast_assign(
>  __entry->bo = bo_va ? bo_va->base.bo : NULL;
>  __entry->start = mapping->start;
> -__entry->last = mapping->last;
> +__entry->end = mapping->end;
>  __entry->offset = mapping->offset;
>  __entry->flags = mapping->flags;
>  ),
> - TP_printk("bo=%p, start=%lx, last=%lx, offset=%010llx, flags=%llx",
> -   __entry->bo, __entry->start,