diff mbox

[01/43] drm/i915: Reorder the actual workload submission so that args checking is done earlier

Message ID 1406217891-8912-2-git-send-email-thomas.daniel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Daniel July 24, 2014, 4:04 p.m. UTC 
From: Oscar Mateo <oscar.mateo@intel.com>

In this patch:

commit 78382593e921c88371abd019aca8978db3248a8f
Author: Oscar Mateo <oscar.mateo@intel.com>
Date:   Thu Jul 3 16:28:05 2014 +0100

    drm/i915: Extract the actual workload submission mechanism from execbuffer

    So that we isolate the legacy ringbuffer submission mechanism, which becomes
    a good candidate to be abstracted away. This is prep-work for Execlists (which
    will its own workload submission mechanism).

    No functional changes.

I changed the order in which the args checking is done. I don't know why I did (brain
fade?) but it? not right. I haven't seen any ill effect from this, but the Execlists
version of this function will have problems if the order is not correct.

Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   86 ++++++++++++++--------------
 1 file changed, 43 insertions(+), 43 deletions(-)

Comments

Daniel Vetter July 25, 2014, 8:30 a.m. UTC | #1 
On Thu, Jul 24, 2014 at 05:04:09PM +0100, Thomas Daniel wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> In this patch:
> 
> commit 78382593e921c88371abd019aca8978db3248a8f
> Author: Oscar Mateo <oscar.mateo@intel.com>
> Date:   Thu Jul 3 16:28:05 2014 +0100
> 
>     drm/i915: Extract the actual workload submission mechanism from execbuffer
> 
>     So that we isolate the legacy ringbuffer submission mechanism, which becomes
>     a good candidate to be abstracted away. This is prep-work for Execlists (which
>     will its own workload submission mechanism).
> 
>     No functional changes.
> 
> I changed the order in which the args checking is done. I don't know why I did (brain
> fade?) but it? not right. I haven't seen any ill effect from this, but the Execlists
> version of this function will have problems if the order is not correct.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

I don't think this matters - the point of no return for legacy execbuf is
the call to ring->dispatch. After that nothing may fail any more. But as
long as we track state correctly (e.g. if we've switched the context
already) we'll be fine.

So presuming I'm not blind I dont' think this is needed. But maybe Chris
spots something.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   86 ++++++++++++++--------------
>  1 file changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 60998fc..c5115957 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1042,6 +1042,43 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  	u32 instp_mask;
>  	int i, ret = 0;
>  
> +	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> +	instp_mask = I915_EXEC_CONSTANTS_MASK;
> +	switch (instp_mode) {
> +	case I915_EXEC_CONSTANTS_REL_GENERAL:
> +	case I915_EXEC_CONSTANTS_ABSOLUTE:
> +	case I915_EXEC_CONSTANTS_REL_SURFACE:
> +		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
> +			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> +			ret = -EINVAL;
> +			goto error;
> +		}
> +
> +		if (instp_mode != dev_priv->relative_constants_mode) {
> +			if (INTEL_INFO(dev)->gen < 4) {
> +				DRM_DEBUG("no rel constants on pre-gen4\n");
> +				ret = -EINVAL;
> +				goto error;
> +			}
> +
> +			if (INTEL_INFO(dev)->gen > 5 &&
> +			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
> +				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
> +				ret = -EINVAL;
> +				goto error;
> +			}
> +
> +			/* The HW changed the meaning on this bit on gen6 */
> +			if (INTEL_INFO(dev)->gen >= 6)
> +				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
> +		}
> +		break;
> +	default:
> +		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
>  	if (args->num_cliprects != 0) {
>  		if (ring != &dev_priv->ring[RCS]) {
>  			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
> @@ -1085,6 +1122,12 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  		}
>  	}
>  
> +	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
> +		ret = i915_reset_gen7_sol_offsets(dev, ring);
> +		if (ret)
> +			goto error;
> +	}
> +
>  	ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
>  	if (ret)
>  		goto error;
> @@ -1093,43 +1136,6 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  	if (ret)
>  		goto error;
>  
> -	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> -	instp_mask = I915_EXEC_CONSTANTS_MASK;
> -	switch (instp_mode) {
> -	case I915_EXEC_CONSTANTS_REL_GENERAL:
> -	case I915_EXEC_CONSTANTS_ABSOLUTE:
> -	case I915_EXEC_CONSTANTS_REL_SURFACE:
> -		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
> -			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
> -			ret = -EINVAL;
> -			goto error;
> -		}
> -
> -		if (instp_mode != dev_priv->relative_constants_mode) {
> -			if (INTEL_INFO(dev)->gen < 4) {
> -				DRM_DEBUG("no rel constants on pre-gen4\n");
> -				ret = -EINVAL;
> -				goto error;
> -			}
> -
> -			if (INTEL_INFO(dev)->gen > 5 &&
> -			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
> -				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
> -				ret = -EINVAL;
> -				goto error;
> -			}
> -
> -			/* The HW changed the meaning on this bit on gen6 */
> -			if (INTEL_INFO(dev)->gen >= 6)
> -				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
> -		}
> -		break;
> -	default:
> -		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> -		ret = -EINVAL;
> -		goto error;
> -	}
> -
>  	if (ring == &dev_priv->ring[RCS] &&
>  			instp_mode != dev_priv->relative_constants_mode) {
>  		ret = intel_ring_begin(ring, 4);
> @@ -1145,12 +1151,6 @@ legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  		dev_priv->relative_constants_mode = instp_mode;
>  	}
>  
> -	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
> -		ret = i915_reset_gen7_sol_offsets(dev, ring);
> -		if (ret)
> -			goto error;
> -	}
> -
>  	exec_len = args->batch_len;
>  	if (cliprects) {
>  		for (i = 0; i < args->num_cliprects; i++) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson July 25, 2014, 9:16 a.m. UTC | #2 
On Fri, Jul 25, 2014 at 10:30:09AM +0200, Daniel Vetter wrote:
> On Thu, Jul 24, 2014 at 05:04:09PM +0100, Thomas Daniel wrote:
> > From: Oscar Mateo <oscar.mateo@intel.com>
> > 
> > In this patch:
> > 
> > commit 78382593e921c88371abd019aca8978db3248a8f
> > Author: Oscar Mateo <oscar.mateo@intel.com>
> > Date:   Thu Jul 3 16:28:05 2014 +0100
> > 
> >     drm/i915: Extract the actual workload submission mechanism from execbuffer
> > 
> >     So that we isolate the legacy ringbuffer submission mechanism, which becomes
> >     a good candidate to be abstracted away. This is prep-work for Execlists (which
> >     will its own workload submission mechanism).
> > 
> >     No functional changes.
> > 
> > I changed the order in which the args checking is done. I don't know why I did (brain
> > fade?) but it? not right. I haven't seen any ill effect from this, but the Execlists
> > version of this function will have problems if the order is not correct.
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> 
> I don't think this matters - the point of no return for legacy execbuf is
> the call to ring->dispatch. After that nothing may fail any more. But as
> long as we track state correctly (e.g. if we've switched the context
> already) we'll be fine.

