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

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
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