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 <ch...@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vet...@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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
