| Message ID | 1449843217-29875-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/11/2015 2:13 PM, Micha? Winiarski wrote: > According to bspec, some parts of HW require the addresses to be in > a canonical form, where bits [63:48] == [47]. Let's convert addresses to > canonical form prior to relocating and return converted offsets to > userspace. We also need to make sure that userspace is using addresses > in canonical form in case of softpin. > > v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville) > v3: Rebase on top of softpin, fix a hole in relocate_entry, > s/expect/require (Chris) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> With updated gem_softpin [http://patchwork.freedesktop.org/patch/msgid/1449843255-32640-1-git-send-email-michel.thierry@intel.com] Tested-by: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 9 +++++++-- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 +++++++++++++++------ > drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++++++++++ > 3 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8e2acde..b83207b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3482,12 +3482,17 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, > > if (flags & PIN_OFFSET_FIXED) { > uint64_t offset = flags & PIN_OFFSET_MASK; > + uint64_t noncanonical_offset = offset & ((1ULL << 48) - 1); > > - if (offset & (alignment - 1) || offset + size > end) { > + if (offset & (alignment - 1) || > + noncanonical_offset + size > end || > + offset != gen8_canonical_addr(offset)) { > ret = -EINVAL; > goto err_free_vma; > } > - vma->node.start = offset; > + /* While userspace is using addresses in canonical form, our > + * allocator is unaware of this */ > + vma->node.start = noncanonical_offset; > vma->node.size = size; > vma->node.color = obj->cache_level; > ret = drm_mm_reserve_node(&vm->mm, &vma->node); > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 48ec484..445ccc7 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -249,6 +249,13 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj) > obj->cache_level != I915_CACHE_NONE); > } > > +static inline uint64_t > +relocation_target(struct drm_i915_gem_relocation_entry *reloc, > + uint64_t target_offset) > +{ > + return gen8_canonical_addr((int)reloc->delta + target_offset); > +} > + > static int > relocate_entry_cpu(struct drm_i915_gem_object *obj, > struct drm_i915_gem_relocation_entry *reloc, > @@ -256,7 +263,7 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj, > { > struct drm_device *dev = obj->base.dev; > uint32_t page_offset = offset_in_page(reloc->offset); > - uint64_t delta = reloc->delta + target_offset; > + uint64_t delta = relocation_target(reloc, target_offset); > char *vaddr; > int ret; > > @@ -292,7 +299,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj, > { > struct drm_device *dev = obj->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - uint64_t delta = reloc->delta + target_offset; > + uint64_t delta = relocation_target(reloc, target_offset); > uint64_t offset; > void __iomem *reloc_page; > int ret; > @@ -347,7 +354,7 @@ relocate_entry_clflush(struct drm_i915_gem_object *obj, > { > struct drm_device *dev = obj->base.dev; > uint32_t page_offset = offset_in_page(reloc->offset); > - uint64_t delta = (int)reloc->delta + target_offset; > + uint64_t delta = relocation_target(reloc, target_offset); > char *vaddr; > int ret; > > @@ -395,7 +402,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, > target_i915_obj = target_vma->obj; > target_obj = &target_vma->obj->base; > > - target_offset = target_vma->node.start; > + target_offset = gen8_canonical_addr(target_vma->node.start); > > /* Sandybridge PPGTT errata: We need a global gtt mapping for MI and > * pipe_control writes because the gpu doesn't properly redirect them > @@ -583,6 +590,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, > struct drm_i915_gem_object *obj = vma->obj; > struct drm_i915_gem_exec_object2 *entry = vma->exec_entry; > uint64_t flags; > + uint64_t offset; > int ret; > > flags = PIN_USER; > @@ -625,8 +633,9 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, > entry->flags |= __EXEC_OBJECT_HAS_FENCE; > } > > - if (entry->offset != vma->node.start) { > - entry->offset = vma->node.start; > + offset = gen8_canonical_addr(vma->node.start); > + if (entry->offset != offset) { > + entry->offset = offset; > *need_reloc = true; > } > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index b448ad8..fa1f3ab 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -504,6 +504,18 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length) > return i915_pte_count(address, length, GEN8_PDE_SHIFT); > } > > +/* Used to convert any address to canonical form. > + * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS, > + * MI_LOAD_REGISTER_MEM and others, see Broadwell PRM Vol2a) require the > + * addresses to be in a canonical form: > + * "GraphicsAddress[63:48] are ignored by the HW and assumed to be in correct > + * canonical form [63:48] == [47]." > + */ > +static inline uint64_t gen8_canonical_addr(uint64_t address) > +{ > + return ((int64_t)address << 16) >> 16; > +} > + > static inline dma_addr_t > i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n) > { >
On Fri, Dec 11, 2015 at 03:13:37PM +0100, Micha? Winiarski wrote: > According to bspec, some parts of HW require the addresses to be in > a canonical form, where bits [63:48] == [47]. Let's convert addresses to > canonical form prior to relocating and return converted offsets to > userspace. We also need to make sure that userspace is using addresses > in canonical form in case of softpin. > > v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville) > v3: Rebase on top of softpin, fix a hole in relocate_entry, > s/expect/require (Chris) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 9 +++++++-- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 +++++++++++++++------ > drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++++++++++ > 3 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8e2acde..b83207b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3482,12 +3482,17 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, > > if (flags & PIN_OFFSET_FIXED) { > uint64_t offset = flags & PIN_OFFSET_MASK; > + uint64_t noncanonical_offset = offset & ((1ULL << 48) - 1); No. Core GEM doesn't care about canonical addresses, it just operates with respect to the drm_mm range manager. More importantly you missed that the only chance we have to validate the user parameters is during validate_exec_list(). -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8e2acde..b83207b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3482,12 +3482,17 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, if (flags & PIN_OFFSET_FIXED) { uint64_t offset = flags & PIN_OFFSET_MASK; + uint64_t noncanonical_offset = offset & ((1ULL << 48) - 1); - if (offset & (alignment - 1) || offset + size > end) { + if (offset & (alignment - 1) || + noncanonical_offset + size > end || + offset != gen8_canonical_addr(offset)) { ret = -EINVAL; goto err_free_vma; } - vma->node.start = offset; + /* While userspace is using addresses in canonical form, our + * allocator is unaware of this */ + vma->node.start = noncanonical_offset; vma->node.size = size; vma->node.color = obj->cache_level; ret = drm_mm_reserve_node(&vm->mm, &vma->node); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 48ec484..445ccc7 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -249,6 +249,13 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj) obj->cache_level != I915_CACHE_NONE); } +static inline uint64_t +relocation_target(struct drm_i915_gem_relocation_entry *reloc, + uint64_t target_offset) +{ + return gen8_canonical_addr((int)reloc->delta + target_offset); +} + static int relocate_entry_cpu(struct drm_i915_gem_object *obj, struct drm_i915_gem_relocation_entry *reloc, @@ -256,7 +263,7 @@ relocate_entry_cpu(struct drm_i915_gem_object *obj, { struct drm_device *dev = obj->base.dev; uint32_t page_offset = offset_in_page(reloc->offset); - uint64_t delta = reloc->delta + target_offset; + uint64_t delta = relocation_target(reloc, target_offset); char *vaddr; int ret; @@ -292,7 +299,7 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj, { struct drm_device *dev = obj->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - uint64_t delta = reloc->delta + target_offset; + uint64_t delta = relocation_target(reloc, target_offset); uint64_t offset; void __iomem *reloc_page; int ret; @@ -347,7 +354,7 @@ relocate_entry_clflush(struct drm_i915_gem_object *obj, { struct drm_device *dev = obj->base.dev; uint32_t page_offset = offset_in_page(reloc->offset); - uint64_t delta = (int)reloc->delta + target_offset; + uint64_t delta = relocation_target(reloc, target_offset); char *vaddr; int ret; @@ -395,7 +402,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, target_i915_obj = target_vma->obj; target_obj = &target_vma->obj->base; - target_offset = target_vma->node.start; + target_offset = gen8_canonical_addr(target_vma->node.start); /* Sandybridge PPGTT errata: We need a global gtt mapping for MI and * pipe_control writes because the gpu doesn't properly redirect them @@ -583,6 +590,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, struct drm_i915_gem_object *obj = vma->obj; struct drm_i915_gem_exec_object2 *entry = vma->exec_entry; uint64_t flags; + uint64_t offset; int ret; flags = PIN_USER; @@ -625,8 +633,9 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, entry->flags |= __EXEC_OBJECT_HAS_FENCE; } - if (entry->offset != vma->node.start) { - entry->offset = vma->node.start; + offset = gen8_canonical_addr(vma->node.start); + if (entry->offset != offset) { + entry->offset = offset; *need_reloc = true; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index b448ad8..fa1f3ab 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -504,6 +504,18 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length) return i915_pte_count(address, length, GEN8_PDE_SHIFT); } +/* Used to convert any address to canonical form. + * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS, + * MI_LOAD_REGISTER_MEM and others, see Broadwell PRM Vol2a) require the + * addresses to be in a canonical form: + * "GraphicsAddress[63:48] are ignored by the HW and assumed to be in correct + * canonical form [63:48] == [47]." + */ +static inline uint64_t gen8_canonical_addr(uint64_t address) +{ + return ((int64_t)address << 16) >> 16; +} + static inline dma_addr_t i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n) {
According to bspec, some parts of HW require the addresses to be in a canonical form, where bits [63:48] == [47]. Let's convert addresses to canonical form prior to relocating and return converted offsets to userspace. We also need to make sure that userspace is using addresses in canonical form in case of softpin. v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville) v3: Rebase on top of softpin, fix a hole in relocate_entry, s/expect/require (Chris) Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 9 +++++++-- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 +++++++++++++++------ drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++++++++++ 3 files changed, 34 insertions(+), 8 deletions(-)