Re: [PATCH v4 1/6] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

Chris Wilson
2015-06-16T20:33:43Z

On Tue, Jun 16, 2015 at 08:25:20PM +0100, Arun Siluvery wrote:
> +static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *ring, u32 size)
> +{
> + int ret;
> + struct drm_device *dev = ring->dev;

You only use it once, keeping it as a local seems counter-intuitive.

> + WARN_ON(ring->id != RCS);
> +
> + size = roundup(size, PAGE_SIZE);

Out of curiousity is gcc smart enough to turn this into an ALIGN()?

> + ring->wa_ctx.obj = i915_gem_alloc_object(dev, size);
> + if (!ring->wa_ctx.obj) {
> + DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
> + return -ENOMEM;
> + }
> +
> + ret = i915_gem_obj_ggtt_pin(ring->wa_ctx.obj, GEN8_LR_CONTEXT_ALIGN, 0);

Strange choice of alignment since we pass around cacheline offsets.

> + if (ret) {
> + DRM_DEBUG_DRIVER("pin LRC WA ctx backing obj failed: %d\n",
> + ret);
> + drm_gem_object_unreference(&ring->wa_ctx.obj->base);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void lrc_destroy_wa_ctx_obj(struct intel_engine_cs *ring)
> +{
> + WARN_ON(ring->id != RCS);
> +
> + i915_gem_object_ggtt_unpin(ring->wa_ctx.obj);
> + drm_gem_object_unreference(&ring->wa_ctx.obj->base);
> + ring->wa_ctx.obj = NULL;
> +}
> +
> /**
> * intel_logical_ring_cleanup() - deallocate the Engine Command Streamer
> *
> @@ -1474,7 +1612,29 @@ static int logical_render_ring_init(struct drm_device *dev)
> if (ret)
> return ret;
>
> - return intel_init_pipe_control(ring);
> + if (INTEL_INFO(ring->dev)->gen >= 8) {
> + ret = lrc_setup_wa_ctx_obj(ring, PAGE_SIZE);
> + if (ret) {
> + DRM_DEBUG_DRIVER("Failed to setup context WA page: %d\n",
> + ret);
> + return ret;
> + }
> +
> + ret = intel_init_workaround_bb(ring);
> + if (ret) {
> + lrc_destroy_wa_ctx_obj(ring);
> + DRM_ERROR("WA batch buffers are not initialized: %d\n",
> + ret);
> + }
> + }
> +
> + ret = intel_init_pipe_control(ring);

Did you consider stuffing it into the spare are of the pipe control
scatch bo? :)
-Chris

--
Chris Wilson, Intel Open Source Technology Centre