[PATCH 1/2] drm/i915: Re-order some checks to do the unlikely one first

[PATCH 1/2] drm/i915: Re-order some checks to do the unlikely one first

[Damien Lespiau]

instpm_mode != relative_constants_mode is quite unlikely to happen, so
we can test it first to use C's && short-circuiting and not test on
'ring'.

I know, probably a useless micro-optimisation in the big scheme of
things, but I'm going to add another test here, so might as well do it.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index aafcef3..896641a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -691,8 +691,8 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		return ret;
 
-	if (ring == &dev_priv->ring[RCS] &&
-	    instp_mode != dev_priv->relative_constants_mode) {
+	if (instp_mode != dev_priv->relative_constants_mode &&
+	    ring == &dev_priv->ring[RCS]) {
 		ret = intel_logical_ring_begin(ringbuf, ctx, 4);
 		if (ret)
 			return ret;
-- 
1.8.3.1

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

[PATCH 2/2] drm/i915: Don't try to set INSTPM for the _ABSOLUTE constant buffer address

[Damien Lespiau]

Gen9 bit to control whether the 3DSTATE_CONSTANT_* address should be an
offset against the Dynamic State Base Address Vs an absolute address has
moved to a different register.

As no-one complained yet and I don't see any use of the
I915_EXEC_CONSTANTS_ABSOLUTE flag in either the DDX, mesa, libdrm or
libva, I'm taking the opportunity to deprecate the flag on gen9 (it
never worked in the first place).

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 13 ++++++++++++-
 include/uapi/drm/i915_drm.h      |  2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 896641a..836356a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -647,6 +647,16 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 	case I915_EXEC_CONSTANTS_REL_GENERAL:
 	case I915_EXEC_CONSTANTS_ABSOLUTE:
 	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (instp_mode != 0 && INTEL_INFO(dev)->gen >= 9) {
+			/*
+			 * While it's possible to implement _ABSOLUTE, noone
+			 * complained when it was broken, so let's simplify the
+			 * driver by not supporting it until further notice.
+			 */
+			DRM_DEBUG("no rel constants on gen9+\n");
+			return -EINVAL;
+		}
+
 		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
 			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
 			return -EINVAL;
@@ -691,7 +701,8 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		return ret;
 
-	if (instp_mode != dev_priv->relative_constants_mode &&
+	/* We deprecated the I915_EXEC_CONSTANTS on gen9+, for lack of user */
+	if (instp_mode != dev_priv->relative_constants_mode && IS_GEN8(dev) &&
 	    ring == &dev_priv->ring[RCS]) {
 		ret = intel_logical_ring_begin(ringbuf, ctx, 4);
 		if (ret)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 6eed16b..d7c17b7 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -715,7 +715,7 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_CONSTANTS_MASK 	(3<<6)
 #define I915_EXEC_CONSTANTS_REL_GENERAL (0<<6) /* default */
-#define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
+#define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6) /* gen 4/5/6/7/8 only */
 #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
 	__u64 flags;
 	__u64 rsvd1; /* now used for context info */
-- 
1.8.3.1

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

Re: [PATCH 1/2] drm/i915: Re-order some checks to do the unlikely one first

[Chris Wilson]

On Mon, Feb 16, 2015 at 06:25:10PM +0000, Damien Lespiau wrote:
> instpm_mode != relative_constants_mode is quite unlikely to happen, so
> we can test it first to use C's && short-circuiting and not test on
> 'ring'.
> 
> I know, probably a useless micro-optimisation in the big scheme of
> things, but I'm going to add another test here, so might as well do it.

If you want to get pedantic, we want to move this to per-context :)
-Chris

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

Re: [PATCH 2/2] drm/i915: Don't try to set INSTPM for the _ABSOLUTE constant buffer address

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5786
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -5              277/277              272/277
ILK                                  313/313              313/313
SNB                 -1              309/309              308/309
IVB                                  382/382              382/382
BYT                                  296/296              296/296
HSW                                  425/425              425/425
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-none      NRUN(1)PASS(2)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(3)      FAIL(1)NO_RESULT(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-y      PASS(3)      FAIL(1)PASS(1)
*PNV  igt_gem_userptr_blits_coherency-sync      CRASH(1)PASS(3)      NO_RESULT(1)CRASH(1)
 PNV  igt_gem_userptr_blits_coherency-unsync      CRASH(1)PASS(2)      CRASH(2)
*SNB  igt_kms_plane_plane-panning-top-left-pipe-B-plane-1      PASS(2)      TIMEOUT(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(5)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Re-order some checks to do the unlikely one first

[Damien Lespiau]

On Mon, Feb 16, 2015 at 09:03:51PM +0000, Chris Wilson wrote:
> On Mon, Feb 16, 2015 at 06:25:10PM +0000, Damien Lespiau wrote:
> > instpm_mode != relative_constants_mode is quite unlikely to happen, so
> > we can test it first to use C's && short-circuiting and not test on
> > 'ring'.
> > 
> > I know, probably a useless micro-optimisation in the big scheme of
> > things, but I'm going to add another test here, so might as well do it.
> 
> If you want to get pedantic, we want to move this to per-context :)

Do we? the API is per execbuf call, so theoretically application can
change that during the context life time (it'd be silly, but they can).
Or am I missing the point?

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

Re: [PATCH 1/2] drm/i915: Re-order some checks to do the unlikely one first

[Chris Wilson]

On Tue, Feb 17, 2015 at 11:48:25AM +0000, Damien Lespiau wrote:
> On Mon, Feb 16, 2015 at 09:03:51PM +0000, Chris Wilson wrote:
> > On Mon, Feb 16, 2015 at 06:25:10PM +0000, Damien Lespiau wrote:
> > > instpm_mode != relative_constants_mode is quite unlikely to happen, so
> > > we can test it first to use C's && short-circuiting and not test on
> > > 'ring'.
> > > 
> > > I know, probably a useless micro-optimisation in the big scheme of
> > > things, but I'm going to add another test here, so might as well do it.
> > 
> > If you want to get pedantic, we want to move this to per-context :)
> 
> Do we? the API is per execbuf call, so theoretically application can
> change that during the context life time (it'd be silly, but they can).
> Or am I missing the point?

It was that this is a context specific register and more about handling
the transaction unwind if the execbuffer were to fail later i.e. an
issue about to arise with new features modifying the code. But in theory
we could do fewer LRI for that one application that used it...
-Chris

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

Re: [PATCH 1/2] drm/i915: Re-order some checks to do the unlikely one first

[Dave Gordon]

On 16/02/15 21:03, Chris Wilson wrote:
> On Mon, Feb 16, 2015 at 06:25:10PM +0000, Damien Lespiau wrote:
>> instpm_mode != relative_constants_mode is quite unlikely to happen, so
>> we can test it first to use C's && short-circuiting and not test on
>> 'ring'.
>>
>> I know, probably a useless micro-optimisation in the big scheme of
>> things, but I'm going to add another test here, so might as well do it.
> 
> If you want to get pedantic, we want to move this to per-context :)
> -Chris

This test-and-set of instpm_mode will get reworked anyway when we add
preemption support, as that means we can no longer assume the value set
in the last execbuffer call is still in force at the beginning of the next.

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

Re: [PATCH 2/2] drm/i915: Don't try to set INSTPM for the _ABSOLUTE constant buffer address

[Daniel Vetter]

On Mon, Feb 16, 2015 at 06:25:11PM +0000, Damien Lespiau wrote:
> Gen9 bit to control whether the 3DSTATE_CONSTANT_* address should be an
> offset against the Dynamic State Base Address Vs an absolute address has
> moved to a different register.
> 
> As no-one complained yet and I don't see any use of the
> I915_EXEC_CONSTANTS_ABSOLUTE flag in either the DDX, mesa, libdrm or
> libva, I'm taking the opportunity to deprecate the flag on gen9 (it
> never worked in the first place).
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Patch to igt/gem_exec_params seems to be missing ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 13 ++++++++++++-
>  include/uapi/drm/i915_drm.h      |  2 +-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 896641a..836356a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -647,6 +647,16 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
>  	case I915_EXEC_CONSTANTS_REL_GENERAL:
>  	case I915_EXEC_CONSTANTS_ABSOLUTE:
>  	case I915_EXEC_CONSTANTS_REL_SURFACE:
> +		if (instp_mode != 0 && INTEL_INFO(dev)->gen >= 9) {
> +			/*
> +			 * While it's possible to implement _ABSOLUTE, noone
> +			 * complained when it was broken, so let's simplify the
> +			 * driver by not supporting it until further notice.
> +			 */
> +			DRM_DEBUG("no rel constants on gen9+\n");
> +			return -EINVAL;
> +		}
> +
>  		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
>  			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
>  			return -EINVAL;
> @@ -691,7 +701,8 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
>  	if (ret)
>  		return ret;
>  
> -	if (instp_mode != dev_priv->relative_constants_mode &&
> +	/* We deprecated the I915_EXEC_CONSTANTS on gen9+, for lack of user */
> +	if (instp_mode != dev_priv->relative_constants_mode && IS_GEN8(dev) &&
>  	    ring == &dev_priv->ring[RCS]) {
>  		ret = intel_logical_ring_begin(ringbuf, ctx, 4);
>  		if (ret)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 6eed16b..d7c17b7 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -715,7 +715,7 @@ struct drm_i915_gem_execbuffer2 {
>   */
>  #define I915_EXEC_CONSTANTS_MASK 	(3<<6)
>  #define I915_EXEC_CONSTANTS_REL_GENERAL (0<<6) /* default */
> -#define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
> +#define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6) /* gen 4/5/6/7/8 only */
>  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
>  	__u64 flags;
>  	__u64 rsvd1; /* now used for context info */
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx