Hi Michel,
Although I cannot comment on the exact implementation I can give you
general some tips which you might find useful.
On 1 July 2015 at 16:28, Michel Thierry <michel.thierry at intel.com> 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 Intruction State Heap (ISH) must be in a
> 32-bit range, because the General State Offset and Instruction State Offset
> are limited to 32-bits.
>
> Provide a flag to set when the 4GB limit is not necessary in a given bo.
> 48-bit range will only be used when explicitly requested.
>
> Calls to the new drm_intel_bo_emit_reloc_48bit function will have this flag
> set automatically, while calls to drm_intel_bo_emit_reloc will clear it.
>
> v2: Make set/clear functions nops on pre-gen8 platforms, and use them
> internally in emit_reloc functions (Ben)
> s/48BADDRESS/48B_ADDRESS/ (Dave)
>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> Cc: Dave Gordon <david.s.gordon at intel.com>
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> ---
> include/drm/i915_drm.h | 3 ++-
> intel/intel_bufmgr.c | 24 +++++++++++++++++++++
> intel/intel_bufmgr.h | 8 ++++++-
> intel/intel_bufmgr_gem.c | 54
> +++++++++++++++++++++++++++++++++++++++++++----
> intel/intel_bufmgr_priv.h | 11 ++++++++++
> 5 files changed, 94 insertions(+), 6 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)
Perhaps you already know this but changes like these go in _after_ the
updated kernel header is part of linux-next or a released kernel
version.
> __u64 flags;
>
> __u64 rsvd1;
> diff --git a/intel/intel_bufmgr.c b/intel/intel_bufmgr.c
> index 14ea9f9..590a855 100644
> --- a/intel/intel_bufmgr.c
> +++ b/intel/intel_bufmgr.c
> @@ -188,6 +188,18 @@ drm_intel_bufmgr_check_aperture_space(drm_intel_bo **
> bo_array, int count)
> return bo_array[0]->bufmgr->check_aperture_space(bo_array, count);
> }
>
> +void drm_intel_bo_set_supports_48b_address(drm_intel_bo *bo)
> +{
> + if (bo->bufmgr->bo_set_supports_48b_address)
> + bo->bufmgr->bo_set_supports_48b_address(bo);
> +}
> +
> +void drm_intel_bo_clear_supports_48b_address(drm_intel_bo *bo)
> +{
> + if (bo->bufmgr->bo_clear_supports_48b_address)
> + bo->bufmgr->bo_clear_supports_48b_address(bo);
> +}
> +
> int
> drm_intel_bo_flink(drm_intel_bo *bo, uint32_t * name)
> {
> @@ -202,6 +214,18 @@ drm_intel_bo_emit_reloc(drm_intel_bo *bo, uint32_t
> offset,
> drm_intel_bo *target_bo, uint32_t target_offset,
> uint32_t read_domains, uint32_t write_domain)
> {
> + drm_intel_bo_clear_supports_48b_address(target_bo);
> + return bo->bufmgr->bo_emit_reloc(bo, offset,
> + target_bo, target_offset,
> + read_domains, write_domain);
> +}
> +
> +int
> +drm_intel_bo_emit_reloc_48bit(drm_intel_bo *bo, uint32_t offset,
> + drm_intel_bo *target_bo, uint32_t target_offset,
> + uint32_t read_domains, uint32_t write_domain)
> +{
> + drm_intel_bo_set_supports_48b_address(target_bo);
> return bo->bufmgr->bo_emit_reloc(bo, offset,
> target_bo, target_offset,
> read_domains, write_domain);
> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
> index 285919e..62480cb 100644
> --- a/intel/intel_bufmgr.h
> +++ b/intel/intel_bufmgr.h
> @@ -87,7 +87,8 @@ struct _drm_intel_bo {
> /**
> * Last seen card virtual address (offset from the beginning of the
> * aperture) for the object. This should be used to fill relocation
> - * entries when calling drm_intel_bo_emit_reloc()
> + * entries when calling drm_intel_bo_emit_reloc() or
> + * drm_intel_bo_emit_reloc_48bit()
> */
> uint64_t offset64;
> };
> @@ -137,6 +138,8 @@ void drm_intel_bo_wait_rendering(drm_intel_bo *bo);
>
> void drm_intel_bufmgr_set_debug(drm_intel_bufmgr *bufmgr, int enable_debug);
> void drm_intel_bufmgr_destroy(drm_intel_bufmgr *bufmgr);
> +void drm_intel_bo_set_supports_48b_address(drm_intel_bo *bo);
> +void drm_intel_bo_clear_supports_48b_address(drm_intel_bo *bo);
Are these two are internal/implementation specific functions ? If so
please don't include them in this public header and annotate both
declaration and definition with drm_private.
> int drm_intel_bo_exec(drm_intel_bo *bo, int used,
> struct drm_clip_rect *cliprects, int num_cliprects, int
> DR4);
> int drm_intel_bo_mrb_exec(drm_intel_bo *bo, int used,
> @@ -147,6 +150,9 @@ int drm_intel_bufmgr_check_aperture_space(drm_intel_bo **
> bo_array, int count);
> int drm_intel_bo_emit_reloc(drm_intel_bo *bo, uint32_t offset,
> drm_intel_bo *target_bo, uint32_t target_offset,
> uint32_t read_domains, uint32_t write_domain);
> +int drm_intel_bo_emit_reloc_48bit(drm_intel_bo *bo, uint32_t offset,
> + drm_intel_bo *target_bo, uint32_t
> target_offset,
> + uint32_t read_domains, uint32_t
> write_domain);
Please add new (public) symbols to the test (intel-symbol-check). If
in doubt make check will give you a lovely message.
Patches that bump the version number are not normally posted to the
ML, but are committed as part of the release process (see RELEASING
for more info).
Cheers,
Emil
