Dave Gordon <david.s.gordon@xxxxxxxxx> writes:
> Existing code did a kunmap() on the wrong page, and didn't account for the
> fact that the HWSP and the default context are different offsets within the
> same GEM object.
>
Just to note here that usually we try to split bug fixes early in the
series with minimal code changes. This helps cherrypickups
to fixes/stable. And also lessens the probability that we accidentally
revert bugfix when some other problem occurs with the patch.
> This patch improves symmetry by creating lrc_teardown_hardware_status_page()
> to complement lrc_setup_hardware_status_page(), making sure that they both
> take the same argument (pointer to engine). They encapsulate all the details
> of the relationship between the default context and the status page, so the
> rest of the LRC code doesn't have to know about it.
>
> Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 57 ++++++++++++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 73d4347..3914120 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -226,9 +226,8 @@ enum {
> #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT 0x17
>
> static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
> -static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> - struct drm_i915_gem_object *default_ctx_obj);
> -
> +static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring);
> +static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring);
>
> /**
> * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
> @@ -1536,8 +1535,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
> struct drm_i915_private *dev_priv = dev->dev_private;
> u8 next_context_status_buffer_hw;
>
> - lrc_setup_hardware_status_page(ring,
> - dev_priv->kernel_context->engine[ring->id].state);
> + lrc_setup_hardware_status_page(ring);
>
> I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
> I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
> @@ -1992,10 +1990,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
> i915_cmd_parser_fini_ring(ring);
> i915_gem_batch_pool_fini(&ring->batch_pool);
>
> - if (ring->status_page.obj) {
> - kunmap(sg_page(ring->status_page.obj->pages->sgl));
> - ring->status_page.obj = NULL;
> - }
> + /* Status page should have gone already */
> + WARN_ON(ring->status_page.page_addr);
> + WARN_ON(ring->status_page.obj);
>
> ring->disable_lite_restore_wa = false;
> ring->ctx_desc_template = 0;
> @@ -2434,6 +2431,11 @@ void intel_lr_context_free(struct intel_context *ctx)
> continue;
>
> if (ctx == ctx->i915->kernel_context) {
> + /*
> + * The HWSP is part of the default context
> + * object in LRC mode.
> + */
> + lrc_teardown_hardware_status_page(ringbuf->ring);
> intel_unpin_ringbuffer_obj(ringbuf);
> i915_gem_object_ggtt_unpin(ctx_obj);
> }
> @@ -2482,24 +2484,45 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *ring)
> return ret;
> }
>
> -static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> - struct drm_i915_gem_object *default_ctx_obj)
> +static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring)
> {
> - struct drm_i915_private *dev_priv = ring->dev->dev_private;
> + struct drm_i915_private *dev_priv = to_i915(ring->dev);
> + struct intel_context *dctx = dev_priv->kernel_context;
> + struct drm_i915_gem_object *dctx_obj = dctx->engine[ring->id].state;
> + u64 dctx_addr = i915_gem_obj_ggtt_offset(dctx_obj);
> struct page *page;
>
> - /* The HWSP is part of the default context object in LRC mode. */
> - ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(default_ctx_obj)
> - + LRC_PPHWSP_PN * PAGE_SIZE;
> - page = i915_gem_object_get_page(default_ctx_obj, LRC_PPHWSP_PN);
> + /*
> + * The HWSP is part of the default context object in LRC mode.
> + * Note that it doesn't contribute to the refcount!
> + */
> + page = i915_gem_object_get_page(dctx_obj, LRC_PPHWSP_PN);
> ring->status_page.page_addr = kmap(page);
> - ring->status_page.obj = default_ctx_obj;
> + ring->status_page.gfx_addr = dctx_addr + LRC_PPHWSP_PN * PAGE_SIZE;
> + ring->status_page.obj = dctx_obj;
>
> I915_WRITE(RING_HWS_PGA(ring->mmio_base),
> (u32)ring->status_page.gfx_addr);
> POSTING_READ(RING_HWS_PGA(ring->mmio_base));
> }
>
> +/* This should be called before the default context is destroyed */
> +static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring)
> +{
> + struct drm_i915_gem_object *dctx_obj = ring->status_page.obj;
> + struct page *page;
> +
> + WARN_ON(!dctx_obj);
> +
> + if (ring->status_page.page_addr) {
> + page = i915_gem_object_get_page(dctx_obj, LRC_PPHWSP_PN);
> + kunmap(page);
> + ring->status_page.page_addr = NULL;
> + }
> +
As the page is always kmapped along with setting of status_page.obj,
I fail to see why we need the check here for the page_addr.
How about just:
if (WARN_ON(!dctx_obj))
return;
-Mika
> + ring->status_page.obj = NULL;
> +}
> +
> /**
> * intel_lr_context_deferred_alloc() - create the LRC specific bits of a context
> * @ctx: LR context to create.
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