Right. Except that I think our tracking is buggy - or at least
insufficient to address the needs of future dispatch mechanisms. I think
that we confuse some bookkeeping that should be at the request level and
place it on the ring. At the moment, we have one request per-ring and so
it doesn't matter, but transitioning to one request per-logical-ring we
start to have issues as that state is being tracked on the wrong struct.

Anyway, that's part of the motivation to fixing up requests and making
them central to accessing the rings/dispatch (whereas at the moment they
are behind the scenes).
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc..c5115957 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1042,6 +1042,43 @@  legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	u32 instp_mask;
 	int i, ret = 0;
 
+	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
+	instp_mask = I915_EXEC_CONSTANTS_MASK;
+	switch (instp_mode) {
+	case I915_EXEC_CONSTANTS_REL_GENERAL:
+	case I915_EXEC_CONSTANTS_ABSOLUTE:
+	case I915_EXEC_CONSTANTS_REL_SURFACE:
+		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
+			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
+			ret = -EINVAL;
+			goto error;
+		}
+
+		if (instp_mode != dev_priv->relative_constants_mode) {
+			if (INTEL_INFO(dev)->gen < 4) {
+				DRM_DEBUG("no rel constants on pre-gen4\n");
+				ret = -EINVAL;
+				goto error;
+			}
+
+			if (INTEL_INFO(dev)->gen > 5 &&
+			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
+				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
+				ret = -EINVAL;
+				goto error;
+			}
+
+			/* The HW changed the meaning on this bit on gen6 */
+			if (INTEL_INFO(dev)->gen >= 6)
+				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
+		}
+		break;
+	default:
+		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
+		ret = -EINVAL;
+		goto error;
+	}
+
 	if (args->num_cliprects != 0) {
 		if (ring != &dev_priv->ring[RCS]) {
 			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
@@ -1085,6 +1122,12 @@  legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 		}
 	}
 
+	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
+		ret = i915_reset_gen7_sol_offsets(dev, ring);
+		if (ret)
+			goto error;
+	}
+
 	ret = i915_gem_execbuffer_move_to_gpu(ring, vmas);
 	if (ret)
 		goto error;
@@ -1093,43 +1136,6 @@  legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		goto error;
 
-	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-	instp_mask = I915_EXEC_CONSTANTS_MASK;
-	switch (instp_mode) {
-	case I915_EXEC_CONSTANTS_REL_GENERAL:
-	case I915_EXEC_CONSTANTS_ABSOLUTE:
-	case I915_EXEC_CONSTANTS_REL_SURFACE:
-		if (instp_mode != 0 && ring != &dev_priv->ring[RCS]) {
-			DRM_DEBUG("non-0 rel constants mode on non-RCS\n");
-			ret = -EINVAL;
-			goto error;
-		}
-
-		if (instp_mode != dev_priv->relative_constants_mode) {
-			if (INTEL_INFO(dev)->gen < 4) {
-				DRM_DEBUG("no rel constants on pre-gen4\n");
-				ret = -EINVAL;
-				goto error;
-			}
-
-			if (INTEL_INFO(dev)->gen > 5 &&
-			    instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
-				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
-				ret = -EINVAL;
-				goto error;
-			}
-
-			/* The HW changed the meaning on this bit on gen6 */
-			if (INTEL_INFO(dev)->gen >= 6)
-				instp_mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE;
-		}
-		break;
-	default:
-		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
-		ret = -EINVAL;
-		goto error;
-	}
-
 	if (ring == &dev_priv->ring[RCS] &&
 			instp_mode != dev_priv->relative_constants_mode) {
 		ret = intel_ring_begin(ring, 4);
@@ -1145,12 +1151,6 @@  legacy_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 		dev_priv->relative_constants_mode = instp_mode;
 	}
 
-	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
-		ret = i915_reset_gen7_sol_offsets(dev, ring);
-		if (ret)
-			goto error;
-	}
-
 	exec_len = args->batch_len;
 	if (cliprects) {
 		for (i = 0; i < args->num_cliprects; i++) {