* [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
@ 2016-06-30 15:12 Dave Gordon
2016-06-30 15:12 ` [PATCH 2/2] drm/i915: refactor eb_get_batch() Dave Gordon
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Dave Gordon @ 2016-06-30 15:12 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-06-30 15:12 [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Dave Gordon
@ 2016-06-30 15:12 ` Dave Gordon
2016-07-13 12:38 ` Daniel Vetter
2016-06-30 15:42 ` ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Patchwork
2016-07-13 12:35 ` [PATCH 1/2] " Daniel Vetter
2 siblings, 1 reply; 11+ messages in thread
From: Dave Gordon @ 2016-06-30 15:12 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
2016-06-30 15:12 [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Dave Gordon
2016-06-30 15:12 ` [PATCH 2/2] drm/i915: refactor eb_get_batch() Dave Gordon
@ 2016-06-30 15:42 ` Patchwork
2016-07-13 12:35 ` [PATCH 1/2] " Daniel Vetter
2 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2016-06-30 15:42 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
2016-06-30 15:12 [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Dave Gordon
2016-06-30 15:12 ` [PATCH 2/2] drm/i915: refactor eb_get_batch() Dave Gordon
2016-06-30 15:42 ` ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Patchwork
@ 2016-07-13 12:35 ` Daniel Vetter
2 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:35 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-06-30 15:12 ` [PATCH 2/2] drm/i915: refactor eb_get_batch() Dave Gordon
@ 2016-07-13 12:38 ` Daniel Vetter
2016-07-13 12:44 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:38 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-07-13 12:38 ` Daniel Vetter
@ 2016-07-13 12:44 ` Chris Wilson
2016-07-14 13:12 ` Dave Gordon
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2016-07-13 12:44 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-07-13 12:44 ` Chris Wilson
@ 2016-07-14 13:12 ` Dave Gordon
2016-07-14 14:03 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Dave Gordon @ 2016-07-14 13:12 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-07-14 13:12 ` Dave Gordon
@ 2016-07-14 14:03 ` Chris Wilson
2016-07-15 8:03 ` Dave Gordon
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2016-07-14 14:03 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-07-14 14:03 ` Chris Wilson
@ 2016-07-15 8:03 ` Dave Gordon
2016-07-19 7:16 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Dave Gordon @ 2016-07-15 8:03 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: refactor eb_get_batch()
2016-07-15 8:03 ` Dave Gordon
@ 2016-07-19 7:16 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2016-07-19 7:16 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
@ 2016-07-06 9:52 Dave Gordon
0 siblings, 0 replies; 11+ messages in thread
From: Dave Gordon @ 2016-07-06 9:52 UTC (permalink / raw)
To: intel-gfx
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 1978633..1bb1f25 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 d7e81a3..51b9360 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -698,12 +698,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-07-19 7:16 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30 15:12 [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Dave Gordon
2016-06-30 15:12 ` [PATCH 2/2] drm/i915: refactor eb_get_batch() Dave Gordon
2016-07-13 12:38 ` Daniel Vetter
2016-07-13 12:44 ` Chris Wilson
2016-07-14 13:12 ` Dave Gordon
2016-07-14 14:03 ` Chris Wilson
2016-07-15 8:03 ` Dave Gordon
2016-07-19 7:16 ` Chris Wilson
2016-06-30 15:42 ` ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags Patchwork
2016-07-13 12:35 ` [PATCH 1/2] " Daniel Vetter
2016-07-06 9:52 Dave Gordon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.