[PATCH] drm/i915: Avoid writing relocs with addresses in non-canonical form

[PATCH] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

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.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 9 ++++++---
 drivers/gpu/drm/i915/i915_gem_gtt.h        | 5 +++++
 2 files changed, 11 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..9f27be9 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,10 @@ 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..65e8f88 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -507,6 +507,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
 	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
 }
 
+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

Re: [PATCH] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Ville Syrjälä]

On Fri, Dec 04, 2015 at 12:33:44PM +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.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 9 ++++++---
>  drivers/gpu/drm/i915/i915_gem_gtt.h        | 5 +++++
>  2 files changed, 11 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..9f27be9 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,10 @@ 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..65e8f88 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -507,6 +507,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
>  	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
>  }
>  
> +static inline uint64_t gen8_canonical_addr(uint64_t address)
> +{
> +	return ((int64_t)address << 16) >> 16;
> +}

This could really use a comment explaining what it's doing and why.
Just lifting the first sentence of your commit message into a comment
would seem to be enough.

> +
>  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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Fri, Dec 04, 2015 at 12:33:44PM +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.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 9 ++++++---
>  drivers/gpu/drm/i915/i915_gem_gtt.h        | 5 +++++
>  2 files changed, 11 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..9f27be9 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);

Ok, this keeps userspace in sync as well as writing the cannonical
addreses to hardware.

>  	/* 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,10 @@ 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);
> +

Kill the extra line.

> +	if (entry->offset != offset) {
> +		entry->offset = offset;
>  		*need_reloc = true;

And this copies back the cannonical address to userspace. Ok, the
userspace side looks correct (the presumed_offset and execobject.offsets
will be updated to store the cannonical address).

>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 877c32c..65e8f88 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -507,6 +507,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
>  	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
>  }
>  
> +static inline uint64_t gen8_canonical_addr(uint64_t address)
> +{

You need the bspec reference here as well.

Looks good to me, with just a quick explanation here as to what we are
doing and why.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

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(-)

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

Re: [PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

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>

There is a hole! Inside the actual relocation fixup you need

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a4c243cec4aa..6d1a2d20337b 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,12 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
                obj->cache_level != I915_CACHE_NONE);
 }
 
+static uint64_t relocation_target(struct drm_i915_gem_relocation_entry *reloc,
+                                 uint64_t target_offset)
+{
+       return gen8_cannonical_address((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 +262,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_get);
        char *vaddr;
        int ret;
 
@@ -292,7 +298,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_get);
        uint64_t offset;
        void __iomem *reloc_page;
        int ret;
@@ -347,7 +353,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_get);
        char *vaddr;
        int ret;
 
Otherwise we may cross the 1<<47 boundary without swapping into cannonical
form.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Winiarski, Michal]

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(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

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);
 
-		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)
 {
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michel Thierry]

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)
>   {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

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)
v4: Handle softpin in validate_exec_list (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_execbuffer.c | 45 ++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..87d9887 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,25 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* 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 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 +275,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 +311,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 +366,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 +414,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 +602,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 +645,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;
 	}
 
@@ -994,6 +1015,18 @@ validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), since
+		 * userspace has to use canonical format, we need to reject all
+		 * non-canonical addresses.
+		 */
+		if (exec[i].offset != gen8_canonical_addr(exec[i].offset))
+			return -EINVAL;
+
+		/* On the other hand, from drm_mm perspective address space is
+		 * continuous, so we're converting to non-canonical form
+		 */
+		exec[i].offset &= (1ULL << 48) - 1;
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Fri, Dec 18, 2015 at 09:01:03PM +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)
> v4: Handle softpin in validate_exec_list (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>

> @@ -625,8 +645,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 is now in normal form.

> +		entry->offset = offset;
>  		*need_reloc = true;
>  	}

But we need to convert back to canonical form again on the userspace
boundary (copy_to_user).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

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)
v4: Handle softpin in validate_exec_list (Chris)
v5: Convert back to canonical form at copy_to_user time (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_execbuffer.c | 107 ++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..c906232 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,25 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* 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 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 +275,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 +311,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 +366,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 +414,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
@@ -994,6 +1013,18 @@ validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), since
+		 * userspace has to use canonical format, we need to reject all
+		 * non-canonical addresses.
+		 */
+		if (exec[i].offset != gen8_canonical_addr(exec[i].offset))
+			return -EINVAL;
+
+		/* On the other hand, from drm_mm perspective address space is
+		 * continuous, so we're converting to non-canonical form
+		 */
+		exec[i].offset &= (1ULL << 48) - 1;
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;
 
@@ -1617,6 +1648,29 @@ pre_mutex_err:
 	return ret;
 }
 
+static inline int
+__i915_gem_execlist_copy_to_user(struct drm_i915_gem_execbuffer2 *args,
+				 struct drm_i915_gem_exec_object2 *exec2_list)
+{
+	struct drm_i915_gem_exec_object2 __user *user_exec_list =
+			   to_user_ptr(args->buffers_ptr);
+	int i;
+
+	for (i = 0; i < args->buffer_count; i++) {
+		/* Userspace uses addresses in canonical form */
+		exec2_list[i].offset = gen8_canonical_addr(exec2_list[i].offset);
+		if (__copy_to_user(&user_exec_list[i].offset,
+				   &exec2_list[i].offset,
+				   sizeof(user_exec_list[i].offset))) {
+			DRM_DEBUG("failed to copy %d exec entries back to user\n",
+				  args->buffer_count);
+			return -EFAULT;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Legacy execbuffer just creates an exec2 list from the original exec object
  * list array and passes it to the real function.
@@ -1681,24 +1735,10 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 	i915_execbuffer2_set_context_id(exec2, 0);
 
 	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
-	if (!ret) {
-		struct drm_i915_gem_exec_object __user *user_exec_list =
-			to_user_ptr(args->buffers_ptr);
-
-		/* Copy the new buffer offsets back to the user's exec list. */
-		for (i = 0; i < args->buffer_count; i++) {
-			ret = __copy_to_user(&user_exec_list[i].offset,
-					     &exec2_list[i].offset,
-					     sizeof(user_exec_list[i].offset));
-			if (ret) {
-				ret = -EFAULT;
-				DRM_DEBUG("failed to copy %d exec entries "
-					  "back to user (%d)\n",
-					  args->buffer_count, ret);
-				break;
-			}
-		}
-	}
+
+	/* Copy the new buffer offsets back to the user's exec list. */
+	if (!ret)
+		ret = __i915_gem_execlist_copy_to_user(&exec2, exec2_list);
 
 	drm_free_large(exec_list);
 	drm_free_large(exec2_list);
@@ -1745,25 +1785,10 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 	}
 
 	ret = i915_gem_do_execbuffer(dev, data, file, args, exec2_list);
-	if (!ret) {
-		/* Copy the new buffer offsets back to the user's exec list. */
-		struct drm_i915_gem_exec_object2 __user *user_exec_list =
-				   to_user_ptr(args->buffers_ptr);
-		int i;
-
-		for (i = 0; i < args->buffer_count; i++) {
-			ret = __copy_to_user(&user_exec_list[i].offset,
-					     &exec2_list[i].offset,
-					     sizeof(user_exec_list[i].offset));
-			if (ret) {
-				ret = -EFAULT;
-				DRM_DEBUG("failed to copy %d exec entries "
-					  "back to user\n",
-					  args->buffer_count);
-				break;
-			}
-		}
-	}
+
+	/* Copy the new buffer offsets back to the user's exec list. */
+	if (!ret)
+		ret = __i915_gem_execlist_copy_to_user(args, exec2_list);
 
 	drm_free_large(exec2_list);
 	return ret;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Winiarski, Michal]

On Tue, 2015-12-22 at 12:00 +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)
> v4: Handle softpin in validate_exec_list (Chris)
> v5: Convert back to canonical form at copy_to_user time (Chris)

Please ignore this one, I'm breaking old execbuffer here.

> 
> 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 | 107 ++++++++++++++++++-
> ----------
>  1 file changed, 66 insertions(+), 41 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

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)
v4: Handle softpin in validate_exec_list (Chris)
v5: Convert back to canonical form at copy_to_user time (Chris)
v6: Don't use struct exec_object2 in place of exec_object

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 | 43 +++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..4e13174 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,25 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* 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 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 +275,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 +311,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 +366,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 +414,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
@@ -994,6 +1013,18 @@ validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), since
+		 * userspace has to use canonical format, we need to reject all
+		 * non-canonical addresses.
+		 */
+		if (exec[i].offset != gen8_canonical_addr(exec[i].offset))
+			return -EINVAL;
+
+		/* On the other hand, from drm_mm perspective address space is
+		 * continuous, so we're converting to non-canonical form
+		 */
+		exec[i].offset &= (1ULL << 48) - 1;
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;
 
@@ -1687,6 +1718,8 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 
 		/* Copy the new buffer offsets back to the user's exec list. */
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
@@ -1752,6 +1785,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		int i;
 
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Tue, Dec 22, 2015 at 01:37:10PM +0100, Michał Winiarski wrote:
> +		/* Offset can be used as input (EXEC_OBJECT_PINNED), since
> +		 * userspace has to use canonical format, we need to reject all
> +		 * non-canonical addresses.
> +		 */
> +		if (exec[i].offset != gen8_canonical_addr(exec[i].offset))
> +			return -EINVAL;
> +
> +		/* On the other hand, from drm_mm perspective address space is
> +		 * continuous, so we're converting to non-canonical form
> +		 */
> +		exec[i].offset &= (1ULL << 48) - 1;

I'm still dubious about making this ABI change backwards. Only
EXEC_OBJECT_PINNED uses this as an input and in the past we have stuffed
bad values in here to force relocations etc.

Also note that EXEC_OBJECT_PINNED doesn't do if (exec[i].offset & 4095)
return -EINVAL;

I would address both at once.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v7] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

According to PRM, 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)
v4: Handle softpin in validate_exec_list (Chris)
v5: Convert back to canonical form at copy_to_user time (Chris)
v6: Don't use struct exec_object2 in place of exec_object
v7: Use sign_extend64 for converting to canonical form (Joonas),
    reject non-canonical and non-page-aligned offset for softpin (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_execbuffer.c | 45 +++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..dd081fd 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,25 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* 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 sign_extend64(address, 47);
+}
+
+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 +275,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 +311,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 +366,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 +414,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
@@ -994,6 +1013,20 @@ validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
+		 * any non-page-aligned or non-canonial addresses.
+		 */
+		if (exec[i].flags & EXEC_OBJECT_PINNED &&
+		    (exec[i].offset != gen8_canonical_addr(exec[i].offset) ||
+		     offset_in_page(exec[i].offset)))
+			return -EINVAL;
+
+		/* From drm_mm perspective address space is continuous,
+		 * so from this point we're always using non-canonical form
+		 * internally.
+		 */
+		exec[i].offset &= (1ULL << 48) - 1;
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;
 
@@ -1687,6 +1720,8 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 
 		/* Copy the new buffer offsets back to the user's exec list. */
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
@@ -1752,6 +1787,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		int i;
 
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v7] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Tue, Dec 29, 2015 at 04:49:01PM +0100, Michał Winiarski wrote:
> @@ -994,6 +1013,20 @@ validate_exec_list(struct drm_device *dev,
>  		if (exec[i].flags & invalid_flags)
>  			return -EINVAL;
>  
> +		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
> +		 * any non-page-aligned or non-canonial addresses.
> +		 */
> +		if (exec[i].flags & EXEC_OBJECT_PINNED &&
> +		    (exec[i].offset != gen8_canonical_addr(exec[i].offset) ||
> +		     offset_in_page(exec[i].offset)))
> +			return -EINVAL;
> +

if (exec[i].flags & EXEC_OBJECT_PINNED) {
	if (exec[i].offset != gen8_canonical_addr(exec[i].offset & PAGE_MASK))
		return -EINVAL;

	/* From drm_mm perspective address space is continuous,
	 * so from this point we're always using non-canonical form
	 * internally.
	 */
	exec[i].offset &= (1ULL << 48) - 1;
}

Splitting up the two tests just makes it a bit easier to read (imo, and
I've been told on numerous occasions to do the same :) Whilst not as
obvious atm, it also helps when we have multiple extension checks in the
validate(). As a secondary point, we can then also demonstate that we
can fully restrict manipulating exec[i].offset to the pinned path.

#define GEN8_HIGH_ADDRESS_BIT 47
#define GEN8_ADDRESS_MASK (1ULL << (GEN8_HIGH_ADDRESS_BIT+1)) - 1

GEN8_CANONICAL_HIGH_BIT ?

since we have two places now that know about the address format, or
perhaps

static u64 gen8_undo_canonical_addr(u64);
exec[i].offset = gen8_undo_canonical_addr(exec[i].offset);

so that we can put them next to each other. That seems a better idea.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

According to PRM, 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)
v4: Handle softpin in validate_exec_list (Chris)
v5: Convert back to canonical form at copy_to_user time (Chris)
v6: Don't use struct exec_object2 in place of exec_object
v7: Use sign_extend64 for converting to canonical form (Joonas),
    reject non-canonical and non-page-aligned offset for softpin (Chris)
v8: Convert back to non-canonical form in a function,
    split the test for EXEC_OBJECT_PINNED (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_execbuffer.c | 53 +++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..97fb9af 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,31 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* 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]."
+ */
+#define GEN8_HIGH_ADDRESS_BIT 47
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return sign_extend64(address, GEN8_HIGH_ADDRESS_BIT);
+}
+
+static inline uint64_t gen8_noncanonical_addr(uint64_t address)
+{
+	return address & ((1ULL << (GEN8_HIGH_ADDRESS_BIT + 1)) - 1);
+}
+
+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 +281,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 +317,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 +372,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 +420,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
@@ -994,6 +1019,22 @@ validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
+		 * any non-page-aligned or non-canonial addresses.
+		 */
+		if (exec[i].flags & EXEC_OBJECT_PINNED) {
+			if (exec[i].offset !=
+			    gen8_canonical_addr(exec[i].offset & PAGE_MASK))
+				return -EINVAL;
+
+			/* From drm_mm perspective address space is continuous,
+			 * so from this point we're always using non-canonical
+			 * form internally.
+			 */
+			exec[i].offset = gen8_noncanonical_addr(exec[i].offset);
+		}
+
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;
 
@@ -1687,6 +1728,8 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 
 		/* Copy the new buffer offsets back to the user's exec list. */
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
@@ -1752,6 +1795,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		int i;
 
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v9] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Michał Winiarski]

According to PRM, 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)
v4: Handle softpin in validate_exec_list (Chris)
v5: Convert back to canonical form at copy_to_user time (Chris)
v6: Don't use struct exec_object2 in place of exec_object
v7: Use sign_extend64 for converting to canonical form (Joonas),
    reject non-canonical and non-page-aligned offset for softpin (Chris)
v8: Convert back to non-canonical form in a function,
    split the test for EXEC_OBJECT_PINNED (Chris)
v9: s/canonial/canonical, drop accidental double newline (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_execbuffer.c | 52 +++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5d01ea6..dccb517 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -249,6 +249,31 @@ static inline int use_cpu_reloc(struct drm_i915_gem_object *obj)
 		obj->cache_level != I915_CACHE_NONE);
 }
 
