[Beignet] [RFC PATCH 3/3] drm/i915: add SVM execbuf ioctl v13

[Chris Wilson]

On Tue, Feb 07, 2017 at 02:11:41PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> >> +static struct intel_engine_cs *
> >> +exec_mm_select_engine(const struct drm_i915_private *dev_priv,
> >> +		      const struct drm_i915_exec_mm *exec_mm)
> >> +{
> >> +	const unsigned int user_ring_id = exec_mm->ring_id;
> >> +	struct intel_engine_cs *engine;
> >> +
> >> +	if (user_ring_id > I915_USER_RINGS) {
> >> +		DRM_DEBUG("exec_mm with unknown ring: %u\n", user_ring_id);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	engine = dev_priv->engine[user_ring_map[user_ring_id]];
> >> +
> >> +	if (!engine) {
> >> +		DRM_DEBUG("exec_mm with invalid engine: %u\n", user_ring_id);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	return engine;
> >> +}
> >
> > I would rather we reuse eb_select_engine(). We know we have to fix the
> > ABI anyway, so might as well kill 2 birds with one stone.
> >
> 
> I am still hesistant about this. Tried but this would expose the exec
> svm flags to similar kind of trickery for BSD ring.

My point was that ABI needs fixing. So rather than fixing it twice, do
it right once.

> >> +static int do_exec_mm(struct drm_i915_exec_mm *exec_mm,
> >> +		      struct drm_i915_gem_request *req,
> >> +		      const u32 flags)
> >> +{
> >> +	struct sync_file *out_fence = NULL;
> >> +	struct dma_fence *in_fence = NULL;
> >> +	int out_fence_fd = -1;
> >> +	int ret;
> >> +
> >> +	if (flags & I915_EXEC_MM_FENCE_IN) {
> >> +		in_fence = sync_file_get_fence(exec_mm->in_fence_fd);
> >> +		if (!in_fence)
> >> +			return -EINVAL;
> >> +
> >> +		ret = i915_gem_request_await_dma_fence(req, in_fence);
> >
> > You can drop the fence ref immediately.
> 
> So closely mimiced from explicit fences that execbuf path
> might be able to do the same :)

Not quite. We do the easy error checking in execbuf before we allocate
the request and commit hw resources. Being picky, exec_svm should do the
same.

> >> +{
> >> +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> +	struct drm_i915_exec_mm *exec_mm = data;
> >> +	struct intel_engine_cs *engine;
> >> +	struct i915_gem_context *ctx;
> >> +	struct drm_i915_gem_request *req;
> >> +	const u32 ctx_id = exec_mm->ctx_id;
> >> +	const u32 flags = exec_mm->flags;
> >> +	int ret;
> >> +
> >> +	if (exec_mm->batch_ptr & 3) {
> >> +		DRM_DEBUG_DRIVER("ptr not 4-byt aligned %llu\n",
> >> +				 exec_mm->batch_ptr);
> >> +		return -EINVAL;
> >> +	}
> >
> > Where does access_ok() get performed? A little spiel on the mm validation
> > would help here if it is all done for us by svm.
> >
> 
> Passing in length wouldn't help much as the refs inside the bb
> would also need to be checked?

Sure, I'm quite happy for a comment on why this is special and doesn't
need access validation checks. But we need something.

For example, why even bother checking for ptr alignment? It is just
another faulting instruction.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre