| Message ID | 1449242443-32493-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 04, 2015 at 04:20:43PM +0100, Micha? Winiarski wrote: > According to bspec, some parts of HW expect 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. > > v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville) > > 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> Can we igt this? Maybe with softpin or whatever ... For cpu address space negative addresses are for the kernel, but I think on the gpu we can do them. -Daniel > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++++--- > drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++++++++++ > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index a4c243c..ceffef9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -395,7 +395,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 +583,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; > @@ -623,8 +624,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 877c32c..4ea9dab 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -507,6 +507,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) expect 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) > { > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2015-12-04 at 17:18 +0100, Daniel Vetter wrote: > On Fri, Dec 04, 2015 at 04:20:43PM +0100, Micha? Winiarski wrote: > > According to bspec, some parts of HW expect 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. > > > > v2: Whitespace fixup, gen8_canonical_addr description (Chris, > > Ville) > > > > 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> > > Can we igt this? Maybe with softpin or whatever ... For cpu address > space > negative addresses are for the kernel, but I think on the gpu we can > do > them. Yup - would definitely be useful, I can add something to one of the existing reloc tests, but without softpin it's tricky to cover this in a reliable way (not to mention corner cases). -Micha? > -Daniel > > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++++--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++++++++++ > > 2 files changed, 17 insertions(+), 3 deletions(-)
On Fri, Dec 04, 2015 at 05:41:11PM +0000, Winiarski, Michal wrote: > On Fri, 2015-12-04 at 17:18 +0100, Daniel Vetter wrote: > > On Fri, Dec 04, 2015 at 04:20:43PM +0100, Micha? Winiarski wrote: > > > According to bspec, some parts of HW expect 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. > > > > > > v2: Whitespace fixup, gen8_canonical_addr description (Chris, > > > Ville) > > > > > > 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> > > > > Can we igt this? Maybe with softpin or whatever ... For cpu address > > space > > negative addresses are for the kernel, but I think on the gpu we can > > do > > them. > > Yup - would definitely be useful, I can add something to one of the > existing reloc tests, but without softpin it's tricky to cover this in > a reliable way (not to mention corner cases). Oh without softpin it's going to be impossible I think ;-) -Daniel
On Fri, Dec 04, 2015 at 04:20:43PM +0100, Micha? Winiarski wrote: > According to bspec, some parts of HW expect 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. > > v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville) What's the impact? The bspec says "assumes" but not required? Does anything truly break otherwise? -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index a4c243c..ceffef9 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -395,7 +395,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 +583,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; @@ -623,8 +624,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 877c32c..4ea9dab 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -507,6 +507,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) expect 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 expect 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. v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville) 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_execbuffer.c | 8 +++++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-)