+/* 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]."
+ */
+#define GEN8_HIGH_ADDRESS_BIT 47
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return sign_extend64(address, GEN8_HIGH_ADDRESS_BIT);
+}
+
+static inline uint64_t gen8_noncanonical_addr(uint64_t address)
+{
+	return address & ((1ULL << (GEN8_HIGH_ADDRESS_BIT + 1)) - 1);
+}
+
+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 +281,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 +317,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 +372,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 +420,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
@@ -994,6 +1019,21 @@ validate_exec_list(struct drm_device *dev,
 		if (exec[i].flags & invalid_flags)
 			return -EINVAL;
 
+		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
+		 * any non-page-aligned or non-canonical addresses.
+		 */
+		if (exec[i].flags & EXEC_OBJECT_PINNED) {
+			if (exec[i].offset !=
+			    gen8_canonical_addr(exec[i].offset & PAGE_MASK))
+				return -EINVAL;
+
+			/* From drm_mm perspective address space is continuous,
+			 * so from this point we're always using non-canonical
+			 * form internally.
+			 */
+			exec[i].offset = gen8_noncanonical_addr(exec[i].offset);
+		}
+
 		if (exec[i].alignment && !is_power_of_2(exec[i].alignment))
 			return -EINVAL;
 
@@ -1687,6 +1727,8 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
 
 		/* Copy the new buffer offsets back to the user's exec list. */
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
@@ -1752,6 +1794,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		int i;
 
 		for (i = 0; i < args->buffer_count; i++) {
+			exec2_list[i].offset =
+				gen8_canonical_addr(exec2_list[i].offset);
 			ret = __copy_to_user(&user_exec_list[i].offset,
 					     &exec2_list[i].offset,
 					     sizeof(user_exec_list[i].offset));
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Tue, Dec 29, 2015 at 06:24:52PM +0100, Michał Winiarski wrote:
> According to PRM, 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)
> v4: Handle softpin in validate_exec_list (Chris)
> v5: Convert back to canonical form at copy_to_user time (Chris)
> v6: Don't use struct exec_object2 in place of exec_object
> v7: Use sign_extend64 for converting to canonical form (Joonas),
>     reject non-canonical and non-page-aligned offset for softpin (Chris)
> v8: Convert back to non-canonical form in a function,
>     split the test for EXEC_OBJECT_PINNED (Chris)
> v9: s/canonial/canonical, drop accidental double newline (Chris)
 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92699
> 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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Daniel Vetter]

On Tue, Dec 29, 2015 at 05:28:04PM +0000, Chris Wilson wrote:
> On Tue, Dec 29, 2015 at 06:24:52PM +0100, Michał Winiarski wrote:
> > According to PRM, 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)
> > v4: Handle softpin in validate_exec_list (Chris)
> > v5: Convert back to canonical form at copy_to_user time (Chris)
> > v6: Don't use struct exec_object2 in place of exec_object
> > v7: Use sign_extend64 for converting to canonical form (Joonas),
> >     reject non-canonical and non-page-aligned offset for softpin (Chris)
> > v8: Convert back to non-canonical form in a function,
> >     split the test for EXEC_OBJECT_PINNED (Chris)
> > v9: s/canonial/canonical, drop accidental double newline (Chris)
>  
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92699
> > 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, with Testcase: and Cc: -fixes added, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson]

On Tue, Dec 29, 2015 at 06:14:19PM +0100, Michał Winiarski wrote:
> According to PRM, 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)
> v4: Handle softpin in validate_exec_list (Chris)
> v5: Convert back to canonical form at copy_to_user time (Chris)
> v6: Don't use struct exec_object2 in place of exec_object
> v7: Use sign_extend64 for converting to canonical form (Joonas),
>     reject non-canonical and non-page-aligned offset for softpin (Chris)
> v8: Convert back to non-canonical form in a function,
>     split the test for EXEC_OBJECT_PINNED (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>

I double checked that all the roundtrips between kernel<->userspace
should be idempotent, so

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Out of curiousity, you should not see any difference in runtime of
gem_exec_lut_handle.

There should also be a Testcase: and perhaps a
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92699

> +		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
> +		 * any non-page-aligned or non-canonial addresses.
                                           ^non-canonical
> +		 */
> +		if (exec[i].flags & EXEC_OBJECT_PINNED) {
> +			if (exec[i].offset !=
> +			    gen8_canonical_addr(exec[i].offset & PAGE_MASK))
> +				return -EINVAL;
> +
> +			/* From drm_mm perspective address space is continuous,
> +			 * so from this point we're always using non-canonical
> +			 * form internally.
> +			 */
> +			exec[i].offset = gen8_noncanonical_addr(exec[i].offset);
> +		}
> +
> +
  ^ extra newline
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 7cdc548e77f503593b83a1c5d58f4dcc862c17e2 drm-intel-nightly: 2015y-12m-18d-19h-26m-21s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (hsw-brixbox)
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (byt-nuc)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-a-frame-sequence:
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup read-crc-pipe-c:
                pass       -> DMESG-WARN (bdw-ultra)
                dmesg-warn -> PASS       (skl-i7k-2)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                dmesg-warn -> PASS       (snb-dellxps)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> DMESG-FAIL (snb-x220t)
                dmesg-warn -> PASS       (bdw-ultra)

bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
byt-nuc          total:135  pass:120  dwarn:2   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:99   dwarn:1   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:121  dwarn:6   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:122  dwarn:5   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:122  dwarn:1   dfail:1   fail:0   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_725/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 9d34680058127d4d628a78dab44cd6ecb62e97c6 drm-intel-nightly: 2015y-12m-22d-09h-40m-43s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (hsw-xps12)
                dmesg-warn -> PASS       (hsw-brixbox)
                dmesg-warn -> PASS       (bdw-nuci7)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t)
                pass       -> DMESG-WARN (skl-i7k-2)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)

