Re: [PATCH v4 1/2] intel: 48b ppgtt support (EXEC_OBJECT_SUPPORTS_48B_ADDRESS flag)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 5, 2015 at 7:03 AM, Michel Thierry <michel.thierry@xxxxxxxxx> wrote:
> On 9/14/2015 2:54 PM, Michał Winiarski wrote:
>>
>> On Thu, Sep 03, 2015 at 03:23:58PM +0100, Michel Thierry wrote:
>>>
>>> Gen8+ supports 48-bit virtual addresses, but some objects must always be
>>> allocated inside the 32-bit address range.
>>>
>>> In specific, any resource used with flat/heapless (0x00000000-0xfffff000)
>>> General State Heap (GSH) or Instruction State Heap (ISH) must be in a
>>> 32-bit range, because the General State Offset and Instruction State
>>> Offset
>>> are limited to 32-bits.
>>>
>>> The i915 driver has been modified to provide a flag to set when the 4GB
>>> limit is not necessary in a given bo (EXEC_OBJECT_SUPPORTS_48B_ADDRESS).
>>> 48-bit range will only be used when explicitly requested.
>>>
>>> Callers to the existing drm_intel_bo_emit_reloc function should set the
>>> use_48b_address_range flag beforehand, in order to use full ppgtt range.
>>>
>>> v2: Make set/clear functions nops on pre-gen8 platforms, and use them
>>>      internally in emit_reloc functions (Ben)
>>>      s/48BADDRESS/48B_ADDRESS/ (Dave)
>>> v3: Keep set/clear functions internal, no-one needs to use them directly.
>>> v4: Don't set 48bit-support flag in emit reloc, check for ppgtt type
>>>      before enabling set/clear function, print full offsets in debug
>>>      statements, using port of lower_32_bits and upper_32_bits from linux
>>>      kernel (Michał)
>>>
>>> References:
>>> http://lists.freedesktop.org/archives/intel-gfx/2015-July/072612.html
>>> Cc: Ben Widawsky <ben@xxxxxxxxxxxx>
>>> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
>>
>>
>> +Kristian
>>
>> LGTM.
>> Acked-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
>>
>>> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
>
>
>
> Hi Kristian,
>
> More comments on this?
> I've resent the mesa patch with the last changes
> (http://lists.freedesktop.org/archives/dri-devel/2015-October/091752.html).
>
> Michał has agreed with the interface and will use it for his work.
> If mesa doesn't plan to use this for now, it's ok. The kernel changes have
> been done to prevent any regressions when the support 48-bit flag is not
> set.

I didn't get any replies to my last comments on this:

http://lists.freedesktop.org/archives/mesa-dev/2015-August/091398.html

Basically, I tried explicitly placing buffers above 8G and didn't see
the HW problem described (ie it all worked fine).  I still think
there's some confusion as to what the W/A is about.

Here's my take: the W/A is a SW-only workaround to handle the cases
where a driver uses heapless and 48-bit ppgtt. The problem is that the
heaps are limited to 4G but with 48bit ppgtt a buffer can be placed
anywhere it the 48 bit address space. If that happens it's consideredd
out-of-bounds for the heap and access fails. To prevent this we need
to make sure the bos we address in a heapless fashion are placed below
4g.

For mesa, we only configure general state base as heap-less, which
affects scratch bos. What this boils down to is that we need the 4G
restriction only for the scratch bos set up on 3DSTATE_VS, 3DSTATE_GS
and 3DSTATE_PS (and tesselation stage eventually). Look for the
OUT_RELOC64 for stage->scratch_bo in gen8_vs_state.c, gen8_gs_state.c
and gen8_ps_state.c

Kristian

> Thanks,
>
> -Michel
>
>
>>> ---
>>>   include/drm/i915_drm.h    |  3 +-
>>>   intel/intel_bufmgr.c      | 11 ++++++
>>>   intel/intel_bufmgr.h      |  1 +
>>>   intel/intel_bufmgr_gem.c  | 88
>>> +++++++++++++++++++++++++++++++++++++----------
>>>   intel/intel_bufmgr_priv.h | 14 ++++++++
>>>   5 files changed, 97 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
>>> index ded43b1..426b25c 100644
>>> --- a/include/drm/i915_drm.h
>>> +++ b/include/drm/i915_drm.h
>>> @@ -680,7 +680,8 @@ struct drm_i915_gem_exec_object2 {
>>>   #define EXEC_OBJECT_NEEDS_FENCE (1<<0)
>>>   #define EXEC_OBJECT_NEEDS_GTT (1<<1)
>>>   #define EXEC_OBJECT_WRITE     (1<<2)
>>> -#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_WRITE<<1)
>>> +#define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
>>> +#define __EXEC_OBJECT_UNKNOWN_FLAGS
>>> -(EXEC_OBJECT_SUPPORTS_48B_ADDRESS<<1)
>>>         __u64 flags;
>>>
>>>         __u64 rsvd1;
>>> diff --git a/intel/intel_bufmgr.c b/intel/intel_bufmgr.c
>>> index 14ea9f9..0856e60 100644
>>> --- a/intel/intel_bufmgr.c
>>> +++ b/intel/intel_bufmgr.c
>>> @@ -293,6 +293,17 @@ drm_intel_bo_madvise(drm_intel_bo *bo, int madv)
>>>   }
>>>
>>>   int
>>> +drm_intel_bo_use_48b_address_range(drm_intel_bo *bo, uint32_t enable)
>>> +{
>>> +       if (bo->bufmgr->bo_use_48b_address_range) {
>>> +               bo->bufmgr->bo_use_48b_address_range(bo, enable);
>>> +               return 0;
>>> +       }
>>> +
>>> +       return -ENODEV;
>>> +}
>>> +
>>> +int
>>>   drm_intel_bo_references(drm_intel_bo *bo, drm_intel_bo *target_bo)
>>>   {
>>>         return bo->bufmgr->bo_references(bo, target_bo);
>>> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
>>> index 95eecb8..a14c78f 100644
>>> --- a/intel/intel_bufmgr.h
>>> +++ b/intel/intel_bufmgr.h
>>> @@ -164,6 +164,7 @@ int drm_intel_bo_get_tiling(drm_intel_bo *bo,
>>> uint32_t * tiling_mode,
>>>   int drm_intel_bo_flink(drm_intel_bo *bo, uint32_t * name);
>>>   int drm_intel_bo_busy(drm_intel_bo *bo);
>>>   int drm_intel_bo_madvise(drm_intel_bo *bo, int madv);
>>> +int drm_intel_bo_use_48b_address_range(drm_intel_bo *bo, uint32_t
>>> enable);
>>>
>>>   int drm_intel_bo_disable_reuse(drm_intel_bo *bo);
>>>   int drm_intel_bo_is_reusable(drm_intel_bo *bo);
>>> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
>>> index 2723e21..09d82d2 100644
>>> --- a/intel/intel_bufmgr_gem.c
>>> +++ b/intel/intel_bufmgr_gem.c
>>> @@ -83,6 +83,22 @@
>>>   #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>>   #define MAX2(A, B) ((A) > (B) ? (A) : (B))
>>>
>>> +/**
>>> + * upper_32_bits - return bits 32-63 of a number
>>> + * @n: the number we're accessing
>>> + *
>>> + * A basic shift-right of a 64- or 32-bit quantity.  Use this to
>>> suppress
>>> + * the "right shift count >= width of type" warning when that quantity
>>> is
>>> + * 32-bits.
>>> + */
>>> +#define upper_32_bits(n) ((__u32)(((n) >> 16) >> 16))
>>> +
>>> +/**
>>> + * lower_32_bits - return bits 0-31 of a number
>>> + * @n: the number we're accessing
>>> + */
>>> +#define lower_32_bits(n) ((__u32)(n))
>>> +
>>>   typedef struct _drm_intel_bo_gem drm_intel_bo_gem;
>>>
>>>   struct drm_intel_gem_bo_bucket {
>>> @@ -237,6 +253,15 @@ struct _drm_intel_bo_gem {
>>>         bool is_userptr;
>>>
>>>         /**
>>> +        * Boolean of whether this buffer can be placed in the full
>>> 48-bit
>>> +        * address range on gen8+.
>>> +        *
>>> +        * By default, buffers will be keep in a 32-bit range, unless
>>> this
>>> +        * flag is explicitly set.
>>> +        */
>>> +       bool use_48b_address_range;
>>> +
>>> +       /**
>>>          * Size in bytes of this buffer and its relocation descendents.
>>>          *
>>>          * Used to avoid costly tree walking in
>>> @@ -395,14 +420,16 @@
>>> drm_intel_gem_dump_validation_list(drm_intel_bufmgr_gem *bufmgr_gem)
>>>                         drm_intel_bo_gem *target_gem =
>>>                             (drm_intel_bo_gem *) target_bo;
>>>
>>> -                       DBG("%2d: %d (%s)@0x%08llx -> "
>>> -                           "%d (%s)@0x%08lx + 0x%08x\n",
>>> +                       DBG("%2d: %d (%s)@0x%08x %08x -> "
>>> +                           "%d (%s)@0x%08x %08x + 0x%08x\n",
>>>                             i,
>>>                             bo_gem->gem_handle, bo_gem->name,
>>> -                           (unsigned long long)bo_gem->relocs[j].offset,
>>> +                           upper_32_bits(bo_gem->relocs[j].offset),
>>> +                           lower_32_bits(bo_gem->relocs[j].offset),
>>>                             target_gem->gem_handle,
>>>                             target_gem->name,
>>> -                           target_bo->offset64,
>>> +                           upper_32_bits(target_bo->offset64),
>>> +                           lower_32_bits(target_bo->offset64),
>>>                             bo_gem->relocs[j].delta);
>>>                 }
>>>         }
>>> @@ -468,11 +495,15 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo,
>>> int need_fence)
>>>         drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem
>>> *)bo->bufmgr;
>>>         drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *)bo;
>>>         int index;
>>> +       int flags = 0;
>>> +
>>> +       if (need_fence)
>>> +               flags |= EXEC_OBJECT_NEEDS_FENCE;
>>> +       if (bo_gem->use_48b_address_range)
>>> +               flags |= EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>>>
>>>         if (bo_gem->validate_index != -1) {
>>> -               if (need_fence)
>>> -
>>> bufmgr_gem->exec2_objects[bo_gem->validate_index].flags |=
>>> -                               EXEC_OBJECT_NEEDS_FENCE;
>>> +               bufmgr_gem->exec2_objects[bo_gem->validate_index].flags
>>> |= flags;
>>>                 return;
>>>         }
>>>
>>> @@ -501,13 +532,9 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo, int
>>> need_fence)
>>>         bufmgr_gem->exec2_objects[index].alignment = bo->align;
>>>         bufmgr_gem->exec2_objects[index].offset = 0;
>>>         bufmgr_gem->exec_bos[index] = bo;
>>> -       bufmgr_gem->exec2_objects[index].flags = 0;
>>> +       bufmgr_gem->exec2_objects[index].flags = flags;
>>>         bufmgr_gem->exec2_objects[index].rsvd1 = 0;
>>>         bufmgr_gem->exec2_objects[index].rsvd2 = 0;
>>> -       if (need_fence) {
>>> -               bufmgr_gem->exec2_objects[index].flags |=
>>> -                       EXEC_OBJECT_NEEDS_FENCE;
>>> -       }
>>>         bufmgr_gem->exec_count++;
>>>   }
>>>
>>> @@ -780,6 +807,7 @@ retry:
>>>         bo_gem->used_as_reloc_target = false;
>>>         bo_gem->has_error = false;
>>>         bo_gem->reusable = true;
>>> +       bo_gem->use_48b_address_range = false;
>>>
>>>         drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem,
>>> alignment);
>>>
>>> @@ -926,6 +954,7 @@ drm_intel_gem_bo_alloc_userptr(drm_intel_bufmgr
>>> *bufmgr,
>>>         bo_gem->used_as_reloc_target = false;
>>>         bo_gem->has_error = false;
>>>         bo_gem->reusable = false;
>>> +       bo_gem->use_48b_address_range = false;
>>>
>>>         drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0);
>>>
>>> @@ -1081,6 +1110,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr
>>> *bufmgr,
>>>         bo_gem->bo.handle = open_arg.handle;
>>>         bo_gem->global_name = handle;
>>>         bo_gem->reusable = false;
>>> +       bo_gem->use_48b_address_range = false;
>>>
>>>         memclear(get_tiling);
>>>         get_tiling.handle = bo_gem->gem_handle;
>>> @@ -1930,6 +1960,13 @@ do_bo_emit_reloc(drm_intel_bo *bo, uint32_t
>>> offset,
>>>         return 0;
>>>   }
>>>
>>> +static void
>>> +drm_intel_gem_bo_use_48b_address_range(drm_intel_bo *bo, uint32_t
>>> enable)
>>> +{
>>> +       drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
>>> +       bo_gem->use_48b_address_range = enable;
>>> +}
>>> +
>>>   static int
>>>   drm_intel_gem_bo_emit_reloc(drm_intel_bo *bo, uint32_t offset,
>>>                             drm_intel_bo *target_bo, uint32_t
>>> target_offset,
>>> @@ -2073,10 +2110,12 @@
>>> drm_intel_update_buffer_offsets(drm_intel_bufmgr_gem *bufmgr_gem)
>>>
>>>                 /* Update the buffer offset */
>>>                 if (bufmgr_gem->exec_objects[i].offset != bo->offset64) {
>>> -                       DBG("BO %d (%s) migrated: 0x%08lx -> 0x%08llx\n",
>>> -                           bo_gem->gem_handle, bo_gem->name,
>>> bo->offset64,
>>> -                           (unsigned long
>>> long)bufmgr_gem->exec_objects[i].
>>> -                           offset);
>>> +                       DBG("BO %d (%s) migrated: 0x%08x %08x -> 0x%08x
>>> %08x\n",
>>> +                           bo_gem->gem_handle, bo_gem->name,
>>> +                           upper_32_bits(bo->offset64),
>>> +                           lower_32_bits(bo->offset64),
>>> +
>>> upper_32_bits(bufmgr_gem->exec_objects[i].offset),
>>> +
>>> lower_32_bits(bufmgr_gem->exec_objects[i].offset));
>>>                         bo->offset64 =
>>> bufmgr_gem->exec_objects[i].offset;
>>>                         bo->offset = bufmgr_gem->exec_objects[i].offset;
>>>                 }
>>> @@ -2094,9 +2133,12 @@ drm_intel_update_buffer_offsets2
>>> (drm_intel_bufmgr_gem *bufmgr_gem)
>>>
>>>                 /* Update the buffer offset */
>>>                 if (bufmgr_gem->exec2_objects[i].offset != bo->offset64)
>>> {
>>> -                       DBG("BO %d (%s) migrated: 0x%08lx -> 0x%08llx\n",
>>> -                           bo_gem->gem_handle, bo_gem->name,
>>> bo->offset64,
>>> -                           (unsigned long
>>> long)bufmgr_gem->exec2_objects[i].offset);
>>> +                       DBG("BO %d (%s) migrated: 0x%08x %08x -> 0x%08x
>>> %08x\n",
>>> +                           bo_gem->gem_handle, bo_gem->name,
>>> +                           upper_32_bits(bo->offset64),
>>> +                           lower_32_bits(bo->offset64),
>>> +
>>> upper_32_bits(bufmgr_gem->exec2_objects[i].offset),
>>> +
>>> lower_32_bits(bufmgr_gem->exec2_objects[i].offset));
>>>                         bo->offset64 =
>>> bufmgr_gem->exec2_objects[i].offset;
>>>                         bo->offset = bufmgr_gem->exec2_objects[i].offset;
>>>                 }
>>> @@ -2481,6 +2523,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr
>>> *bufmgr, int prime_fd, int s
>>>         bo_gem->used_as_reloc_target = false;
>>>         bo_gem->has_error = false;
>>>         bo_gem->reusable = false;
>>> +       bo_gem->use_48b_address_range = false;
>>>
>>>         DRMINITLISTHEAD(&bo_gem->vma_list);
>>>         DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
>>> @@ -3278,6 +3321,13 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
>>>                 }
>>>         }
>>>
>>> +       if (bufmgr_gem->gen >= 8) {
>>> +               gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
>>> +               ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM,
>>> &gp);
>>> +               if (ret == 0 && *gp.value == 3)
>>> +                       bufmgr_gem->bufmgr.bo_use_48b_address_range =
>>> drm_intel_gem_bo_use_48b_address_range;
>>> +       }
>>> +
>>>         /* Let's go with one relocation per every 2 dwords (but round
>>> down a bit
>>>          * since a power of two will mean an extra page allocation for
>>> the reloc
>>>          * buffer).
>>> diff --git a/intel/intel_bufmgr_priv.h b/intel/intel_bufmgr_priv.h
>>> index 59ebd18..5c17ffb 100644
>>> --- a/intel/intel_bufmgr_priv.h
>>> +++ b/intel/intel_bufmgr_priv.h
>>> @@ -152,6 +152,20 @@ struct _drm_intel_bufmgr {
>>>         void (*destroy) (drm_intel_bufmgr *bufmgr);
>>>
>>>         /**
>>> +        * Indicate if the buffer can be placed anywhere in the full
>>> ppgtt
>>> +        * address range (2^48).
>>> +        *
>>> +        * Any resource used with flat/heapless (0x00000000-0xfffff000)
>>> +        * General State Heap (GSH) or Intructions State Heap (ISH) must
>>> +        * be in a 32-bit range. 48-bit range will only be used when
>>> explicitly
>>> +        * requested.
>>> +        *
>>> +        * \param bo Buffer to set the use_48b_address_range flag.
>>> +        * \param enable The flag value.
>>> +        */
>>> +       void (*bo_use_48b_address_range) (drm_intel_bo *bo, uint32_t
>>> enable);
>>> +
>>> +       /**
>>>          * Add relocation entry in reloc_buf, which will be updated with
>>> the
>>>          * target buffer's real offset on on command submission.
>>>          *
>>> --
>>> 2.5.0
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux