[Intel-gfx] [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
Chris Wilson
chris at chris-wilson.co.uk
Sat Dec 24 21:22:17 UTC 2016
On Sat, Dec 24, 2016 at 12:52:37PM -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 12/24/2016 11:31 AM, Chris Wilson wrote:
> >Add an assertion to the plain i915_ggtt_offset() to double check that
> >any offset we hand to the GuC is outside of its unmappable ranges.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> >---
> > drivers/gpu/drm/i915/i915_guc_submission.c | 14 +++++++-------
> > drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++---
> > drivers/gpu/drm/i915/intel_uc.h | 9 +++++++++
> > 3 files changed, 19 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >index 3e20fe2be811..30e012b9e93c 100644
> >--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >@@ -270,11 +270,11 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
> > /* The state page is after PPHWSP */
> > lrc->ring_lcra =
> >- i915_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
> >+ guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
> > lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
> > (guc_engine_id << GUC_ELC_ENGINE_OFFSET);
> >- lrc->ring_begin = i915_ggtt_offset(ce->ring->vma);
> >+ lrc->ring_begin = guc_ggtt_offset(ce->ring->vma);
> > lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
> > lrc->ring_next_free_location = lrc->ring_begin;
> > lrc->ring_current_tail_pointer_value = 0;
> >@@ -290,7 +290,7 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
> > * The doorbell, process descriptor, and workqueue are all parts
> > * of the client object, which the GuC will reference via the GGTT
> > */
> >- gfx_addr = i915_ggtt_offset(client->vma);
> >+ gfx_addr = guc_ggtt_offset(client->vma);
> > desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
> > client->doorbell_offset;
> > desc.db_trigger_cpu =
> >@@ -1226,7 +1226,7 @@ static void guc_log_create(struct intel_guc *guc)
> > (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> > (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> >- offset = i915_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> >+ offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> > guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> > }
> >@@ -1329,7 +1329,7 @@ static void guc_addon_create(struct intel_guc *guc)
> > guc_policies_init(policies);
> > ads->scheduler_policies =
> >- i915_ggtt_offset(vma) + sizeof(struct guc_ads);
> >+ guc_ggtt_offset(vma) + sizeof(struct guc_ads);
> > /* MMIO reg state */
> > reg_state = (void *)policies + sizeof(struct guc_policies);
> >@@ -1495,7 +1495,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
> > /* any value greater than GUC_POWER_D0 */
> > data[1] = GUC_POWER_D1;
> > /* first page is shared data with GuC */
> >- data[2] = i915_ggtt_offset(ctx->engine[RCS].state);
> >+ data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
> > return intel_guc_send(guc, data, ARRAY_SIZE(data));
> > }
> >@@ -1522,7 +1522,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
> > data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> > data[1] = GUC_POWER_D0;
> > /* first page is shared data with GuC */
> >- data[2] = i915_ggtt_offset(ctx->engine[RCS].state);
> >+ data[2] = guc_ggtt_offset(ctx->engine[RCS].state);
> > return intel_guc_send(guc, data, ARRAY_SIZE(data));
> > }
> >diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> >index 21db69705458..35d5690f47a2 100644
> >--- a/drivers/gpu/drm/i915/intel_guc_loader.c
> >+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> >@@ -220,14 +220,14 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
> > params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
> > if (guc->ads_vma) {
> >- u32 ads = i915_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> >+ u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> > params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
> > params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
> > }
> > /* If GuC submission is enabled, set up additional parameters here */
> > if (i915.enable_guc_submission) {
> >- u32 pgs = i915_ggtt_offset(dev_priv->guc.ctx_pool_vma);
> >+ u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool_vma);
> > u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
> > pgs >>= PAGE_SHIFT;
> >@@ -297,7 +297,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
> > I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size);
> > /* Set the source address for the new blob */
> >- offset = i915_ggtt_offset(vma) + guc_fw->header_offset;
> >+ offset = guc_ggtt_offset(vma) + guc_fw->header_offset;
> > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
> > I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
> >diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> >index 11f56082b363..d556215e691f 100644
> >--- a/drivers/gpu/drm/i915/intel_uc.h
> >+++ b/drivers/gpu/drm/i915/intel_uc.h
> >@@ -28,6 +28,8 @@
> > #include "i915_guc_reg.h"
> > #include "intel_ringbuffer.h"
> >+#include "i915_vma.h"
> >+
> > struct drm_i915_gem_request;
> > /*
> >@@ -198,4 +200,11 @@ void i915_guc_register(struct drm_i915_private *dev_priv);
> > void i915_guc_unregister(struct drm_i915_private *dev_priv);
> > int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
> >+static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> >+{
> >+ u32 offset = i915_ggtt_offset(vma);
> >+ GEM_BUG_ON(offset < GUC_WOPCM_TOP);
>
> GEM_BUG_ON(offset < GUC_WOPCM_TOP || offset > 0xFEE00000);
>
> Maybe we could add a define for 0xFEE00000 as we might have to
> re-use it elsewhere.
>
> With the change for the GEM_BUG_ON:
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
I wasn't going to do the upper check until we have the fix in place
(i.e. that patch to apply the limit would add the assertion as well). No
point in intentionally panicking the kernel, just hang the hw instead!
;)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
More information about the Intel-gfx
mailing list