[PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[Dave Gordon]

Two different sets of flag bits are stored in the 'flags' member of a
'struct drm_i915_gem_exec_object2', and they're defined in two different
source files, increasing the risk of an accidental clash.

Some flags in this field are supplied by the user; these are defined in
i915_drm.h, and they start from the LSB and work up.

Other flags are defined in i915_gem_execbuffer, for internal use within
that file only; they start from the MSB and work down.

So here we add a compile-time check that the two sets of flags do not
overlap, which would cause all sorts of confusion.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++++++++----
 include/uapi/drm/i915_drm.h                | 11 ++++++-----
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7941f1f..608fdc4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -34,10 +34,11 @@
 #include <linux/dma_remapping.h>
 #include <linux/uaccess.h>
 
-#define  __EXEC_OBJECT_HAS_PIN (1<<31)
-#define  __EXEC_OBJECT_HAS_FENCE (1<<30)
-#define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
-#define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
+#define  __EXEC_OBJECT_HAS_PIN		(1<<31)
+#define  __EXEC_OBJECT_HAS_FENCE	(1<<30)
+#define  __EXEC_OBJECT_NEEDS_MAP	(1<<29)
+#define  __EXEC_OBJECT_NEEDS_BIAS	(1<<28)
+#define  __EXEC_OBJECT_INTERNAL_FLAGS (0xf<<28) /* all of the above */
 
 #define BATCH_OFFSET_BIAS (256*1024)
 
@@ -1007,6 +1008,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	unsigned invalid_flags;
 	int i;
 
+	/* INTERNAL flags must not overlap with external ones */
+	BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & ~__EXEC_OBJECT_UNKNOWN_FLAGS);
+
 	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
 	if (USES_FULL_PPGTT(dev))
 		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index c17d63d..079d274 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -696,12 +696,13 @@ struct drm_i915_gem_exec_object2 {
 	 */
 	__u64 offset;
 
-#define EXEC_OBJECT_NEEDS_FENCE (1<<0)
-#define EXEC_OBJECT_NEEDS_GTT	(1<<1)
-#define EXEC_OBJECT_WRITE	(1<<2)
+#define EXEC_OBJECT_NEEDS_FENCE		 (1<<0)
+#define EXEC_OBJECT_NEEDS_GTT		 (1<<1)
+#define EXEC_OBJECT_WRITE		 (1<<2)
 #define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
-#define EXEC_OBJECT_PINNED	(1<<4)
-#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_PINNED<<1)
+#define EXEC_OBJECT_PINNED		 (1<<4)
+/* All remaining bits are MBZ and RESERVED FOR FUTURE USE */
+#define __EXEC_OBJECT_UNKNOWN_FLAGS	(-(EXEC_OBJECT_PINNED<<1))
 	__u64 flags;
 
 	__u64 rsvd1;
-- 
1.9.1

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

[PATCH 2/2] drm/i915: refactor eb_get_batch()

[Dave Gordon]

Precursor for fix to secure batch execution. We will need to be able to
retrieve the batch VMA (as well as the batch itself) from the eb list,
so this patch extracts that part of eb_get_batch() into a separate
function, and moves both parts to a more logical place in the file, near
where the eb list is created.

Also, it may not be obvious, but the current execbuffer2 ioctl interface
requires that the buffer object containing the batch-to-be-executed be
the LAST entry in the exec2_list[] array (I expected it to be the first!).

To clarify this, we can replace the rather obscure construct
	"list_entry(eb->vmas.prev, ...)"
in the old version of eb_get_batch() with the equivalent but more explicit
	"list_last_entry(&eb->vmas,...)"
in the new eb_get_batch_vma() and of course add an explanatory comment.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++++++++++++++++++------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 608fdc4..eea8b1f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -186,6 +186,35 @@ struct eb_vmas {
 	return ret;
 }
 
+static inline struct i915_vma *
+eb_get_batch_vma(struct eb_vmas *eb)
+{
+	/* The batch is always the LAST item in the VMA list */
+	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
+
+	return vma;
+}
+
+static struct drm_i915_gem_object *
+eb_get_batch(struct eb_vmas *eb)
+{
+	struct i915_vma *vma = eb_get_batch_vma(eb);
+
+	/*
+	 * SNA is doing fancy tricks with compressing batch buffers, which leads
+	 * to negative relocation deltas. Usually that works out ok since the
+	 * relocate address is still positive, except when the batch is placed
+	 * very low in the GTT. Ensure this doesn't happen.
+	 *
+	 * Note that actual hangs have only been observed on gen7, but for
+	 * paranoia do it everywhere.
+	 */
+	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
+		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
+
+	return vma->obj;
+}
+
 static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle)
 {
 	if (eb->and < 0) {
@@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	return file_priv->bsd_ring;
 }
 
-static struct drm_i915_gem_object *
-eb_get_batch(struct eb_vmas *eb)
-{
-	struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list);
-
-	/*
-	 * SNA is doing fancy tricks with compressing batch buffers, which leads
-	 * to negative relocation deltas. Usually that works out ok since the
-	 * relocate address is still positive, except when the batch is placed
-	 * very low in the GTT. Ensure this doesn't happen.
-	 *
-	 * Note that actual hangs have only been observed on gen7, but for
-	 * paranoia do it everywhere.
-	 */
-	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
-		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
-
-	return vma->obj;
-}
-
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
-- 
1.9.1

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

✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
URL   : https://patchwork.freedesktop.org/series/9325/
State : warning

== Summary ==

Series 9325v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9325/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> SKIP       (fi-skl-i5-6260u)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> SKIP       (fi-skl-i5-6260u)
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i7-5557U)

fi-kbl-qkkr      total:229  pass:160  dwarn:28  dfail:1   fail:0   skip:40 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:0   skip:27 
fi-skl-i7-6700k  total:229  pass:190  dwarn:0   dfail:0   fail:0   skip:39 
fi-snb-i7-2600   total:229  pass:176  dwarn:0   dfail:0   fail:0   skip:53 
ro-bdw-i5-5250u  total:229  pass:204  dwarn:1   dfail:1   fail:0   skip:23 
ro-bdw-i7-5557U  total:229  pass:204  dwarn:2   dfail:1   fail:0   skip:22 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:180  dwarn:0   dfail:1   fail:3   skip:45 
ro-hsw-i3-4010u  total:229  pass:197  dwarn:0   dfail:1   fail:0   skip:31 
ro-hsw-i7-4770r  total:229  pass:197  dwarn:0   dfail:1   fail:0   skip:31 
ro-ilk-i7-620lm  total:229  pass:157  dwarn:0   dfail:1   fail:1   skip:70 
ro-ilk1-i5-650   total:224  pass:157  dwarn:0   dfail:1   fail:1   skip:65 
ro-ivb-i7-3770   total:229  pass:188  dwarn:0   dfail:1   fail:0   skip:40 
ro-ivb2-i7-3770  total:229  pass:192  dwarn:0   dfail:1   fail:0   skip:36 
ro-skl3-i5-6260u total:229  pass:208  dwarn:1   dfail:1   fail:0   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 
fi-hsw-i7-4770k failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1344/

51ba6a7 drm-intel-nightly: 2016y-06m-30d-15h-06m-59s UTC integration manifest
f81c533 drm/i915: refactor eb_get_batch()
ec09fb7 drm/i915: compile-time consistency check on __EXEC_OBJECT flags

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

Re: [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

[Daniel Vetter]

On Thu, Jun 30, 2016 at 04:12:48PM +0100, Dave Gordon wrote:
> Two different sets of flag bits are stored in the 'flags' member of a
> 'struct drm_i915_gem_exec_object2', and they're defined in two different
> source files, increasing the risk of an accidental clash.
> 
> Some flags in this field are supplied by the user; these are defined in
> i915_drm.h, and they start from the LSB and work up.
> 
> Other flags are defined in i915_gem_execbuffer, for internal use within
> that file only; they start from the MSB and work down.
> 
> So here we add a compile-time check that the two sets of flags do not
> overlap, which would cause all sorts of confusion.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++++++++----
>  include/uapi/drm/i915_drm.h                | 11 ++++++-----
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7941f1f..608fdc4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -34,10 +34,11 @@
>  #include <linux/dma_remapping.h>
>  #include <linux/uaccess.h>
>  
> -#define  __EXEC_OBJECT_HAS_PIN (1<<31)
> -#define  __EXEC_OBJECT_HAS_FENCE (1<<30)
> -#define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
> -#define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
> +#define  __EXEC_OBJECT_HAS_PIN		(1<<31)
> +#define  __EXEC_OBJECT_HAS_FENCE	(1<<30)
> +#define  __EXEC_OBJECT_NEEDS_MAP	(1<<29)
> +#define  __EXEC_OBJECT_NEEDS_BIAS	(1<<28)
> +#define  __EXEC_OBJECT_INTERNAL_FLAGS (0xf<<28) /* all of the above */

s/(0xf<<28)/(~(__EXEC_OBJECT_NEEDS_BIAS-1))/ if you feel like, with the
comment retained. Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  #define BATCH_OFFSET_BIAS (256*1024)
>  
> @@ -1007,6 +1008,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
>  	unsigned invalid_flags;
>  	int i;
>  
> +	/* INTERNAL flags must not overlap with external ones */
> +	BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & ~__EXEC_OBJECT_UNKNOWN_FLAGS);
> +
>  	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
>  	if (USES_FULL_PPGTT(dev))
>  		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index c17d63d..079d274 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -696,12 +696,13 @@ struct drm_i915_gem_exec_object2 {
>  	 */
>  	__u64 offset;
>  
> -#define EXEC_OBJECT_NEEDS_FENCE (1<<0)
> -#define EXEC_OBJECT_NEEDS_GTT	(1<<1)
> -#define EXEC_OBJECT_WRITE	(1<<2)
> +#define EXEC_OBJECT_NEEDS_FENCE		 (1<<0)
> +#define EXEC_OBJECT_NEEDS_GTT		 (1<<1)
> +#define EXEC_OBJECT_WRITE		 (1<<2)
>  #define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
> -#define EXEC_OBJECT_PINNED	(1<<4)
> -#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_PINNED<<1)
> +#define EXEC_OBJECT_PINNED		 (1<<4)
> +/* All remaining bits are MBZ and RESERVED FOR FUTURE USE */
> +#define __EXEC_OBJECT_UNKNOWN_FLAGS	(-(EXEC_OBJECT_PINNED<<1))
>  	__u64 flags;
>  
>  	__u64 rsvd1;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Daniel Vetter]

On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
> Precursor for fix to secure batch execution. We will need to be able to
> retrieve the batch VMA (as well as the batch itself) from the eb list,
> so this patch extracts that part of eb_get_batch() into a separate
> function, and moves both parts to a more logical place in the file, near
> where the eb list is created.
> 
> Also, it may not be obvious, but the current execbuffer2 ioctl interface
> requires that the buffer object containing the batch-to-be-executed be
> the LAST entry in the exec2_list[] array (I expected it to be the first!).
> 
> To clarify this, we can replace the rather obscure construct
> 	"list_entry(eb->vmas.prev, ...)"
> in the old version of eb_get_batch() with the equivalent but more explicit
> 	"list_last_entry(&eb->vmas,...)"
> in the new eb_get_batch_vma() and of course add an explanatory comment.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

I have no context on the secure batch fix you're talking about, but this
here makes sense as an independent cleanup.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 608fdc4..eea8b1f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -186,6 +186,35 @@ struct eb_vmas {
>  	return ret;
>  }
>  
> +static inline struct i915_vma *
> +eb_get_batch_vma(struct eb_vmas *eb)
> +{
> +	/* The batch is always the LAST item in the VMA list */
> +	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
> +
> +	return vma;
> +}
> +
> +static struct drm_i915_gem_object *
> +eb_get_batch(struct eb_vmas *eb)
> +{
> +	struct i915_vma *vma = eb_get_batch_vma(eb);
> +
> +	/*
> +	 * SNA is doing fancy tricks with compressing batch buffers, which leads
> +	 * to negative relocation deltas. Usually that works out ok since the
> +	 * relocate address is still positive, except when the batch is placed
> +	 * very low in the GTT. Ensure this doesn't happen.
> +	 *
> +	 * Note that actual hangs have only been observed on gen7, but for
> +	 * paranoia do it everywhere.
> +	 */
> +	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
> +		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
> +
> +	return vma->obj;
> +}
> +
>  static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle)
>  {
>  	if (eb->and < 0) {
> @@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags)
>  	return file_priv->bsd_ring;
>  }
>  
> -static struct drm_i915_gem_object *
> -eb_get_batch(struct eb_vmas *eb)
> -{
> -	struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list);
> -
> -	/*
> -	 * SNA is doing fancy tricks with compressing batch buffers, which leads
> -	 * to negative relocation deltas. Usually that works out ok since the
> -	 * relocate address is still positive, except when the batch is placed
> -	 * very low in the GTT. Ensure this doesn't happen.
> -	 *
> -	 * Note that actual hangs have only been observed on gen7, but for
> -	 * paranoia do it everywhere.
> -	 */
> -	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
> -		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
> -
> -	return vma->obj;
> -}
> -
>  #define I915_USER_RINGS (4)
>  
>  static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Chris Wilson]

On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote:
> On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
> > Precursor for fix to secure batch execution. We will need to be able to
> > retrieve the batch VMA (as well as the batch itself) from the eb list,
> > so this patch extracts that part of eb_get_batch() into a separate
> > function, and moves both parts to a more logical place in the file, near
> > where the eb list is created.
> > 
> > Also, it may not be obvious, but the current execbuffer2 ioctl interface
> > requires that the buffer object containing the batch-to-be-executed be
> > the LAST entry in the exec2_list[] array (I expected it to be the first!).
> > 
> > To clarify this, we can replace the rather obscure construct
> > 	"list_entry(eb->vmas.prev, ...)"
> > in the old version of eb_get_batch() with the equivalent but more explicit
> > 	"list_last_entry(&eb->vmas,...)"
> > in the new eb_get_batch_vma() and of course add an explanatory comment.
> > 
> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> 
> I have no context on the secure batch fix you're talking about, but this
> here makes sense as an independent cleanup.

It won't help though, so this is just churn for no purpose.
-Chris

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

Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Dave Gordon]

On 13/07/16 13:44, Chris Wilson wrote:
> On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote:
>> On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
>>> Precursor for fix to secure batch execution. We will need to be able to
>>> retrieve the batch VMA (as well as the batch itself) from the eb list,
>>> so this patch extracts that part of eb_get_batch() into a separate
>>> function, and moves both parts to a more logical place in the file, near
>>> where the eb list is created.
>>>
>>> Also, it may not be obvious, but the current execbuffer2 ioctl interface
>>> requires that the buffer object containing the batch-to-be-executed be
>>> the LAST entry in the exec2_list[] array (I expected it to be the first!).
>>>
>>> To clarify this, we can replace the rather obscure construct
>>> 	"list_entry(eb->vmas.prev, ...)"
>>> in the old version of eb_get_batch() with the equivalent but more explicit
>>> 	"list_last_entry(&eb->vmas,...)"
>>> in the new eb_get_batch_vma() and of course add an explanatory comment.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>
>> I have no context on the secure batch fix you're talking about, but this
>> here makes sense as an independent cleanup.
>
> It won't help though, so this is just churn for no purpose.
> -Chris

At the very least, it replaces a confusing construct with
a comprehensible one annotated with an explanatory comment.

Separating finding the VMA for the batch from finding the batch itself
also improves clarity and costs nothing (compiler inlines it anyway).

Comprehensibility -- and hence maintainability -- is always
a worthwhile purpose :)

BTW, do the comments in this code from patch

d23db88 drm/i915: Prevent negative relocation deltas from wrapping

still apply? 'Cos I think it's pretty ugly to be setting a flag
on a VMA as a side-effect of a "lookup" type operation :( Surely
cleaner to do that sort of think at the top level i.e. inside
i915_gem_do_execbuffer() ?

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

Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Chris Wilson]

On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote:
> On 13/07/16 13:44, Chris Wilson wrote:
> >On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote:
> >>On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
> >>>Precursor for fix to secure batch execution. We will need to be able to
> >>>retrieve the batch VMA (as well as the batch itself) from the eb list,
> >>>so this patch extracts that part of eb_get_batch() into a separate
> >>>function, and moves both parts to a more logical place in the file, near
> >>>where the eb list is created.
> >>>
> >>>Also, it may not be obvious, but the current execbuffer2 ioctl interface
> >>>requires that the buffer object containing the batch-to-be-executed be
> >>>the LAST entry in the exec2_list[] array (I expected it to be the first!).
> >>>
> >>>To clarify this, we can replace the rather obscure construct
> >>>	"list_entry(eb->vmas.prev, ...)"
> >>>in the old version of eb_get_batch() with the equivalent but more explicit
> >>>	"list_last_entry(&eb->vmas,...)"
> >>>in the new eb_get_batch_vma() and of course add an explanatory comment.
> >>>
> >>>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >>
> >>I have no context on the secure batch fix you're talking about, but this
> >>here makes sense as an independent cleanup.
> >
> >It won't help though, so this is just churn for no purpose.
> >-Chris
> 
> At the very least, it replaces a confusing construct with
> a comprehensible one annotated with an explanatory comment.

No. It deepens a confusion in the code that I've been trying to get
removed over the last couple of years.
 
> Separating finding the VMA for the batch from finding the batch itself
> also improves clarity and costs nothing (compiler inlines it anyway).

No. That's the confusion you have here. The object is irrelevant.
 
> Comprehensibility -- and hence maintainability -- is always
> a worthwhile purpose :)

s/comprehensibility/greater confusion/

> BTW, do the comments in this code from patch
> 
> d23db88 drm/i915: Prevent negative relocation deltas from wrapping
> 
> still apply? 'Cos I think it's pretty ugly to be setting a flag
> on a VMA as a side-effect of a "lookup" type operation :( Surely
> cleaner to do that sort of think at the top level i.e. inside
> i915_gem_do_execbuffer() ?

The comment is wrong since the practice is more widespread and it is
a particular hw bug on Ivybridge.
-Chris

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

Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Dave Gordon]

On 14/07/16 15:03, Chris Wilson wrote:
> On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote:
>> On 13/07/16 13:44, Chris Wilson wrote:
>>> On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote:
>>>> On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
>>>>> Precursor for fix to secure batch execution. We will need to be able to
>>>>> retrieve the batch VMA (as well as the batch itself) from the eb list,
>>>>> so this patch extracts that part of eb_get_batch() into a separate
>>>>> function, and moves both parts to a more logical place in the file, near
>>>>> where the eb list is created.
>>>>>
>>>>> Also, it may not be obvious, but the current execbuffer2 ioctl interface
>>>>> requires that the buffer object containing the batch-to-be-executed be
>>>>> the LAST entry in the exec2_list[] array (I expected it to be the first!).
>>>>>
>>>>> To clarify this, we can replace the rather obscure construct
>>>>> 	"list_entry(eb->vmas.prev, ...)"
>>>>> in the old version of eb_get_batch() with the equivalent but more explicit
>>>>> 	"list_last_entry(&eb->vmas,...)"
>>>>> in the new eb_get_batch_vma() and of course add an explanatory comment.
>>>>>
>>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>>>
>>>> I have no context on the secure batch fix you're talking about, but this
>>>> here makes sense as an independent cleanup.
>>>
>>> It won't help though, so this is just churn for no purpose.
>>> -Chris
>>
>> At the very least, it replaces a confusing construct with
>> a comprehensible one annotated with an explanatory comment.
>
> No. It deepens a confusion in the code that I've been trying to get
> removed over the last couple of years.

?

I was referring to the list_{last_}entry() change. That's definitely a 
clarification as to how things work now. Of course, if you're planning 
to make the batch the first object rather than the last, I won't object. 
But whichever it is, let's use the most-appropriately-named of the 
available list functions when we pick an item from a list. And comment 
why or what it's doing.

>> Separating finding the VMA for the batch from finding the batch itself
>> also improves clarity and costs nothing (compiler inlines it anyway).
>
> No. That's the confusion you have here. The object is irrelevant.

Ah, so we have a function to return an irrelevant object. Let's just 
delete it then ;)

Do you think we /should/ just get rid of eb_get_batch()? Maybe just
have eb_get_batch_vma() return the VMA to the [single] caller
i915_gem_do_execbuffer() instead, and then have /that/ do both
the flag-setting ugliness and the indirection to the object (which
evidently is not irrelevant to it) ?

>> Comprehensibility -- and hence maintainability -- is always
>> a worthwhile purpose :)
>
> s/comprehensibility/greater confusion/

Spoken like a true Discordian ;)

> > BTW, do the comments in this code from patch
>>
>> d23db88 drm/i915: Prevent negative relocation deltas from wrapping
>>
>> still apply? 'Cos I think it's pretty ugly to be setting a flag
>> on a VMA as a side-effect of a "lookup" type operation :( Surely
>> cleaner to do that sort of think at the top level i.e. inside
>> i915_gem_do_execbuffer() ?
>
> The comment is wrong since the practice is more widespread and it is
> a particular hw bug on Ivybridge.
> -Chris

Another reason to move it out to the caller and update the comments in 
the process!

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

Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()

[Chris Wilson]

On Fri, Jul 15, 2016 at 09:03:40AM +0100, Dave Gordon wrote:
> On 14/07/16 15:03, Chris Wilson wrote:
> >On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote:
> >>On 13/07/16 13:44, Chris Wilson wrote:
> >>>On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote:
> >>>>On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote:
> >>>>>Precursor for fix to secure batch execution. We will need to be able to
> >>>>>retrieve the batch VMA (as well as the batch itself) from the eb list,
> >>>>>so this patch extracts that part of eb_get_batch() into a separate
> >>>>>function, and moves both parts to a more logical place in the file, near
> >>>>>where the eb list is created.
> >>>>>
> >>>>>Also, it may not be obvious, but the current execbuffer2 ioctl interface
> >>>>>requires that the buffer object containing the batch-to-be-executed be
> >>>>>the LAST entry in the exec2_list[] array (I expected it to be the first!).
> >>>>>
> >>>>>To clarify this, we can replace the rather obscure construct
> >>>>>	"list_entry(eb->vmas.prev, ...)"
> >>>>>in the old version of eb_get_batch() with the equivalent but more explicit
> >>>>>	"list_last_entry(&eb->vmas,...)"
> >>>>>in the new eb_get_batch_vma() and of course add an explanatory comment.
> >>>>>
> >>>>>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >>>>
> >>>>I have no context on the secure batch fix you're talking about, but this
> >>>>here makes sense as an independent cleanup.
> >>>
> >>>It won't help though, so this is just churn for no purpose.
> >>>-Chris
> >>
> >>At the very least, it replaces a confusing construct with
> >>a comprehensible one annotated with an explanatory comment.
> >
> >No. It deepens a confusion in the code that I've been trying to get
> >removed over the last couple of years.
> 
> ?
> 
> I was referring to the list_{last_}entry() change. That's definitely
> a clarification as to how things work now. Of course, if you're
> planning to make the batch the first object rather than the last, I
> won't object. But whichever it is, let's use the
> most-appropriately-named of the available list functions when we
> pick an item from a list. And comment why or what it's doing.
> 
> >>Separating finding the VMA for the batch from finding the batch itself
> >>also improves clarity and costs nothing (compiler inlines it anyway).
> >
> >No. That's the confusion you have here. The object is irrelevant.
> 
> Ah, so we have a function to return an irrelevant object. Let's just
> delete it then ;)
> 
> Do you think we /should/ just get rid of eb_get_batch()? Maybe just
> have eb_get_batch_vma() return the VMA to the [single] caller
> i915_gem_do_execbuffer() instead, and then have /that/ do both
> the flag-setting ugliness and the indirection to the object (which
> evidently is not irrelevant to it) ?
> 
> >>Comprehensibility -- and hence maintainability -- is always
> >>a worthwhile purpose :)
> >
> >s/comprehensibility/greater confusion/
> 
> Spoken like a true Discordian ;)
> 
> >> BTW, do the comments in this code from patch
> >>
> >>d23db88 drm/i915: Prevent negative relocation deltas from wrapping
> >>
> >>still apply? 'Cos I think it's pretty ugly to be setting a flag
> >>on a VMA as a side-effect of a "lookup" type operation :( Surely
> >>cleaner to do that sort of think at the top level i.e. inside
> >>i915_gem_do_execbuffer() ?
> >
> >The comment is wrong since the practice is more widespread and it is
> >a particular hw bug on Ivybridge.
> >-Chris
> 
> Another reason to move it out to the caller and update the comments
> in the process!

Then review the patches that have already been posted several times on
the list to do everything above.
-Chris

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

[PATCH 2/2] drm/i915: refactor eb_get_batch()

[Dave Gordon]

Precursor for fix to secure batch execution. We will need to be able to
retrieve the batch VMA (as well as the batch itself) from the eb list,
so this patch extracts that part of eb_get_batch() into a separate
function, and moves both parts to a more logical place in the file, near
where the eb list is created.

Also, it may not be obvious, but the current execbuffer2 ioctl interface
requires that the buffer object containing the batch-to-be-executed be
the LAST entry in the exec2_list[] array (I expected it to be the first!).

To clarify this, we can replace the rather obscure construct
	"list_entry(eb->vmas.prev, ...)"
in the old version of eb_get_batch() with the equivalent but more explicit
	"list_last_entry(&eb->vmas,...)"
in the new eb_get_batch_vma() and of course add an explanatory comment.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++++++++++++++++++------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1bb1f25..f6724ae 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -186,6 +186,35 @@ struct eb_vmas {
 	return ret;
 }
 
+static inline struct i915_vma *
+eb_get_batch_vma(struct eb_vmas *eb)
+{
+	/* The batch is always the LAST item in the VMA list */
+	struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list);
+
+	return vma;
+}
+
+static struct drm_i915_gem_object *
+eb_get_batch(struct eb_vmas *eb)
+{
+	struct i915_vma *vma = eb_get_batch_vma(eb);
+
+	/*
+	 * SNA is doing fancy tricks with compressing batch buffers, which leads
+	 * to negative relocation deltas. Usually that works out ok since the
+	 * relocate address is still positive, except when the batch is placed
+	 * very low in the GTT. Ensure this doesn't happen.
+	 *
+	 * Note that actual hangs have only been observed on gen7, but for
+	 * paranoia do it everywhere.
+	 */
+	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
+		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
+
+	return vma->obj;
+}
+
 static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle)
 {
 	if (eb->and < 0) {
@@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	return file_priv->bsd_ring;
 }
 
-static struct drm_i915_gem_object *
-eb_get_batch(struct eb_vmas *eb)
-{
-	struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list);
-
-	/*
-	 * SNA is doing fancy tricks with compressing batch buffers, which leads
-	 * to negative relocation deltas. Usually that works out ok since the
-	 * relocate address is still positive, except when the batch is placed
-	 * very low in the GTT. Ensure this doesn't happen.
-	 *
-	 * Note that actual hangs have only been observed on gen7, but for
-	 * paranoia do it everywhere.
-	 */
-	if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
-		vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
-
-	return vma->obj;
-}
-
 #define I915_USER_RINGS (4)
 
 static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
-- 
1.9.1

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