diff mbox

[1/2] drm/i915: Kill DRI1 cliprects

Message ID 1444127996-23561-1-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson Oct. 6, 2015, 10:39 a.m. UTC
Passing cliprects into the kernel for it to re-execute the batch buffer
with different CMD_DRAWRECT died out long ago. As DRI1 support has been
removed from the kernel, we can now simply reject any execbuf trying to
use this "feature".

To keep Daniel happy with the prospect of being able to reuse these
fields in the next decade, continue to ensure that current userspace is
not passing garbage in through the dead fields.

v2: Fix the cliprects_ptr check

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
 drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
 2 files changed, 31 insertions(+), 138 deletions(-)

Comments

Daniel Vetter Oct. 6, 2015, 11:21 a.m. UTC | #1
On Tue, Oct 06, 2015 at 11:39:55AM +0100, Chris Wilson wrote:
> Passing cliprects into the kernel for it to re-execute the batch buffer
> with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> removed from the kernel, we can now simply reject any execbuf trying to
> use this "feature".
> 
> To keep Daniel happy with the prospect of being able to reuse these
> fields in the next decade, continue to ensure that current userspace is
> not passing garbage in through the dead fields.
> 
> v2: Fix the cliprects_ptr check
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

igt subtest seems to be missing to ensure we enforce this. Yay otherwise!
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
>  drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
>  2 files changed, 31 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 75a0c8b5305b..045a7631faa0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -947,7 +947,21 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
>  	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
>  		return false;
>  
> -	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
> +	/* Kernel clipping was a DRI1 misfeature */
> +	if (exec->num_cliprects || exec->cliprects_ptr)
> +		return false;
> +
> +	if (exec->DR4 == 0xffffffff) {
> +		DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> +		exec->DR4 = 0;
> +	}
> +	if (exec->DR1 || exec->DR4)
> +		return false;
> +
> +	if ((exec->batch_start_offset | exec->batch_len) & 0x7)
> +		return false;
> +
> +	return true;
>  }
>  
>  static int
> @@ -1111,47 +1125,6 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static int
> -i915_emit_box(struct drm_i915_gem_request *req,
> -	      struct drm_clip_rect *box,
> -	      int DR1, int DR4)
> -{
> -	struct intel_engine_cs *ring = req->ring;
> -	int ret;
> -
> -	if (box->y2 <= box->y1 || box->x2 <= box->x1 ||
> -	    box->y2 <= 0 || box->x2 <= 0) {
> -		DRM_ERROR("Bad box %d,%d..%d,%d\n",
> -			  box->x1, box->y1, box->x2, box->y2);
> -		return -EINVAL;
> -	}
> -
> -	if (INTEL_INFO(ring->dev)->gen >= 4) {
> -		ret = intel_ring_begin(req, 4);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO_I965);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -	} else {
> -		ret = intel_ring_begin(req, 6);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO);
> -		intel_ring_emit(ring, DR1);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -		intel_ring_emit(ring, 0);
> -	}
> -	intel_ring_advance(ring);
> -
> -	return 0;
> -}
> -
>  static struct drm_i915_gem_object*
>  i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
>  			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
> @@ -1210,65 +1183,21 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  			       struct drm_i915_gem_execbuffer2 *args,
>  			       struct list_head *vmas)
>  {
> -	struct drm_clip_rect *cliprects = NULL;
>  	struct drm_device *dev = params->dev;
>  	struct intel_engine_cs *ring = params->ring;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u64 exec_start, exec_len;
>  	int instp_mode;
>  	u32 instp_mask;
> -	int i, ret = 0;
> -
> -	if (args->num_cliprects != 0) {
> -		if (ring != &dev_priv->ring[RCS]) {
> -			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
> -			return -EINVAL;
> -		}
> -
> -		if (INTEL_INFO(dev)->gen >= 5) {
> -			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -			return -EINVAL;
> -		}
> -
> -		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
> -			DRM_DEBUG("execbuf with %u cliprects\n",
> -				  args->num_cliprects);
> -			return -EINVAL;
> -		}
> -
> -		cliprects = kcalloc(args->num_cliprects,
> -				    sizeof(*cliprects),
> -				    GFP_KERNEL);
> -		if (cliprects == NULL) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> -
> -		if (copy_from_user(cliprects,
> -				   to_user_ptr(args->cliprects_ptr),
> -				   sizeof(*cliprects)*args->num_cliprects)) {
> -			ret = -EFAULT;
> -			goto error;
> -		}
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> +	int ret;
>  
>  	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	ret = i915_switch_context(params->request);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
>  	     "%s didn't clear reload\n", ring->name);
> @@ -1281,22 +1210,19 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  	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;
> +			return -EINVAL;
>  		}
>  
>  		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;
> +				return -EINVAL;
>  			}
>  
>  			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;
> +				return -EINVAL;
>  			}
>  
>  			/* The HW changed the meaning on this bit on gen6 */
> @@ -1306,15 +1232,14 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  		break;
>  	default:
>  		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> -		ret = -EINVAL;
> -		goto error;
> +		return -EINVAL;
>  	}
>  
>  	if (ring == &dev_priv->ring[RCS] &&
> -			instp_mode != dev_priv->relative_constants_mode) {
> +	    instp_mode != dev_priv->relative_constants_mode) {
>  		ret = intel_ring_begin(params->request, 4);
>  		if (ret)
> -			goto error;
> +			return ret;
>  
>  		intel_ring_emit(ring, MI_NOOP);
>  		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> @@ -1328,42 +1253,25 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>  		ret = i915_reset_gen7_sol_offsets(dev, params->request);
>  		if (ret)
> -			goto error;
> +			return ret;
>  	}
>  
>  	exec_len   = args->batch_len;
>  	exec_start = params->batch_obj_vm_offset +
>  		     params->args_batch_start_offset;
>  
> -	if (cliprects) {
> -		for (i = 0; i < args->num_cliprects; i++) {
> -			ret = i915_emit_box(params->request, &cliprects[i],
> -					    args->DR1, args->DR4);
> -			if (ret)
> -				goto error;
> -
> -			ret = ring->dispatch_execbuffer(params->request,
> -							exec_start, exec_len,
> -							params->dispatch_flags);
> -			if (ret)
> -				goto error;
> -		}
> -	} else {
> -		ret = ring->dispatch_execbuffer(params->request,
> -						exec_start, exec_len,
> -						params->dispatch_flags);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ring->dispatch_execbuffer(params->request,
> +					exec_start, exec_len,
> +					params->dispatch_flags);
> +	if (ret)
> +		return ret;
>  
>  	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>  
>  	i915_gem_execbuffer_move_to_active(vmas, params->request);
>  	i915_gem_execbuffer_retire_commands(params);
>  
> -error:
> -	kfree(cliprects);
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 78aab946db04..d38746c5370d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -878,21 +878,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  		return -EINVAL;
>  	}
>  
> -	if (args->num_cliprects != 0) {
> -		DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -		return -EINVAL;
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>  	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>  		DRM_DEBUG("sol reset is gen7 only\n");
>  		return -EINVAL;
> -- 
> 2.6.0
>
Chris Wilson Oct. 6, 2015, 12:43 p.m. UTC | #2
On Tue, Oct 06, 2015 at 01:21:25PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 11:39:55AM +0100, Chris Wilson wrote:
> > Passing cliprects into the kernel for it to re-execute the batch buffer
> > with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> > removed from the kernel, we can now simply reject any execbuf trying to
> > use this "feature".
> > 
> > To keep Daniel happy with the prospect of being able to reuse these
> > fields in the next decade, continue to ensure that current userspace is
> > not passing garbage in through the dead fields.
> > 
> > v2: Fix the cliprects_ptr check
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> igt subtest seems to be missing to ensure we enforce this. Yay otherwise!

Looking at gem_exec_params/cliprects-invalid, presumably all you want to
do is to remove the igt_require(). To be more precise you would need to
a flag to tell when the wicked witch is dead and you could expect the
execbuf to fail with any cliprect garbage.
-Chris
> -Da
Tvrtko Ursulin Oct. 6, 2015, 2:19 p.m. UTC | #3
On 06/10/15 11:39, Chris Wilson wrote:
> Passing cliprects into the kernel for it to re-execute the batch buffer
> with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> removed from the kernel, we can now simply reject any execbuf trying to
> use this "feature".
>
> To keep Daniel happy with the prospect of being able to reuse these
> fields in the next decade, continue to ensure that current userspace is
> not passing garbage in through the dead fields.
>
> v2: Fix the cliprects_ptr check
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Don't know anything about the DRI1 history but the removal looks fine to 
me, so:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Would it also make sense to rename the related fields in 
drm_i915_gem_execbuffer2 to reserved?

Regards,

Tvrtko
Dave Gordon Oct. 6, 2015, 2:29 p.m. UTC | #4
On 06/10/15 11:39, Chris Wilson wrote:
> Passing cliprects into the kernel for it to re-execute the batch buffer
> with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> removed from the kernel, we can now simply reject any execbuf trying to
> use this "feature".
>
> To keep Daniel happy with the prospect of being able to reuse these
> fields in the next decade, continue to ensure that current userspace is
> not passing garbage in through the dead fields.
>
> v2: Fix the cliprects_ptr check
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
>   drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
>   2 files changed, 31 insertions(+), 138 deletions(-)

This will cause John Harrison & myself a certain amount of pain (because 
the changes collide with the scheduler's reorganisation of the 
execbuffer path), but I'm all in favour of getting rid of the legacy 
crud cluttering up this code, so ...

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Perhaps we could get rid of relative-constants-mode next :)

> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 75a0c8b5305b..045a7631faa0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -947,7 +947,21 @@ i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
>   	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
>   		return false;
>
> -	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
> +	/* Kernel clipping was a DRI1 misfeature */
> +	if (exec->num_cliprects || exec->cliprects_ptr)
> +		return false;
> +
> +	if (exec->DR4 == 0xffffffff) {
> +		DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> +		exec->DR4 = 0;
> +	}
> +	if (exec->DR1 || exec->DR4)
> +		return false;
> +
> +	if ((exec->batch_start_offset | exec->batch_len) & 0x7)
> +		return false;
> +
> +	return true;
>   }
>
>   static int
> @@ -1111,47 +1125,6 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
>   	return 0;
>   }
>
> -static int
> -i915_emit_box(struct drm_i915_gem_request *req,
> -	      struct drm_clip_rect *box,
> -	      int DR1, int DR4)
> -{
> -	struct intel_engine_cs *ring = req->ring;
> -	int ret;
> -
> -	if (box->y2 <= box->y1 || box->x2 <= box->x1 ||
> -	    box->y2 <= 0 || box->x2 <= 0) {
> -		DRM_ERROR("Bad box %d,%d..%d,%d\n",
> -			  box->x1, box->y1, box->x2, box->y2);
> -		return -EINVAL;
> -	}
> -
> -	if (INTEL_INFO(ring->dev)->gen >= 4) {
> -		ret = intel_ring_begin(req, 4);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO_I965);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -	} else {
> -		ret = intel_ring_begin(req, 6);
> -		if (ret)
> -			return ret;
> -
> -		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO);
> -		intel_ring_emit(ring, DR1);
> -		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
> -		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
> -		intel_ring_emit(ring, DR4);
> -		intel_ring_emit(ring, 0);
> -	}
> -	intel_ring_advance(ring);
> -
> -	return 0;
> -}
> -
>   static struct drm_i915_gem_object*
>   i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
>   			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
> @@ -1210,65 +1183,21 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   			       struct drm_i915_gem_execbuffer2 *args,
>   			       struct list_head *vmas)
>   {
> -	struct drm_clip_rect *cliprects = NULL;
>   	struct drm_device *dev = params->dev;
>   	struct intel_engine_cs *ring = params->ring;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	u64 exec_start, exec_len;
>   	int instp_mode;
>   	u32 instp_mask;
> -	int i, ret = 0;
> -
> -	if (args->num_cliprects != 0) {
> -		if (ring != &dev_priv->ring[RCS]) {
> -			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
> -			return -EINVAL;
> -		}
> -
> -		if (INTEL_INFO(dev)->gen >= 5) {
> -			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -			return -EINVAL;
> -		}
> -
> -		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
> -			DRM_DEBUG("execbuf with %u cliprects\n",
> -				  args->num_cliprects);
> -			return -EINVAL;
> -		}
> -
> -		cliprects = kcalloc(args->num_cliprects,
> -				    sizeof(*cliprects),
> -				    GFP_KERNEL);
> -		if (cliprects == NULL) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> -
> -		if (copy_from_user(cliprects,
> -				   to_user_ptr(args->cliprects_ptr),
> -				   sizeof(*cliprects)*args->num_cliprects)) {
> -			ret = -EFAULT;
> -			goto error;
> -		}
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> +	int ret;
>
>   	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
>   	if (ret)
> -		goto error;
> +		return ret;
>
>   	ret = i915_switch_context(params->request);
>   	if (ret)
> -		goto error;
> +		return ret;
>
>   	WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
>   	     "%s didn't clear reload\n", ring->name);
> @@ -1281,22 +1210,19 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   	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;
> +			return -EINVAL;
>   		}
>
>   		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;
> +				return -EINVAL;
>   			}
>
>   			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;
> +				return -EINVAL;
>   			}
>
>   			/* The HW changed the meaning on this bit on gen6 */
> @@ -1306,15 +1232,14 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   		break;
>   	default:
>   		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
> -		ret = -EINVAL;
> -		goto error;
> +		return -EINVAL;
>   	}
>
>   	if (ring == &dev_priv->ring[RCS] &&
> -			instp_mode != dev_priv->relative_constants_mode) {
> +	    instp_mode != dev_priv->relative_constants_mode) {
>   		ret = intel_ring_begin(params->request, 4);
>   		if (ret)
> -			goto error;
> +			return ret;
>
>   		intel_ring_emit(ring, MI_NOOP);
>   		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> @@ -1328,42 +1253,25 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>   	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>   		ret = i915_reset_gen7_sol_offsets(dev, params->request);
>   		if (ret)
> -			goto error;
> +			return ret;
>   	}
>
>   	exec_len   = args->batch_len;
>   	exec_start = params->batch_obj_vm_offset +
>   		     params->args_batch_start_offset;
>
> -	if (cliprects) {
> -		for (i = 0; i < args->num_cliprects; i++) {
> -			ret = i915_emit_box(params->request, &cliprects[i],
> -					    args->DR1, args->DR4);
> -			if (ret)
> -				goto error;
> -
> -			ret = ring->dispatch_execbuffer(params->request,
> -							exec_start, exec_len,
> -							params->dispatch_flags);
> -			if (ret)
> -				goto error;
> -		}
> -	} else {
> -		ret = ring->dispatch_execbuffer(params->request,
> -						exec_start, exec_len,
> -						params->dispatch_flags);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = ring->dispatch_execbuffer(params->request,
> +					exec_start, exec_len,
> +					params->dispatch_flags);
> +	if (ret)
> +		return ret;
>
>   	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
>
>   	i915_gem_execbuffer_move_to_active(vmas, params->request);
>   	i915_gem_execbuffer_retire_commands(params);
>
> -error:
> -	kfree(cliprects);
> -	return ret;
> +	return 0;
>   }
>
>   /**
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 78aab946db04..d38746c5370d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -878,21 +878,6 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>   		return -EINVAL;
>   	}
>
> -	if (args->num_cliprects != 0) {
> -		DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
> -		return -EINVAL;
> -	} else {
> -		if (args->DR4 == 0xffffffff) {
> -			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
> -			args->DR4 = 0;
> -		}
> -
> -		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
> -			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>   	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
>   		DRM_DEBUG("sol reset is gen7 only\n");
>   		return -EINVAL;
>
Chris Wilson Oct. 6, 2015, 3:36 p.m. UTC | #5
On Tue, Oct 06, 2015 at 03:29:20PM +0100, Dave Gordon wrote:
> On 06/10/15 11:39, Chris Wilson wrote:
> >Passing cliprects into the kernel for it to re-execute the batch buffer
> >with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> >removed from the kernel, we can now simply reject any execbuf trying to
> >use this "feature".
> >
> >To keep Daniel happy with the prospect of being able to reuse these
> >fields in the next decade, continue to ensure that current userspace is
> >not passing garbage in through the dead fields.
> >
> >v2: Fix the cliprects_ptr check
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 154 ++++++-----------------------
> >  drivers/gpu/drm/i915/intel_lrc.c           |  15 ---
> >  2 files changed, 31 insertions(+), 138 deletions(-)
> 
> This will cause John Harrison & myself a certain amount of pain
> (because the changes collide with the scheduler's reorganisation of
> the execbuffer path), but I'm all in favour of getting rid of the
> legacy crud cluttering up this code, so ...

Oh, don't worry I'm completely rewriting it and will nak anything that
adds a cycle of latency to execbuffer.
-Chris
Chris Wilson Oct. 6, 2015, 3:37 p.m. UTC | #6
On Tue, Oct 06, 2015 at 03:19:36PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/10/15 11:39, Chris Wilson wrote:
> >Passing cliprects into the kernel for it to re-execute the batch buffer
> >with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> >removed from the kernel, we can now simply reject any execbuf trying to
> >use this "feature".
> >
> >To keep Daniel happy with the prospect of being able to reuse these
> >fields in the next decade, continue to ensure that current userspace is
> >not passing garbage in through the dead fields.
> >
> >v2: Fix the cliprects_ptr check
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Don't know anything about the DRI1 history but the removal looks
> fine to me, so:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Would it also make sense to rename the related fields in
> drm_i915_gem_execbuffer2 to reserved?

In our heads, yes. Renaming structs in uapi.h is harder than it looks.
-Chris
Daniel Vetter Oct. 7, 2015, 1:58 p.m. UTC | #7
On Tue, Oct 06, 2015 at 04:37:13PM +0100, Chris Wilson wrote:
> On Tue, Oct 06, 2015 at 03:19:36PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 06/10/15 11:39, Chris Wilson wrote:
> > >Passing cliprects into the kernel for it to re-execute the batch buffer
> > >with different CMD_DRAWRECT died out long ago. As DRI1 support has been
> > >removed from the kernel, we can now simply reject any execbuf trying to
> > >use this "feature".
> > >
> > >To keep Daniel happy with the prospect of being able to reuse these
> > >fields in the next decade, continue to ensure that current userspace is
> > >not passing garbage in through the dead fields.
> > >
> > >v2: Fix the cliprects_ptr check
> > >
> > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Don't know anything about the DRI1 history but the removal looks
> > fine to me, so:
> > 
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Queued for -next, thanks for the patch. Assuming the igt materializes ;-)
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 75a0c8b5305b..045a7631faa0 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -947,7 +947,21 @@  i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
 	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
 		return false;
 
-	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
+	/* Kernel clipping was a DRI1 misfeature */
+	if (exec->num_cliprects || exec->cliprects_ptr)
+		return false;
+
+	if (exec->DR4 == 0xffffffff) {
+		DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
+		exec->DR4 = 0;
+	}
+	if (exec->DR1 || exec->DR4)
+		return false;
+
+	if ((exec->batch_start_offset | exec->batch_len) & 0x7)
+		return false;
+
+	return true;
 }
 
 static int
@@ -1111,47 +1125,6 @@  i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
-static int
-i915_emit_box(struct drm_i915_gem_request *req,
-	      struct drm_clip_rect *box,
-	      int DR1, int DR4)
-{
-	struct intel_engine_cs *ring = req->ring;
-	int ret;
-
-	if (box->y2 <= box->y1 || box->x2 <= box->x1 ||
-	    box->y2 <= 0 || box->x2 <= 0) {
-		DRM_ERROR("Bad box %d,%d..%d,%d\n",
-			  box->x1, box->y1, box->x2, box->y2);
-		return -EINVAL;
-	}
-
-	if (INTEL_INFO(ring->dev)->gen >= 4) {
-		ret = intel_ring_begin(req, 4);
-		if (ret)
-			return ret;
-
-		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO_I965);
-		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
-		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
-		intel_ring_emit(ring, DR4);
-	} else {
-		ret = intel_ring_begin(req, 6);
-		if (ret)
-			return ret;
-
-		intel_ring_emit(ring, GFX_OP_DRAWRECT_INFO);
-		intel_ring_emit(ring, DR1);
-		intel_ring_emit(ring, (box->x1 & 0xffff) | box->y1 << 16);
-		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
-		intel_ring_emit(ring, DR4);
-		intel_ring_emit(ring, 0);
-	}
-	intel_ring_advance(ring);
-
-	return 0;
-}
-
 static struct drm_i915_gem_object*
 i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
 			  struct drm_i915_gem_exec_object2 *shadow_exec_entry,
@@ -1210,65 +1183,21 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 			       struct drm_i915_gem_execbuffer2 *args,
 			       struct list_head *vmas)
 {
-	struct drm_clip_rect *cliprects = NULL;
 	struct drm_device *dev = params->dev;
 	struct intel_engine_cs *ring = params->ring;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u64 exec_start, exec_len;
 	int instp_mode;
 	u32 instp_mask;
-	int i, ret = 0;
-
-	if (args->num_cliprects != 0) {
-		if (ring != &dev_priv->ring[RCS]) {
-			DRM_DEBUG("clip rectangles are only valid with the render ring\n");
-			return -EINVAL;
-		}
-
-		if (INTEL_INFO(dev)->gen >= 5) {
-			DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
-			return -EINVAL;
-		}
-
-		if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
-			DRM_DEBUG("execbuf with %u cliprects\n",
-				  args->num_cliprects);
-			return -EINVAL;
-		}
-
-		cliprects = kcalloc(args->num_cliprects,
-				    sizeof(*cliprects),
-				    GFP_KERNEL);
-		if (cliprects == NULL) {
-			ret = -ENOMEM;
-			goto error;
-		}
-
-		if (copy_from_user(cliprects,
-				   to_user_ptr(args->cliprects_ptr),
-				   sizeof(*cliprects)*args->num_cliprects)) {
-			ret = -EFAULT;
-			goto error;
-		}
-	} else {
-		if (args->DR4 == 0xffffffff) {
-			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
-			args->DR4 = 0;
-		}
-
-		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
-			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
-			return -EINVAL;
-		}
-	}
+	int ret;
 
 	ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas);
 	if (ret)
-		goto error;
+		return ret;
 
 	ret = i915_switch_context(params->request);
 	if (ret)
-		goto error;
+		return ret;
 
 	WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
 	     "%s didn't clear reload\n", ring->name);
@@ -1281,22 +1210,19 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 	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;
+			return -EINVAL;
 		}
 
 		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;
+				return -EINVAL;
 			}
 
 			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;
+				return -EINVAL;
 			}
 
 			/* The HW changed the meaning on this bit on gen6 */
@@ -1306,15 +1232,14 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 		break;
 	default:
 		DRM_DEBUG("execbuf with unknown constants: %d\n", instp_mode);
-		ret = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 
 	if (ring == &dev_priv->ring[RCS] &&
-			instp_mode != dev_priv->relative_constants_mode) {
+	    instp_mode != dev_priv->relative_constants_mode) {
 		ret = intel_ring_begin(params->request, 4);
 		if (ret)
-			goto error;
+			return ret;
 
 		intel_ring_emit(ring, MI_NOOP);
 		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
@@ -1328,42 +1253,25 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
 		ret = i915_reset_gen7_sol_offsets(dev, params->request);
 		if (ret)
-			goto error;
+			return ret;
 	}
 
 	exec_len   = args->batch_len;
 	exec_start = params->batch_obj_vm_offset +
 		     params->args_batch_start_offset;
 
-	if (cliprects) {
-		for (i = 0; i < args->num_cliprects; i++) {
-			ret = i915_emit_box(params->request, &cliprects[i],
-					    args->DR1, args->DR4);
-			if (ret)
-				goto error;
-
-			ret = ring->dispatch_execbuffer(params->request,
-							exec_start, exec_len,
-							params->dispatch_flags);
-			if (ret)
-				goto error;
-		}
-	} else {
-		ret = ring->dispatch_execbuffer(params->request,
-						exec_start, exec_len,
-						params->dispatch_flags);
-		if (ret)
-			return ret;
-	}
+	ret = ring->dispatch_execbuffer(params->request,
+					exec_start, exec_len,
+					params->dispatch_flags);
+	if (ret)
+		return ret;
 
 	trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags);
 
 	i915_gem_execbuffer_move_to_active(vmas, params->request);
 	i915_gem_execbuffer_retire_commands(params);
 
-error:
-	kfree(cliprects);
-	return ret;
+	return 0;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 78aab946db04..d38746c5370d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -878,21 +878,6 @@  int intel_execlists_submission(struct i915_execbuffer_params *params,
 		return -EINVAL;
 	}
 
-	if (args->num_cliprects != 0) {
-		DRM_DEBUG("clip rectangles are only valid on pre-gen5\n");
-		return -EINVAL;
-	} else {
-		if (args->DR4 == 0xffffffff) {
-			DRM_DEBUG("UXA submitting garbage DR4, fixing up\n");
-			args->DR4 = 0;
-		}
-
-		if (args->DR1 || args->DR4 || args->cliprects_ptr) {
-			DRM_DEBUG("0 cliprects but dirt in cliprects fields\n");
-			return -EINVAL;
-		}
-	}
-
 	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
 		DRM_DEBUG("sol reset is gen7 only\n");
 		return -EINVAL;