* [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
@ 2016-12-24 19:31 Chris Wilson
2016-12-24 20:23 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-12-24 20:52 ` [PATCH] " Ceraolo Spurio, Daniele
0 siblings, 2 replies; 5+ messages in thread
From: Chris Wilson @ 2016-12-24 19:31 UTC (permalink / raw)
To: intel-gfx
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@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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);
+ return offset;
+}
+
#endif
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
2016-12-24 19:31 [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable Chris Wilson
@ 2016-12-24 20:23 ` Patchwork
2016-12-24 20:52 ` [PATCH] " Ceraolo Spurio, Daniele
1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2016-12-24 20:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
URL : https://patchwork.freedesktop.org/series/17200/
State : warning
== Summary ==
Series 17200v1 drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
https://patchwork.freedesktop.org/api/1.0/series/17200/revisions/1/mbox/
Test kms_force_connector_basic:
Subgroup force-edid:
pass -> DMESG-WARN (fi-snb-2520m)
Test kms_pipe_crc_basic:
Subgroup nonblocking-crc-pipe-a:
pass -> DMESG-WARN (fi-snb-2520m)
fi-bdw-5557u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-bsw-n3050 total:246 pass:207 dwarn:0 dfail:0 fail:0 skip:39
fi-bxt-j4205 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22
fi-bxt-t5700 total:82 pass:69 dwarn:0 dfail:0 fail:0 skip:12
fi-byt-j1900 total:246 pass:219 dwarn:0 dfail:0 fail:0 skip:27
fi-byt-n2820 total:246 pass:215 dwarn:0 dfail:0 fail:0 skip:31
fi-hsw-4770 total:246 pass:227 dwarn:0 dfail:0 fail:0 skip:19
fi-hsw-4770r total:246 pass:227 dwarn:0 dfail:0 fail:0 skip:19
fi-ivb-3520m total:246 pass:225 dwarn:0 dfail:0 fail:0 skip:21
fi-ivb-3770 total:246 pass:225 dwarn:0 dfail:0 fail:0 skip:21
fi-kbl-7500u total:246 pass:225 dwarn:0 dfail:0 fail:0 skip:21
fi-skl-6260u total:246 pass:233 dwarn:0 dfail:0 fail:0 skip:13
fi-skl-6700hq total:246 pass:226 dwarn:0 dfail:0 fail:0 skip:20
fi-skl-6700k total:246 pass:222 dwarn:3 dfail:0 fail:0 skip:21
fi-skl-6770hq total:246 pass:233 dwarn:0 dfail:0 fail:0 skip:13
fi-snb-2520m total:246 pass:213 dwarn:2 dfail:0 fail:0 skip:31
fi-snb-2600 total:246 pass:214 dwarn:0 dfail:0 fail:0 skip:32
34544fe6abaa651211d871af4978060e72abd040 drm-tip: 2016y-12m-24d-10h-12m-42s UTC integration manifest
ee9c6ab drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3390/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
2016-12-24 19:31 [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable Chris Wilson
2016-12-24 20:23 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-12-24 20:52 ` Ceraolo Spurio, Daniele
2016-12-24 21:22 ` Chris Wilson
1 sibling, 1 reply; 5+ messages in thread
From: Ceraolo Spurio, Daniele @ 2016-12-24 20:52 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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@intel.com>
BR,
Daniele
> + return offset;
> +}
> +
> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
2016-12-24 20:52 ` [PATCH] " Ceraolo Spurio, Daniele
@ 2016-12-24 21:22 ` Chris Wilson
2016-12-24 21:25 ` Ceraolo Spurio, Daniele
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2016-12-24 21:22 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele; +Cc: intel-gfx
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@chris-wilson.co.uk>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
2016-12-24 21:22 ` Chris Wilson
@ 2016-12-24 21:25 ` Ceraolo Spurio, Daniele
0 siblings, 0 replies; 5+ messages in thread
From: Ceraolo Spurio, Daniele @ 2016-12-24 21:25 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 12/24/2016 1:22 PM, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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@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
>
Oh well, in that case I'll trust you to update the check after the fix
is merged ;-)
You can apply my r-b to this version.
Thanks,
Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-24 21:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-24 19:31 [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable Chris Wilson
2016-12-24 20:23 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-12-24 20:52 ` [PATCH] " Ceraolo Spurio, Daniele
2016-12-24 21:22 ` Chris Wilson
2016-12-24 21:25 ` Ceraolo Spurio, Daniele
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.