On Mon, Apr 28, 2014 at 05:18:28PM -0700, Ben Widawsky wrote:
> All the rest of the code to enable this is in my branch. Without my
> branch, hitting > 32b offsets is impossible. The code has always
> "supported" 64b, but it's never actually been run of tested. This change
> doesn't actually fix anything. [1] I am not sure why X won't work yet. I
> do not get hangs or obvious errors.
>
> There are 3 fixes grouped together here. First is to remove the
> hardcoded 0 for the upper dword of the relocation. The next fix is to
> use a 64b value for target_offset. The final fix is to not directly
> apply target_offset to reloc->delta. reloc->delta is part of ABI, and so
> we cannot change it. As it stands, 32b is enough to represent everything
> we're interested in representing anyway. The main problem is, we cannot
> add greater than 32b values to it directly.
>
> [1] Almost all of intel-gpu-tools is not yet ready to test 64b
> relocations. There are a few places that expect 32b values for offsets
> and these all won't work.
>
> Cc: Rafael Barbalho <rafael.barbalho@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
Seriously, we did this? I am ashamed. I was annoyed by the original
assertion that no userspace was ready in the first place, and to see
that the code was a complete farce anyway...
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0d806fc..6ffecd2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -262,10 +262,12 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
>
> static int
> relocate_entry_cpu(struct drm_i915_gem_object *obj,
> - struct drm_i915_gem_relocation_entry *reloc)
> + struct drm_i915_gem_relocation_entry *reloc,
> + uint64_t target_offset)
> {
> struct drm_device *dev = obj->base.dev;
> uint32_t page_offset = offset_in_page(reloc->offset);
> + uint64_t delta = reloc->delta + target_offset;
I would not have called the final value delta, but target_offset.
I was going to quible over the use of a local variable instead of
reloc->delta, but you successfully argued in my head that your way was
less obtuse.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