bdw-nuci7        total:132  pass:122  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:120  dwarn:2   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
hsw-xps12        total:132  pass:126  dwarn:2   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:121  dwarn:6   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:121  dwarn:6   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_790/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 7e671e69deffb88d60687dacffe6e34a5d046500 drm-intel-nightly: 2015y-12m-22d-13h-28m-34s UTC integration manifest

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (skl-i5k-2)
                dmesg-warn -> PASS       (bsw-nuc-2)
                dmesg-warn -> PASS       (hsw-xps12)
                pass       -> DMESG-WARN (hsw-brixbox)
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (snb-x220t)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (skl-i5k-2)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (bdw-ultra)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (bdw-ultra)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (hsw-xps12)

bdw-nuci7        total:132  pass:122  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:119  dwarn:3   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:126  dwarn:2   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
hsw-xps12        total:132  pass:125  dwarn:3   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:99   dwarn:1   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:124  dwarn:3   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:124  dwarn:3   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_791/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on ec0382c73cb1adc972bebdd94afad3f0ea117114 drm-intel-nightly: 2015y-12m-23d-22h-28m-25s UTC integration manifest

Test gem_ctx_param_basic:
        Subgroup root-set:
                skip       -> PASS       (ilk-hp8440p)
Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (bdw-nuci7)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (skl-i5k-2)
                pass       -> DMESG-WARN (hsw-xps12)
                pass       -> DMESG-WARN (hsw-brixbox)
                pass       -> DMESG-WARN (bdw-nuci7)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (snb-x220t)
                pass       -> DMESG-WARN (snb-dellxps)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
                pass       -> DMESG-WARN (skl-i7k-2)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (byt-nuc)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                dmesg-warn -> PASS       (snb-dellxps)
Test pm_rpm:
        Subgroup basic-rte:
                dmesg-warn -> PASS       (hsw-xps12)
                pass       -> DMESG-WARN (byt-nuc)

bdw-nuci7        total:132  pass:121  dwarn:2   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:113  dwarn:2   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:119  dwarn:3   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:126  dwarn:2   dfail:0   fail:0   skip:7  
hsw-xps12        total:132  pass:125  dwarn:3   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:101  dwarn:0   dfail:0   fail:0   skip:34 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:124  dwarn:3   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

HANGED hsw-gt2 in igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c

Results at /archive/results/CI_IGT_test/Patchwork_945/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on ec0382c73cb1adc972bebdd94afad3f0ea117114 drm-intel-nightly: 2015y-12m-23d-22h-28m-25s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (bdw-nuci7)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (bdw-nuci7)
                pass       -> DMESG-WARN (byt-nuc)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (skl-i7k-2)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                dmesg-warn -> PASS       (snb-dellxps)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (byt-nuc)

bdw-nuci7        total:132  pass:121  dwarn:2   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:113  dwarn:2   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:119  dwarn:3   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_948/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx