On Wed, 17 Oct 2012 12:09:54 +0100
Chris Wilson <chris at chris-wilson.co.uk> wrote:
> With the introduction of per-process GTT space, the hardware designers
> thought it wise to also limit the ability to write to MMIO space to only
> a "secure" batch buffer. The ability to rewrite registers is the only
> way to program the hardware to perform certain operations like scanline
> waits (required for tear-free windowed updates). So we either have a
> choice of adding an interface to perform those synchronized updates
> inside the kernel, or we permit certain processes the ability to write
> to the "safe" registers from within its command stream. This patch
> exposes the ability to submit a SECURE batch buffer to
> DRM_ROOT_ONLY|DRM_MASTER processes.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 ++++++++++++++++++---
> drivers/gpu/drm/i915/i915_trace.h | 10 ++++++----
> drivers/gpu/drm/i915/intel_ringbuffer.c | 23 +++++++++++++++--------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++-
> include/drm/i915_drm.h | 6 ++++++
> 6 files changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5779e8f..f92c849 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1015,6 +1015,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> case I915_PARAM_HAS_PRIME_VMAP_FLUSH:
> value = 1;
> break;
> + case I915_PARAM_HAS_SECURE_BATCHES:
> + value = capable(CAP_SYS_ADMIN);
> + break;
> default:
> DRM_DEBUG_DRIVER("Unknown parameter %d\n",
> param->param);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index bcd5aa2..4bbd088 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -816,6 +816,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> u32 exec_start, exec_len;
> u32 seqno;
> u32 mask;
> + u32 flags;
> int ret, mode, i;
>
> if (!i915_gem_check_execbuffer(args)) {
> @@ -827,6 +828,14 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> if (ret)
> return ret;
>
> + flags = 0;
> + if (args->flags & I915_EXEC_SECURE) {
> + if (!file->is_master || !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + flags |= I915_DISPATCH_SECURE;
> + }
> +
> switch (args->flags & I915_EXEC_RING_MASK) {
> case I915_EXEC_DEFAULT:
> case I915_EXEC_RENDER:
> @@ -999,6 +1008,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> }
> batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
>
> + if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
> + i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
> +
> ret = i915_gem_execbuffer_move_to_gpu(ring, &objects);
> if (ret)
> goto err;
> @@ -1044,7 +1056,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto err;
> }
>
> - trace_i915_gem_ring_dispatch(ring, seqno);
> + trace_i915_gem_ring_dispatch(ring, seqno, flags);
>
> exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> exec_len = args->batch_len;
> @@ -1056,12 +1068,15 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> goto err;
>
> ret = ring->dispatch_execbuffer(ring,
> - exec_start, exec_len);
> + exec_start, exec_len,
> + flags);
> if (ret)
> goto err;
> }
> } else {
> - ret = ring->dispatch_execbuffer(ring, exec_start, exec_len);
> + ret = ring->dispatch_execbuffer(ring,
> + exec_start, exec_len,
> + flags);
> if (ret)
> goto err;
> }
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 8134421..3db4a68 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -229,24 +229,26 @@ TRACE_EVENT(i915_gem_evict_everything,
> );
>
> TRACE_EVENT(i915_gem_ring_dispatch,
> - TP_PROTO(struct intel_ring_buffer *ring, u32 seqno),
> - TP_ARGS(ring, seqno),
> + TP_PROTO(struct intel_ring_buffer *ring, u32 seqno, u32 flags),
> + TP_ARGS(ring, seqno, flags),
>
> TP_STRUCT__entry(
> __field(u32, dev)
> __field(u32, ring)
> __field(u32, seqno)
> + __field(u32, flags)
> ),
>
> TP_fast_assign(
> __entry->dev = ring->dev->primary->index;
> __entry->ring = ring->id;
> __entry->seqno = seqno;
> + __entry->flags = flags;
> i915_trace_irq_get(ring, seqno);
> ),
>
> - TP_printk("dev=%u, ring=%u, seqno=%u",
> - __entry->dev, __entry->ring, __entry->seqno)
> + TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x",
> + __entry->dev, __entry->ring, __entry->seqno, __entry->flags)
> );
>
> TRACE_EVENT(i915_gem_ring_flush,
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 133beb6..6a4b8fa 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -964,7 +964,9 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
> }
>
> static int
> -i965_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
> +i965_dispatch_execbuffer(struct intel_ring_buffer *ring,
> + u32 offset, u32 length,
> + unsigned flags)
> {
> int ret;
>
> @@ -975,7 +977,7 @@ i965_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
> intel_ring_emit(ring,
> MI_BATCH_BUFFER_START |
> MI_BATCH_GTT |
> - MI_BATCH_NON_SECURE_I965);
> + (flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_I965));
> intel_ring_emit(ring, offset);
> intel_ring_advance(ring);
>
> @@ -984,7 +986,8 @@ i965_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length)
>
> static int
> i830_dispatch_execbuffer(struct intel_ring_buffer *ring,
> - u32 offset, u32 len)
> + u32 offset, u32 len,
> + unsigned flags)
> {
> int ret;
>
> @@ -993,7 +996,7 @@ i830_dispatch_execbuffer(struct intel_ring_buffer *ring,
> return ret;
>
> intel_ring_emit(ring, MI_BATCH_BUFFER);
> - intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE);
> + intel_ring_emit(ring, offset | (flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE));
> intel_ring_emit(ring, offset + len - 8);
> intel_ring_emit(ring, 0);
> intel_ring_advance(ring);
> @@ -1003,7 +1006,8 @@ i830_dispatch_execbuffer(struct intel_ring_buffer *ring,
>
> static int
> i915_dispatch_execbuffer(struct intel_ring_buffer *ring,
> - u32 offset, u32 len)
> + u32 offset, u32 len,
> + unsigned flags)
> {
> int ret;
>
> @@ -1012,7 +1016,7 @@ i915_dispatch_execbuffer(struct intel_ring_buffer *ring,
> return ret;
>
> intel_ring_emit(ring, MI_BATCH_BUFFER_START | MI_BATCH_GTT);
> - intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE);
> + intel_ring_emit(ring, offset | (flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE));
> intel_ring_advance(ring);
>
> return 0;
> @@ -1410,7 +1414,8 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
>
> static int
> gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
> - u32 offset, u32 len)
> + u32 offset, u32 len,
> + unsigned flags)
> {
> int ret;
>
> @@ -1418,7 +1423,9 @@ gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
> if (ret)
> return ret;
>
> - intel_ring_emit(ring, MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965);
> + intel_ring_emit(ring,
> + MI_BATCH_BUFFER_START |
> + (flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_I965));
> /* bit0-7 is the length on GEN6+ */
> intel_ring_emit(ring, offset);
> intel_ring_advance(ring);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 40b252e..2667f33 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -81,7 +81,9 @@ struct intel_ring_buffer {
> u32 (*get_seqno)(struct intel_ring_buffer *ring,
> bool lazy_coherency);
> int (*dispatch_execbuffer)(struct intel_ring_buffer *ring,
> - u32 offset, u32 length);
> + u32 offset, u32 length,
> + unsigned flags);
> +#define I915_DISPATCH_SECURE 0x1
> void (*cleanup)(struct intel_ring_buffer *ring);
> int (*sync_to)(struct intel_ring_buffer *ring,
> struct intel_ring_buffer *to,
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index acfb377..4f41f8d 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -316,6 +316,7 @@ typedef struct drm_i915_irq_wait {
> #define I915_PARAM_HAS_SEMAPHORES 20
> #define I915_PARAM_HAS_PRIME_VMAP_FLUSH 21
> #define I915_PARAM_RSVD_FOR_FUTURE_USE 22
> +#define I915_PARAM_HAS_SECURE_BATCHES 23
>
> typedef struct drm_i915_getparam {
> int param;
> @@ -694,6 +695,11 @@ struct drm_i915_gem_execbuffer2 {
> /** Resets the SO write offset registers for transform feedback on gen7. */
> #define I915_EXEC_GEN7_SOL_RESET (1<<8)
>
> +/** Request a privileged ("secure") batch buffer. Note only available for
> + * DRM_ROOT_ONLY | DRM_MASTER processes.
> + */
> +#define I915_EXEC_SECURE (1<<9)
> +
> #define I915_EXEC_CONTEXT_ID_MASK (0xffffffff)
> #define i915_execbuffer2_set_context_id(eb2, context) \
> (eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
Yeah looks good.
Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
