[Intel-gfx] [PATCH] drm/i915/guc: Assert that all GGTT offsets used by the GuC are mappable
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Sat Dec 24 21:25:27 UTC 2016
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 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
>
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
More information about the Intel-gfx
mailing list