* [PATCH] drm/i915: Avoid selecting unavailable BSD2 ring
@ 2016-02-22 12:07 Gabriel Feceoru
2016-02-22 12:47 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Gabriel Feceoru @ 2016-02-22 12:07 UTC (permalink / raw)
To: intel-gfx
Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring
is not available in the HW.
Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com>
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 8fd00d2..47305ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1394,19 +1394,22 @@ eb_select_ring(struct drm_i915_private *dev_priv,
return -EINVAL;
}
- if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
+ if (user_ring_id == I915_EXEC_BSD) {
unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
+ if ((!HAS_BSD2(dev_priv) && (bsd_idx != I915_EXEC_BSD_DEFAULT)) ||
+ (bsd_idx == (I915_EXEC_BSD_RING1 | I915_EXEC_BSD_RING2))) {
+
+ DRM_DEBUG("execbuf with wrong bsd ring: %u\n",
+ bsd_idx);
+ return -EINVAL;
+ }
+
if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
- } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
- bsd_idx <= I915_EXEC_BSD_RING2) {
+ } else {
bsd_idx >>= I915_EXEC_BSD_SHIFT;
bsd_idx--;
- } else {
- DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
- bsd_idx);
- return -EINVAL;
}
*ring = &dev_priv->ring[_VCS(bsd_idx)];
--
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* ✗ Fi.CI.BAT: failure for drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-22 12:07 [PATCH] drm/i915: Avoid selecting unavailable BSD2 ring Gabriel Feceoru @ 2016-02-22 12:47 ` Patchwork 2016-02-23 10:52 ` [PATCH v2] " Gabriel Feceoru 2016-02-23 11:48 ` ✗ Fi.CI.BAT: warning for drm/i915: Avoid selecting unavailable BSD2 ring (rev2) Patchwork 2 siblings, 0 replies; 11+ messages in thread From: Patchwork @ 2016-02-22 12:47 UTC (permalink / raw) To: Feceoru, Gabriel; +Cc: intel-gfx == Summary == Series 3678v1 drm/i915: Avoid selecting unavailable BSD2 ring http://patchwork.freedesktop.org/api/1.0/series/3678/revisions/1/mbox/ Test drv_getparams_basic: Subgroup basic-eu-total: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup basic-subslice-total: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test drv_hangman: Subgroup error-state-basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Test drv_module_reload_basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Test gem_basic: Subgroup bad-close: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup create-close: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup create-fd-close: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test gem_cpu_reloc: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test gem_cs_prefetch: Subgroup basic-default: skip -> FAIL (bsw-nuc-2) skip -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) skip -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) incomplete -> FAIL (ilk-hp8440p) Test gem_cs_tlb: Subgroup basic-default: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test gem_ctx_basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Test gem_ctx_create: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Test gem_ctx_exec: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Test gem_ctx_param_basic: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup basic-default: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup invalid-ctx-get: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup invalid-ctx-set: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup invalid-param-get: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup invalid-param-set: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup invalid-size-get: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup invalid-size-set: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup non-root-set: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup non-root-set-no-zeromap: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup root-set: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup root-set-no-zeromap-disabled: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup root-set-no-zeromap-enabled: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Test gem_exec_basic: Subgroup basic-blt: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup basic-bsd: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup basic-bsd1: skip -> FAIL (bsw-nuc-2) skip -> FAIL (skl-i5k-2) skip -> FAIL (hsw-gt2) skip -> FAIL (bdw-ultra) skip -> FAIL (ivb-t430s) skip -> FAIL (snb-x220t) skip -> FAIL (snb-dellxps) Subgroup basic-bsd2: skip -> FAIL (bsw-nuc-2) skip -> FAIL (skl-i5k-2) skip -> FAIL (hsw-gt2) skip -> FAIL (bdw-ultra) skip -> FAIL (ivb-t430s) skip -> FAIL (snb-x220t) skip -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup basic-default: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup basic-render: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup basic-vebox: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) skip -> FAIL (ivb-t430s) skip -> FAIL (snb-x220t) skip -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Test gem_exec_parse: Subgroup basic-allowed: skip -> FAIL (bsw-nuc-2) skip -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) skip -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) skip -> FAIL (snb-x220t) skip -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Subgroup basic-rejected: skip -> FAIL (bsw-nuc-2) skip -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) skip -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) skip -> FAIL (snb-x220t) skip -> FAIL (snb-dellxps) skip -> FAIL (ilk-hp8440p) Test gem_flink_basic: Subgroup bad-flink: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup bad-open: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup double-flink: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup flink-lifetime: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test gem_linear_blits: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test gem_mmap: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup basic-small-bo: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Test gem_mmap_gtt: Subgroup basic: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) pass -> FAIL (ilk-hp8440p) Subgroup basic-copy: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) pass -> FAIL (hsw-gt2) pass -> FAIL (bdw-ultra) pass -> FAIL (ivb-t430s) pass -> FAIL (snb-x220t) pass -> FAIL (snb-dellxps) Subgroup basic-read: pass -> FAIL (bsw-nuc-2) pass -> FAIL (skl-i5k-2) _______________________________________________ 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 v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-22 12:07 [PATCH] drm/i915: Avoid selecting unavailable BSD2 ring Gabriel Feceoru 2016-02-22 12:47 ` ✗ Fi.CI.BAT: failure for " Patchwork @ 2016-02-23 10:52 ` Gabriel Feceoru 2016-02-23 11:05 ` Tvrtko Ursulin 2016-02-23 11:48 ` ✗ Fi.CI.BAT: warning for drm/i915: Avoid selecting unavailable BSD2 ring (rev2) Patchwork 2 siblings, 1 reply; 11+ messages in thread From: Gabriel Feceoru @ 2016-02-23 10:52 UTC (permalink / raw) To: intel-gfx Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring is not available in the HW. v2: Reworked Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 8fd00d2..9fbd023 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1394,6 +1394,13 @@ eb_select_ring(struct drm_i915_private *dev_priv, return -EINVAL; } + if ((user_ring_id == I915_EXEC_BSD) && !HAS_BSD2(dev_priv) && + ((args->flags & I915_EXEC_BSD_MASK) != 0)) { + DRM_DEBUG("execbuf with bsd ring but with invalid " + "bsd dispatch flags: %d\n", (int)(args->flags)); + return -EINVAL; + } + if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; -- 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
* Re: [PATCH v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 10:52 ` [PATCH v2] " Gabriel Feceoru @ 2016-02-23 11:05 ` Tvrtko Ursulin 2016-02-23 13:06 ` Gabriel Feceoru 0 siblings, 1 reply; 11+ messages in thread From: Tvrtko Ursulin @ 2016-02-23 11:05 UTC (permalink / raw) To: Gabriel Feceoru, intel-gfx Hi, On 23/02/16 10:52, Gabriel Feceoru wrote: > Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring > is not available in the HW. What is the reasoning behind this? So far kernel was allowing userspace to select these bits and execute on the first engine. With this patch it would start failing potentially breaking userspace. Is it not too late to make such change? Regards, Tvrtko > v2: Reworked > Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 8fd00d2..9fbd023 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1394,6 +1394,13 @@ eb_select_ring(struct drm_i915_private *dev_priv, > return -EINVAL; > } > > + if ((user_ring_id == I915_EXEC_BSD) && !HAS_BSD2(dev_priv) && > + ((args->flags & I915_EXEC_BSD_MASK) != 0)) { > + DRM_DEBUG("execbuf with bsd ring but with invalid " > + "bsd dispatch flags: %d\n", (int)(args->flags)); > + return -EINVAL; > + } > + > if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { > unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; > > _______________________________________________ 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 v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 11:05 ` Tvrtko Ursulin @ 2016-02-23 13:06 ` Gabriel Feceoru 2016-02-23 13:31 ` Tvrtko Ursulin 0 siblings, 1 reply; 11+ messages in thread From: Gabriel Feceoru @ 2016-02-23 13:06 UTC (permalink / raw) To: Tvrtko Ursulin, intel-gfx On 23.02.2016 13:05, Tvrtko Ursulin wrote: > > Hi, > > On 23/02/16 10:52, Gabriel Feceoru wrote: >> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring >> is not available in the HW. > > What is the reasoning behind this? So far kernel was allowing userspace > to select these bits and execute on the first engine. With this patch it > would start failing potentially breaking userspace. Is it not too late > to make such change? I noticed some inconsistencies in igt with regards to bsd and bsd1. For instance, if bsd2 is not available, gem_sync@basic-bsd1 is skipped, but it's skipped because of the 2nd check gem_has_bsd2 (see gem_require_ring). Surprisingly gem_has_ring() didn't complain about bsd1. This fix will make gem_has_ring() return false. I'm not aware about legacy/compatibility issue - if that's the case, please disregard this. Regards, Gabriel > > Regards, > > Tvrtko > >> v2: Reworked >> Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index 8fd00d2..9fbd023 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1394,6 +1394,13 @@ eb_select_ring(struct drm_i915_private *dev_priv, >> return -EINVAL; >> } >> >> + if ((user_ring_id == I915_EXEC_BSD) && !HAS_BSD2(dev_priv) && >> + ((args->flags & I915_EXEC_BSD_MASK) != 0)) { >> + DRM_DEBUG("execbuf with bsd ring but with invalid " >> + "bsd dispatch flags: %d\n", (int)(args->flags)); >> + return -EINVAL; >> + } >> + >> if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { >> unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; >> >> _______________________________________________ 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 v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 13:06 ` Gabriel Feceoru @ 2016-02-23 13:31 ` Tvrtko Ursulin 2016-02-23 14:03 ` Chris Wilson 0 siblings, 1 reply; 11+ messages in thread From: Tvrtko Ursulin @ 2016-02-23 13:31 UTC (permalink / raw) To: Gabriel Feceoru, intel-gfx On 23/02/16 13:06, Gabriel Feceoru wrote: > > > On 23.02.2016 13:05, Tvrtko Ursulin wrote: >> >> Hi, >> >> On 23/02/16 10:52, Gabriel Feceoru wrote: >>> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring >>> is not available in the HW. >> >> What is the reasoning behind this? So far kernel was allowing userspace >> to select these bits and execute on the first engine. With this patch it >> would start failing potentially breaking userspace. Is it not too late >> to make such change? > > I noticed some inconsistencies in igt with regards to bsd and bsd1. > For instance, if bsd2 is not available, gem_sync@basic-bsd1 is skipped, > but it's skipped because of the 2nd check gem_has_bsd2 (see > gem_require_ring). Surprisingly gem_has_ring() didn't complain about bsd1. > > This fix will make gem_has_ring() return false. > > I'm not aware about legacy/compatibility issue - if that's the case, > please disregard this. Hmmm.. Chris, what is the reasoning behind: commit eaa03678b00179da89f194113c0740c033857c1c Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Thu Jan 28 13:44:19 2016 +0000 lib: Hide BSD1/BSD2 rings on hardware without BSD2 The kernel happily lets us run on I915_EXEC_BSD2 even with such hardware existing. Sigh. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 9dfa9b2603ce..fa44080e5902 100644 --- a/lib/ioctl_wrappers.c +++ b/lib/ioctl_wrappers.c @@ -1341,6 +1341,12 @@ static int gem_has_ring(int fd, int ring) void gem_require_ring(int fd, int ring_id) { igt_require(gem_has_ring(fd, ring_id)); + + /* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */ + if ((ring_id & ~(3<<13)) == I915_EXEC_BSD) { + if (ring_id & (3 << 13)) + igt_require(gem_has_bsd2(fd)); + } } /* prime */ ABI was (and still is) that specifying BSD1 or BSD2 explicitly is silently ignored by the kernel when BSD2 is not preset, defaulting to BSD1. This patch makes tests requesting BSD1 skip when there is no BSD2 which I think is wrong in any case. If we want to (and can) change the ABI it should only reject the non-existent ring and not limit the selection mechanism to hardware which has BSD2. Regards, Tvrtko _______________________________________________ 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
* Re: [PATCH v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 13:31 ` Tvrtko Ursulin @ 2016-02-23 14:03 ` Chris Wilson 2016-02-23 14:39 ` Tvrtko Ursulin 0 siblings, 1 reply; 11+ messages in thread From: Chris Wilson @ 2016-02-23 14:03 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx On Tue, Feb 23, 2016 at 01:31:17PM +0000, Tvrtko Ursulin wrote: > > On 23/02/16 13:06, Gabriel Feceoru wrote: > > > > > > On 23.02.2016 13:05, Tvrtko Ursulin wrote: > >> > >> Hi, > >> > >> On 23/02/16 10:52, Gabriel Feceoru wrote: > >>> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring > >>> is not available in the HW. > >> > >> What is the reasoning behind this? So far kernel was allowing userspace > >> to select these bits and execute on the first engine. With this patch it > >> would start failing potentially breaking userspace. Is it not too late > >> to make such change? > > > > I noticed some inconsistencies in igt with regards to bsd and bsd1. > > For instance, if bsd2 is not available, gem_sync@basic-bsd1 is skipped, > > but it's skipped because of the 2nd check gem_has_bsd2 (see > > gem_require_ring). Surprisingly gem_has_ring() didn't complain about bsd1. > > > > This fix will make gem_has_ring() return false. > > > > I'm not aware about legacy/compatibility issue - if that's the case, > > please disregard this. > > Hmmm.. Chris, what is the reasoning behind: > > commit eaa03678b00179da89f194113c0740c033857c1c > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Thu Jan 28 13:44:19 2016 +0000 > > lib: Hide BSD1/BSD2 rings on hardware without BSD2 > > The kernel happily lets us run on I915_EXEC_BSD2 even with such hardware > existing. Sigh. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c > index 9dfa9b2603ce..fa44080e5902 100644 > --- a/lib/ioctl_wrappers.c > +++ b/lib/ioctl_wrappers.c > @@ -1341,6 +1341,12 @@ static int gem_has_ring(int fd, int ring) > void gem_require_ring(int fd, int ring_id) > { > igt_require(gem_has_ring(fd, ring_id)); > + > + /* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */ > + if ((ring_id & ~(3<<13)) == I915_EXEC_BSD) { > + if (ring_id & (3 << 13)) > + igt_require(gem_has_bsd2(fd)); > + } > } > > /* prime */ > > ABI was (and still is) that specifying BSD1 or BSD2 explicitly is > silently ignored by the kernel when BSD2 is not preset, defaulting > to BSD1. Thereby pretending that BSD, BSD1, BSD2 exist. > This patch makes tests requesting BSD1 skip when there is no BSD2 > which I think is wrong in any case. BSD 1/2 selection only makes sense when we have multiple BSD rings. Running tests on BSD default, BSD1 and BSD2 is pointless if they all are equivalent. Using the BSD ping-pong when we have BSD1 and BSD2 is questionable as the ping-pong nature is uncontrolled, but nevertheless the code path needs to be tested. > If we want to (and can) change the ABI it should only reject the > non-existent ring and not limit the selection mechanism to > hardware which has BSD2. I disagree, we have a ring selection mechanism. If the extension doesn't exist, trying to use it should be an error. The extension was not only an ABI mistake but undesired (it is now defunct). -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 v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 14:03 ` Chris Wilson @ 2016-02-23 14:39 ` Tvrtko Ursulin 2016-02-23 19:36 ` Dave Gordon 0 siblings, 1 reply; 11+ messages in thread From: Tvrtko Ursulin @ 2016-02-23 14:39 UTC (permalink / raw) To: Chris Wilson, Gabriel Feceoru, intel-gfx On 23/02/16 14:03, Chris Wilson wrote: > On Tue, Feb 23, 2016 at 01:31:17PM +0000, Tvrtko Ursulin wrote: >> >> On 23/02/16 13:06, Gabriel Feceoru wrote: >>> >>> >>> On 23.02.2016 13:05, Tvrtko Ursulin wrote: >>>> >>>> Hi, >>>> >>>> On 23/02/16 10:52, Gabriel Feceoru wrote: >>>>> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring >>>>> is not available in the HW. >>>> >>>> What is the reasoning behind this? So far kernel was allowing userspace >>>> to select these bits and execute on the first engine. With this patch it >>>> would start failing potentially breaking userspace. Is it not too late >>>> to make such change? >>> >>> I noticed some inconsistencies in igt with regards to bsd and bsd1. >>> For instance, if bsd2 is not available, gem_sync@basic-bsd1 is skipped, >>> but it's skipped because of the 2nd check gem_has_bsd2 (see >>> gem_require_ring). Surprisingly gem_has_ring() didn't complain about bsd1. >>> >>> This fix will make gem_has_ring() return false. >>> >>> I'm not aware about legacy/compatibility issue - if that's the case, >>> please disregard this. >> >> Hmmm.. Chris, what is the reasoning behind: >> >> commit eaa03678b00179da89f194113c0740c033857c1c >> Author: Chris Wilson <chris@chris-wilson.co.uk> >> Date: Thu Jan 28 13:44:19 2016 +0000 >> >> lib: Hide BSD1/BSD2 rings on hardware without BSD2 >> >> The kernel happily lets us run on I915_EXEC_BSD2 even with such hardware >> existing. Sigh. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> >> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c >> index 9dfa9b2603ce..fa44080e5902 100644 >> --- a/lib/ioctl_wrappers.c >> +++ b/lib/ioctl_wrappers.c >> @@ -1341,6 +1341,12 @@ static int gem_has_ring(int fd, int ring) >> void gem_require_ring(int fd, int ring_id) >> { >> igt_require(gem_has_ring(fd, ring_id)); >> + >> + /* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */ >> + if ((ring_id & ~(3<<13)) == I915_EXEC_BSD) { >> + if (ring_id & (3 << 13)) >> + igt_require(gem_has_bsd2(fd)); >> + } >> } >> >> /* prime */ >> >> ABI was (and still is) that specifying BSD1 or BSD2 explicitly is >> silently ignored by the kernel when BSD2 is not preset, defaulting >> to BSD1. > > Thereby pretending that BSD, BSD1, BSD2 exist. > >> This patch makes tests requesting BSD1 skip when there is no BSD2 >> which I think is wrong in any case. > > BSD 1/2 selection only makes sense when we have multiple BSD rings. > Running tests on BSD default, BSD1 and BSD2 is pointless if they all > are equivalent. Using the BSD ping-pong when we have BSD1 and BSD2 is > questionable as the ping-pong nature is uncontrolled, but nevertheless > the code path needs to be tested. > >> If we want to (and can) change the ABI it should only reject the >> non-existent ring and not limit the selection mechanism to >> hardware which has BSD2. > > I disagree, we have a ring selection mechanism. If the extension doesn't > exist, trying to use it should be an error. The extension was not only > an ABI mistake but undesired (it is now defunct). Not sure that I understand what you meant here. Nothing as far as I can tell is now defunct. Neither the selection mechanism, or the existence of BSD2. To be absolutely clear, you are, or you are not, in favour of Gabriel's patch to start failing execbuf with fine grained BSD selection flags when BSD2 is not present? Regards, Tvrtko _______________________________________________ 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 v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 14:39 ` Tvrtko Ursulin @ 2016-02-23 19:36 ` Dave Gordon 2016-02-24 15:27 ` Gabriel Feceoru 0 siblings, 1 reply; 11+ messages in thread From: Dave Gordon @ 2016-02-23 19:36 UTC (permalink / raw) To: Tvrtko Ursulin, Chris Wilson, Gabriel Feceoru; +Cc: intel-gfx On 23/02/16 14:39, Tvrtko Ursulin wrote: > > On 23/02/16 14:03, Chris Wilson wrote: >> On Tue, Feb 23, 2016 at 01:31:17PM +0000, Tvrtko Ursulin wrote: >>> >>> On 23/02/16 13:06, Gabriel Feceoru wrote: >>>> >>>> >>>> On 23.02.2016 13:05, Tvrtko Ursulin wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 23/02/16 10:52, Gabriel Feceoru wrote: >>>>>> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring >>>>>> is not available in the HW. >>>>> >>>>> What is the reasoning behind this? So far kernel was allowing >>>>> userspace >>>>> to select these bits and execute on the first engine. With this >>>>> patch it >>>>> would start failing potentially breaking userspace. Is it not too late >>>>> to make such change? >>>> >>>> I noticed some inconsistencies in igt with regards to bsd and bsd1. >>>> For instance, if bsd2 is not available, gem_sync@basic-bsd1 is skipped, >>>> but it's skipped because of the 2nd check gem_has_bsd2 (see >>>> gem_require_ring). Surprisingly gem_has_ring() didn't complain about >>>> bsd1. >>>> >>>> This fix will make gem_has_ring() return false. >>>> >>>> I'm not aware about legacy/compatibility issue - if that's the case, >>>> please disregard this. >>> >>> Hmmm.. Chris, what is the reasoning behind: >>> >>> commit eaa03678b00179da89f194113c0740c033857c1c >>> Author: Chris Wilson <chris@chris-wilson.co.uk> >>> Date: Thu Jan 28 13:44:19 2016 +0000 >>> >>> lib: Hide BSD1/BSD2 rings on hardware without BSD2 >>> >>> The kernel happily lets us run on I915_EXEC_BSD2 even with such >>> hardware >>> existing. Sigh. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> >>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c >>> index 9dfa9b2603ce..fa44080e5902 100644 >>> --- a/lib/ioctl_wrappers.c >>> +++ b/lib/ioctl_wrappers.c >>> @@ -1341,6 +1341,12 @@ static int gem_has_ring(int fd, int ring) >>> void gem_require_ring(int fd, int ring_id) >>> { >>> igt_require(gem_has_ring(fd, ring_id)); >>> + >>> + /* silly ABI, the kernel thinks everyone who has BSD also has >>> BSD2 */ >>> + if ((ring_id & ~(3<<13)) == I915_EXEC_BSD) { >>> + if (ring_id & (3 << 13)) >>> + igt_require(gem_has_bsd2(fd)); >>> + } >>> } >>> >>> /* prime */ >>> >>> ABI was (and still is) that specifying BSD1 or BSD2 explicitly is >>> silently ignored by the kernel when BSD2 is not preset, defaulting >>> to BSD1. >> >> Thereby pretending that BSD, BSD1, BSD2 exist. >> >>> This patch makes tests requesting BSD1 skip when there is no BSD2 >>> which I think is wrong in any case. >> >> BSD 1/2 selection only makes sense when we have multiple BSD rings. >> Running tests on BSD default, BSD1 and BSD2 is pointless if they all >> are equivalent. Using the BSD ping-pong when we have BSD1 and BSD2 is >> questionable as the ping-pong nature is uncontrolled, but nevertheless >> the code path needs to be tested. >> >>> If we want to (and can) change the ABI it should only reject the >>> non-existent ring and not limit the selection mechanism to >>> hardware which has BSD2. >> >> I disagree, we have a ring selection mechanism. If the extension doesn't >> exist, trying to use it should be an error. The extension was not only >> an ABI mistake but undesired (it is now defunct). > > Not sure that I understand what you meant here. Nothing as far as I can > tell is now defunct. Neither the selection mechanism, or the existence > of BSD2. > > To be absolutely clear, you are, or you are not, in favour of Gabriel's > patch to start failing execbuf with fine grained BSD selection flags > when BSD2 is not present? > > Regards, > Tvrtko Currently: #define I915_EXEC_BSD (2<<0) /** Used for switching BSD rings on the platforms with two BSD rings */ #define I915_EXEC_BSD_SHIFT (13) #define I915_EXEC_BSD_MASK (3 << I915_EXEC_BSD_SHIFT) /* default ping-pong mode */ #define I915_EXEC_BSD_DEFAULT (0 << I915_EXEC_BSD_SHIFT) #define I915_EXEC_BSD_RING1 (1 << I915_EXEC_BSD_SHIFT) #define I915_EXEC_BSD_RING2 (2 << I915_EXEC_BSD_SHIFT) It makes sense to have the original "BSD" flag mean "the default BSD", and use different flags to mean specifically "BSD1" or "BSD2". Which appears to be what we've done; naive clients that don't set any of the new BSD bits will get default behaviour (execute on *any* BSD ring) with no control over the ping-pong mechanism (if any). Clients that specify a specific ring should get that one, and only that one; if it doesn't exist then it's an error. Any program that's going to set these bits should first ask whether (or which) engines exist and select appropriately. So I think I'm with Chris here. On the other hand, I think what Tvrtko said was "it should not be an error to select a specific ring [that exists] just because there are no other rings". Which I also agree with. So the ring-select-checking code should: 1. reject the I915_EXEC_BSD_RING2 case if BSD2 does not exist 2. reject the I915_EXEC_BSD_RING1 case if BSD1 does not exist (for some future bizarre numbering scheme? or a partitioning system that reserves BSD1 for someone else?) 3. never reject the I915_EXEC_BSD_DEFAULT case (although it will have rejected the I915_EXEC_BSD flag before looking at these if there is no BSD ring at all) 4. for now (until we assign it a meaning) reject the case where BOTH BSD ring-select bits are set. Therefore I disagree with Gabriel's patch which would reject trying to select BSD1 on a platform that only has the one BSD engine. .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 v2] drm/i915: Avoid selecting unavailable BSD2 ring 2016-02-23 19:36 ` Dave Gordon @ 2016-02-24 15:27 ` Gabriel Feceoru 0 siblings, 0 replies; 11+ messages in thread From: Gabriel Feceoru @ 2016-02-24 15:27 UTC (permalink / raw) To: Dave Gordon, Tvrtko Ursulin, Chris Wilson; +Cc: intel-gfx On 23.02.2016 21:36, Dave Gordon wrote: > On 23/02/16 14:39, Tvrtko Ursulin wrote: >> >> On 23/02/16 14:03, Chris Wilson wrote: >>> On Tue, Feb 23, 2016 at 01:31:17PM +0000, Tvrtko Ursulin wrote: >>>> >>>> On 23/02/16 13:06, Gabriel Feceoru wrote: >>>>> >>>>> >>>>> On 23.02.2016 13:05, Tvrtko Ursulin wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 23/02/16 10:52, Gabriel Feceoru wrote: >>>>>>> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring >>>>>>> is not available in the HW. >>>>>> >>>>>> What is the reasoning behind this? So far kernel was allowing >>>>>> userspace >>>>>> to select these bits and execute on the first engine. With this >>>>>> patch it >>>>>> would start failing potentially breaking userspace. Is it not too >>>>>> late >>>>>> to make such change? >>>>> >>>>> I noticed some inconsistencies in igt with regards to bsd and bsd1. >>>>> For instance, if bsd2 is not available, gem_sync@basic-bsd1 is >>>>> skipped, >>>>> but it's skipped because of the 2nd check gem_has_bsd2 (see >>>>> gem_require_ring). Surprisingly gem_has_ring() didn't complain about >>>>> bsd1. >>>>> >>>>> This fix will make gem_has_ring() return false. >>>>> >>>>> I'm not aware about legacy/compatibility issue - if that's the case, >>>>> please disregard this. >>>> >>>> Hmmm.. Chris, what is the reasoning behind: >>>> >>>> commit eaa03678b00179da89f194113c0740c033857c1c >>>> Author: Chris Wilson <chris@chris-wilson.co.uk> >>>> Date: Thu Jan 28 13:44:19 2016 +0000 >>>> >>>> lib: Hide BSD1/BSD2 rings on hardware without BSD2 >>>> >>>> The kernel happily lets us run on I915_EXEC_BSD2 even with such >>>> hardware >>>> existing. Sigh. >>>> >>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>> >>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c >>>> index 9dfa9b2603ce..fa44080e5902 100644 >>>> --- a/lib/ioctl_wrappers.c >>>> +++ b/lib/ioctl_wrappers.c >>>> @@ -1341,6 +1341,12 @@ static int gem_has_ring(int fd, int ring) >>>> void gem_require_ring(int fd, int ring_id) >>>> { >>>> igt_require(gem_has_ring(fd, ring_id)); >>>> + >>>> + /* silly ABI, the kernel thinks everyone who has BSD also has >>>> BSD2 */ >>>> + if ((ring_id & ~(3<<13)) == I915_EXEC_BSD) { >>>> + if (ring_id & (3 << 13)) >>>> + igt_require(gem_has_bsd2(fd)); >>>> + } >>>> } >>>> >>>> /* prime */ >>>> >>>> ABI was (and still is) that specifying BSD1 or BSD2 explicitly is >>>> silently ignored by the kernel when BSD2 is not preset, defaulting >>>> to BSD1. >>> >>> Thereby pretending that BSD, BSD1, BSD2 exist. >>> >>>> This patch makes tests requesting BSD1 skip when there is no BSD2 >>>> which I think is wrong in any case. >>> >>> BSD 1/2 selection only makes sense when we have multiple BSD rings. >>> Running tests on BSD default, BSD1 and BSD2 is pointless if they all >>> are equivalent. Using the BSD ping-pong when we have BSD1 and BSD2 is >>> questionable as the ping-pong nature is uncontrolled, but nevertheless >>> the code path needs to be tested. >>> >>>> If we want to (and can) change the ABI it should only reject the >>>> non-existent ring and not limit the selection mechanism to >>>> hardware which has BSD2. >>> >>> I disagree, we have a ring selection mechanism. If the extension doesn't >>> exist, trying to use it should be an error. The extension was not only >>> an ABI mistake but undesired (it is now defunct). >> >> Not sure that I understand what you meant here. Nothing as far as I can >> tell is now defunct. Neither the selection mechanism, or the existence >> of BSD2. >> >> To be absolutely clear, you are, or you are not, in favour of Gabriel's >> patch to start failing execbuf with fine grained BSD selection flags >> when BSD2 is not present? >> >> Regards, >> Tvrtko > > Currently: > > #define I915_EXEC_BSD (2<<0) > > /** Used for switching BSD rings on the platforms with two BSD rings */ > #define I915_EXEC_BSD_SHIFT (13) > #define I915_EXEC_BSD_MASK (3 << I915_EXEC_BSD_SHIFT) /* default > ping-pong mode */ > #define I915_EXEC_BSD_DEFAULT (0 << I915_EXEC_BSD_SHIFT) > #define I915_EXEC_BSD_RING1 (1 << I915_EXEC_BSD_SHIFT) > #define I915_EXEC_BSD_RING2 (2 << I915_EXEC_BSD_SHIFT) > > It makes sense to have the original "BSD" flag mean "the default BSD", > and use different flags to mean specifically "BSD1" or "BSD2". Which > appears to be what we've done; naive clients that don't set any of the > new BSD bits will get default behaviour (execute on *any* BSD ring) with > no control over the ping-pong mechanism (if any). Clients that specify a > specific ring should get that one, and only that one; if it doesn't > exist then it's an error. > > Any program that's going to set these bits should first ask whether (or > which) engines exist and select appropriately. So I think I'm with Chris > here. > > On the other hand, I think what Tvrtko said was "it should not be an > error to select a specific ring [that exists] just because there are no > other rings". Which I also agree with. > > So the ring-select-checking code should: > 1. reject the I915_EXEC_BSD_RING2 case if BSD2 does not exist This is one of the things the patch does, I guess everybody agrees on rejecting BSD2. > 2. reject the I915_EXEC_BSD_RING1 case if BSD1 does not exist > (for some future bizarre numbering scheme? or a > partitioning system that reserves BSD1 for someone else?) > 3. never reject the I915_EXEC_BSD_DEFAULT case > (although it will have rejected the I915_EXEC_BSD flag > before looking at these if there is no BSD ring at all) > 4. for now (until we assign it a meaning) reject the case where > BOTH BSD ring-select bits are set. > > Therefore I disagree with Gabriel's patch which would reject trying to > select BSD1 on a platform that only has the one BSD engine. Depending on the BSD1 debate resolution I see two possible solutions: 1. Change the patch to reject I915_EXEC_BSD_RING2 only and change gem_require_ring to allow BSD1. 2. Keep the patch, eventually remove that check in gem_require_ring (which will become redundant) in igt Thank you all for your input on this. Gabriel. > > .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
* ✗ Fi.CI.BAT: warning for drm/i915: Avoid selecting unavailable BSD2 ring (rev2) 2016-02-22 12:07 [PATCH] drm/i915: Avoid selecting unavailable BSD2 ring Gabriel Feceoru 2016-02-22 12:47 ` ✗ Fi.CI.BAT: failure for " Patchwork 2016-02-23 10:52 ` [PATCH v2] " Gabriel Feceoru @ 2016-02-23 11:48 ` Patchwork 2 siblings, 0 replies; 11+ messages in thread From: Patchwork @ 2016-02-23 11:48 UTC (permalink / raw) To: Feceoru, Gabriel; +Cc: intel-gfx == Series Details == Series: drm/i915: Avoid selecting unavailable BSD2 ring (rev2) URL : https://patchwork.freedesktop.org/series/3678/ State : warning == Summary == Series 3678v2 drm/i915: Avoid selecting unavailable BSD2 ring http://patchwork.freedesktop.org/api/1.0/series/3678/revisions/2/mbox/ Test gem_cs_prefetch: Subgroup basic-default: incomplete -> PASS (ilk-hp8440p) Test kms_force_connector_basic: Subgroup force-edid: skip -> PASS (snb-x220t) Subgroup force-load-detect: dmesg-fail -> FAIL (snb-x220t) dmesg-fail -> FAIL (snb-dellxps) Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: dmesg-warn -> PASS (skl-i5k-2) UNSTABLE Subgroup suspend-read-crc-pipe-c: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE Test pm_rpm: Subgroup basic-pci-d3-state: pass -> DMESG-WARN (bsw-nuc-2) bdw-nuci7 total:165 pass:154 dwarn:0 dfail:0 fail:0 skip:11 bdw-ultra total:168 pass:154 dwarn:0 dfail:0 fail:0 skip:14 bsw-nuc-2 total:168 pass:136 dwarn:1 dfail:0 fail:1 skip:30 byt-nuc total:168 pass:142 dwarn:1 dfail:0 fail:0 skip:25 hsw-gt2 total:168 pass:157 dwarn:0 dfail:1 fail:0 skip:10 ilk-hp8440p total:168 pass:118 dwarn:0 dfail:0 fail:1 skip:49 ivb-t430s total:168 pass:153 dwarn:0 dfail:0 fail:1 skip:14 skl-i5k-2 total:168 pass:151 dwarn:1 dfail:0 fail:0 skip:16 snb-dellxps total:168 pass:145 dwarn:0 dfail:0 fail:1 skip:22 snb-x220t total:168 pass:145 dwarn:0 dfail:0 fail:2 skip:21 Results at /archive/results/CI_IGT_test/Patchwork_1461/ 08fc1b101049694778bff7559e1d05250d2e7072 drm-intel-nightly: 2016y-02m-22d-17h-30m-27s UTC integration manifest 6afb1e4a01be18acf8e5c117977004750bacf4b8 drm/i915: Avoid selecting unavailable BSD2 ring _______________________________________________ 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
end of thread, other threads:[~2016-02-24 15:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-22 12:07 [PATCH] drm/i915: Avoid selecting unavailable BSD2 ring Gabriel Feceoru 2016-02-22 12:47 ` ✗ Fi.CI.BAT: failure for " Patchwork 2016-02-23 10:52 ` [PATCH v2] " Gabriel Feceoru 2016-02-23 11:05 ` Tvrtko Ursulin 2016-02-23 13:06 ` Gabriel Feceoru 2016-02-23 13:31 ` Tvrtko Ursulin 2016-02-23 14:03 ` Chris Wilson 2016-02-23 14:39 ` Tvrtko Ursulin 2016-02-23 19:36 ` Dave Gordon 2016-02-24 15:27 ` Gabriel Feceoru 2016-02-23 11:48 ` ✗ Fi.CI.BAT: warning for drm/i915: Avoid selecting unavailable BSD2 ring (rev2) Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).