[Intel-gfx] [PATCH 7/8] drm/i915: Asynchronous cmdparser

[Joonas Lahtinen]

+ Daniel/Maarten for the dma_resv

Quoting Chris Wilson (2019-12-07 19:01:09)
> Execute the cmdparser asynchronously as part of the submission pipeline.
> Using our dma-fences, we can schedule execution after an asynchronous
> piece of work, so we move the cmdparser out from under the struct_mutex
> inside execbuf as run it as part of the submission pipeline. The same
> security rules apply, we copy the user batch before validation and
> userspace cannot touch the validation shadow. The only caveat is that we
> will do request construction before we complete cmdparsing and so we
> cannot know the outcome of the validation step until later -- so the
> execbuf ioctl does not report -EINVAL directly, but we must cancel
> execution of the request and flag the error on the out-fence.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>

<SNIP>

> +static const struct dma_fence_work_ops eb_parse_ops = {
> +       .name = "parse",

I'm noticing all our dma_fence_work_ops are named very briefly.

.name = "eb_parse" might be in order.

> +       .work = __eb_parse,
> +};
> +
> +static int eb_parse_pipeline(struct i915_execbuffer *eb,
> +                            struct i915_vma *shadow)
> +{
> +       struct eb_parse_work *pw;
> +       int err;
> +
> +       pw = kzalloc(sizeof(*pw), GFP_KERNEL);
> +       if (!pw)
> +               return -ENOMEM;
> +
> +       dma_fence_work_init(&pw->base, &eb_parse_ops);
> +
> +       pw->engine = eb->engine;
> +       pw->batch = eb->batch;
> +       pw->batch_offset = eb->batch_start_offset;
> +       pw->batch_length = eb->batch_len;
> +       pw->shadow = shadow;
> +
> +       dma_resv_lock(pw->batch->resv, NULL);
> +       err = dma_resv_reserve_shared(pw->batch->resv, 1);
> +       if (err) {
> +               dma_resv_unlock(pw->batch->resv);
> +               kfree(pw);
> +               return err;
> +       }
> +
> +       err = i915_sw_fence_await_reservation(&pw->base.chain,
> +                                             pw->batch->resv, NULL, false,
> +                                             0, I915_FENCE_GFP);
> +       if (err < 0) {

Onion teardown to dedupe code.

> +               dma_resv_unlock(pw->batch->resv);
> +               kfree(pw);
> +               return err;
> +       }
> +
> +       dma_resv_add_shared_fence(pw->batch->resv, &pw->base.dma);
> +       dma_resv_unlock(pw->batch->resv);
> +
> +       dma_resv_lock(shadow->resv, NULL);
> +       dma_resv_add_excl_fence(shadow->resv, &pw->base.dma);
> +       dma_resv_unlock(shadow->resv);
> +
> +       dma_fence_work_commit(&pw->base);
> +       return 0;
> +}

After de-duping, I think this is just fine as far as the fences come.

Kernel wouldn't initiate any requests in need of cmd parsing and some
work needs to be waited upon to free memory, the cmdparser will fail
gracefully as the only allocation is __GFP_RETRY_MAYFAIL.

The rest looks fine to me, too. We probably want another set of eyes
to also ack the clflushing correctness.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Regards, Joonas