| Message ID | 1516325372-24448-2-git-send-email-yaodong.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/19/2018 6:59 AM, Jackie Li wrote: > GuC related exported functions should start with "intel_guc_" > prefix and pass intel_guc as the first parameter since its guc > related. Current guc_ggtt_offset() failed to follow this code > convention. > > This patch renames the guc_ggtt_offset to intel_guc_ggtt_offset > and updates the related code to pass intel_guc pointer to > this function call. so that we have a unified coding style for > GuC code. > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Jackie Li <yaodong.li@intel.com> > --- > drivers/gpu/drm/i915/intel_guc.c | 12 +++++++----- > drivers/gpu/drm/i915/intel_guc.h | 10 ++++++++-- > drivers/gpu/drm/i915/intel_guc_ads.c | 25 +++++++++++++------------ > drivers/gpu/drm/i915/intel_guc_ct.c | 5 +++-- > drivers/gpu/drm/i915/intel_guc_fw.c | 2 +- > drivers/gpu/drm/i915/intel_guc_log.c | 2 +- > drivers/gpu/drm/i915/intel_guc_submission.c | 10 +++++----- > drivers/gpu/drm/i915/intel_huc.c | 6 ++++-- > 8 files changed, 42 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c > index b8b6d4a..e70885b 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -264,8 +264,10 @@ void intel_guc_init_params(struct intel_guc *guc) > > /* If GuC submission is enabled, set up additional parameters here */ > if (USES_GUC_SUBMISSION(dev_priv)) { > - u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; > - u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool); > + u32 ads = intel_guc_ggtt_offset(guc, > + guc->ads_vma) >> PAGE_SHIFT; > + u32 pgs = intel_guc_ggtt_offset(guc, > + dev_priv->guc.stage_desc_pool); > u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16; > > params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; > @@ -413,7 +415,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) > data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; > /* any value greater than GUC_POWER_D0 */ > data[1] = GUC_POWER_D1; > - data[2] = guc_ggtt_offset(guc->shared_data); > + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); > > return intel_guc_send(guc, data, ARRAY_SIZE(data)); > } > @@ -436,7 +438,7 @@ int intel_guc_reset_engine(struct intel_guc *guc, > data[3] = 0; > data[4] = 0; > data[5] = guc->execbuf_client->stage_id; > - data[6] = guc_ggtt_offset(guc->shared_data); > + data[6] = intel_guc_ggtt_offset(guc, guc->shared_data); > > return intel_guc_send(guc, data, ARRAY_SIZE(data)); > } > @@ -458,7 +460,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) > > data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; > data[1] = GUC_POWER_D0; > - data[2] = guc_ggtt_offset(guc->shared_data); > + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); > > return intel_guc_send(guc, data, ARRAY_SIZE(data)); > } > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 9e0a97e..b7e2a18 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -101,13 +101,19 @@ static inline void intel_guc_notify(struct intel_guc *guc) > guc->notify(guc); > } > > -/* Should add "/**" instead of removing "/*" > +/* intel_guc_ggtt_offset() - Get the GGTT offset of @vma. I feel this function is more validating the offset so "Validate and get the ggtt offset of @vma" ? > + * @guc: intel guc. > + * @vma: i915 graphics virtual memory area. > + * > * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP), > * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is > * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects > * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM. > + * > + * Return: GGTT offset that meets the GuC gfx address requirement. > */ > -static inline u32 guc_ggtt_offset(struct i915_vma *vma) > +static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc, > + struct i915_vma *vma) > { > u32 offset = i915_ggtt_offset(vma); > This function uses GUC_GGTT_TOP that is defined in guc_reg.h and I think we can move it to guc.h similar to other WOPCM related moves in earlier patch. > diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c > index ac62753..7215594 100644 > --- a/drivers/gpu/drm/i915/intel_guc_ads.c > +++ b/drivers/gpu/drm/i915/intel_guc_ads.c > @@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc) > blob->reg_state.white_list[engine->guc_id].count = 0; > } > > - /* > - * The GuC requires a "Golden Context" when it reinitialises > - * engines after a reset. Here we use the Render ring default > - * context, which must already exist and be pinned in the GGTT, > - * so its address won't change after we've told the GuC where > - * to find it. Note that we have to skip our header (1 page), > - * because our GuC shared data is there. > - */ > - blob->ads.golden_context_lrca = > - guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + > - skipped_offset; > This move seems unnecessary > /* > * The GuC expects us to exclude the portion of the context image that > @@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc) > blob->ads.eng_state_size[engine->guc_id] = > engine->context_size - skipped_size; > > - base = guc_ggtt_offset(vma); > + base = intel_guc_ggtt_offset(guc, vma); > blob->ads.scheduler_policies = base + ptr_offset(blob, policies); > blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer); > blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state); > > + /* > + * The GuC requires a "Golden Context" when it reinitialises > + * engines after a reset. Here we use the Render ring default > + * context, which must already exist and be pinned in the GGTT, > + * so its address won't change after we've told the GuC where > + * to find it. Note that we have to skip our header (1 page), > + * because our GuC shared data is there. > + */ > + vma = dev_priv->kernel_context->engine[RCS].state; > + blob->ads.golden_context_lrca = intel_guc_ggtt_offset(guc, vma) + > + skipped_offset; > + > kunmap(page); > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c > index 24ad557..0a0d3d5 100644 > --- a/drivers/gpu/drm/i915/intel_guc_ct.c > +++ b/drivers/gpu/drm/i915/intel_guc_ct.c > @@ -156,7 +156,8 @@ static int ctch_init(struct intel_guc *guc, > err = PTR_ERR(blob); > goto err_vma; > } > - DRM_DEBUG_DRIVER("CT: vma base=%#x\n", guc_ggtt_offset(ctch->vma)); > + DRM_DEBUG_DRIVER("CT: vma base=%#x\n", > + intel_guc_ggtt_offset(guc, ctch->vma)); > > /* store pointers to desc and cmds */ > for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) { > @@ -202,7 +203,7 @@ static int ctch_open(struct intel_guc *guc, > } > > /* vma should be already allocated and map'ed */ > - base = guc_ggtt_offset(ctch->vma); > + base = intel_guc_ggtt_offset(guc, ctch->vma); > > /* (re)initialize descriptors > * cmds buffers are in the second half of the blob page > diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c > index 3b09329..178d339 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fw.c > +++ b/drivers/gpu/drm/i915/intel_guc_fw.c > @@ -165,7 +165,7 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma) > I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size); > > /* Set the source address for the new blob */ > - offset = guc_ggtt_offset(vma) + guc_fw->header_offset; > + offset = intel_guc_ggtt_offset(guc, 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_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index 2ffc966..4efe564 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -549,7 +549,7 @@ int intel_guc_log_create(struct intel_guc *guc) > (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) | > (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT); > > - offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */ > + offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT; /* in pages */ > guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c > index 1f3a878..c56e4f4 100644 > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > @@ -375,8 +375,8 @@ static void guc_stage_desc_init(struct intel_guc *guc, > lrc->context_desc = lower_32_bits(ce->lrc_desc); > > /* The state page is after PPHWSP */ > - lrc->ring_lrca = > - guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE; > + lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) + > + LRC_STATE_PN * PAGE_SIZE; > > /* XXX: In direct submission, the GuC wants the HW context id > * here. In proxy submission, it wants the stage id > @@ -384,7 +384,7 @@ static void guc_stage_desc_init(struct intel_guc *guc, > lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) | > (guc_engine_id << GUC_ELC_ENGINE_OFFSET); > > - lrc->ring_begin = guc_ggtt_offset(ce->ring->vma); > + lrc->ring_begin = intel_guc_ggtt_offset(guc, 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; > @@ -400,7 +400,7 @@ static void guc_stage_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 = guc_ggtt_offset(client->vma); > + gfx_addr = intel_guc_ggtt_offset(guc, client->vma); > desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) + > client->doorbell_offset; > desc->db_trigger_cpu = ptr_to_u64(__get_doorbell(client)); > @@ -596,7 +596,7 @@ static void inject_preempt_context(struct work_struct *work) > data[3] = engine->guc_id; > data[4] = guc->execbuf_client->priority; > data[5] = guc->execbuf_client->stage_id; > - data[6] = guc_ggtt_offset(guc->shared_data); > + data[6] = intel_guc_ggtt_offset(guc, guc->shared_data); > > if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) { > execlists_clear_active(&engine->execlists, > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c > index 8ed0518..aed9c1c 100644 > --- a/drivers/gpu/drm/i915/intel_huc.c > +++ b/drivers/gpu/drm/i915/intel_huc.c > @@ -137,7 +137,8 @@ static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma) > intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > > /* Set the source address for the uCode */ > - offset = guc_ggtt_offset(vma) + huc_fw->header_offset; > + offset = intel_guc_ggtt_offset(&dev_priv->guc, vma) + > + huc_fw->header_offset; > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); > I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF); > > @@ -213,7 +214,8 @@ int intel_huc_auth(struct intel_huc *huc) > } > > ret = intel_guc_auth_huc(guc, > - guc_ggtt_offset(vma) + huc->fw.rsa_offset); > + intel_guc_ggtt_offset(guc, vma) + > + huc->fw.rsa_offset); > if (ret) { > DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret); > goto out;
Quoting Jackie Li (2018-01-19 01:29:28) > GuC related exported functions should start with "intel_guc_" > prefix and pass intel_guc as the first parameter since its guc > related. Current guc_ggtt_offset() failed to follow this code > convention. But it was not, and still does not operate on the guc. Is that changing? -Chris
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c >> b/drivers/gpu/drm/i915/intel_guc_ads.c >> index ac62753..7215594 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_ads.c >> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c >> @@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc) >> blob->reg_state.white_list[engine->guc_id].count = 0; >> } >> - /* >> - * The GuC requires a "Golden Context" when it reinitialises >> - * engines after a reset. Here we use the Render ring default >> - * context, which must already exist and be pinned in the GGTT, >> - * so its address won't change after we've told the GuC where >> - * to find it. Note that we have to skip our header (1 page), >> - * because our GuC shared data is there. >> - */ >> - blob->ads.golden_context_lrca = >> - guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + >> - skipped_offset; > This move seems unnecessary This move is mainly for reusing "vma" variable so that the line won't get too long. Besides, this move can also make sure the assignment will get closer to the code that actually uses the value from "blob->ads.golden_context_lrca":-) >> /* >> * The GuC expects us to exclude the portion of the context >> image that >> @@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc) >> blob->ads.eng_state_size[engine->guc_id] = >> engine->context_size - skipped_size; >> - base = guc_ggtt_offset(vma); >> + base = intel_guc_ggtt_offset(guc, vma); >> blob->ads.scheduler_policies = base + ptr_offset(blob, policies); >> blob->ads.reg_state_buffer = base + ptr_offset(blob, >> reg_state_buffer); >> blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state); >> + /* >> + * The GuC requires a "Golden Context" when it reinitialises >> + * engines after a reset. Here we use the Render ring default >> + * context, which must already exist and be pinned in the GGTT, >> + * so its address won't change after we've told the GuC where >> + * to find it. Note that we have to skip our header (1 page), >> + * because our GuC shared data is there. >> + */ >> + vma = dev_priv->kernel_context->engine[RCS].state; >> + blob->ads.golden_context_lrca = intel_guc_ggtt_offset(guc, vma) + >> + skipped_offset; >> + >> kunmap(page); >> return 0;
On 01/31/2018 11:38 PM, Chris Wilson wrote: > Quoting Jackie Li (2018-01-19 01:29:28) >> GuC related exported functions should start with "intel_guc_" >> prefix and pass intel_guc as the first parameter since its guc >> related. Current guc_ggtt_offset() failed to follow this code >> convention. > But it was not, and still does not operate on the guc. Is that changing? this problem is that it's guc related and the following patches do need to access the data from intel_guc. Do you think it's getting better if I add a sentence like "the future patches will need to access the intel_guc to verify the offset"? Regards, -Jackie
Quoting Yaodong Li (2018-02-01 19:47:53) > > On 01/31/2018 11:38 PM, Chris Wilson wrote: > > Quoting Jackie Li (2018-01-19 01:29:28) > >> GuC related exported functions should start with "intel_guc_" > >> prefix and pass intel_guc as the first parameter since its guc > >> related. Current guc_ggtt_offset() failed to follow this code > >> convention. > > But it was not, and still does not operate on the guc. Is that changing? > this problem is that it's guc related and the following patches do need > to access the data from intel_guc. Do you think it's getting better > if I add a sentence like "the future patches will need to access > the intel_guc to verify the offset"? That's the idea. You need to explain _why_ you need a particular change, in some cases like this where it's not clear from the context of the patch, you need to fill in the missing details for the reader. In patch series like this where there is upfront refactoring required, remember the reader is starting at the beginning with no idea of what's coming next, so a bit^Wlot of foreshadowing is required in the story you tell. -Chris
On 02/01/2018 02:35 PM, Chris Wilson wrote: > Quoting Yaodong Li (2018-02-01 19:47:53) >> On 01/31/2018 11:38 PM, Chris Wilson wrote: >>> Quoting Jackie Li (2018-01-19 01:29:28) >>>> GuC related exported functions should start with "intel_guc_" >>>> prefix and pass intel_guc as the first parameter since its guc >>>> related. Current guc_ggtt_offset() failed to follow this code >>>> convention. >>> But it was not, and still does not operate on the guc. Is that changing? >> this problem is that it's guc related and the following patches do need >> to access the data from intel_guc. Do you think it's getting better >> if I add a sentence like "the future patches will need to access >> the intel_guc to verify the offset"? > That's the idea. You need to explain _why_ you need a particular change, > in some cases like this where it's not clear from the context of the > patch, you need to fill in the missing details for the reader. In patch > series like this where there is upfront refactoring required, remember > the reader is starting at the beginning with no idea of what's coming > next, so a bit^Wlot of foreshadowing is required in the story you tell. Got it. Will keep that in mind. Thank you Chris:-) Regards, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index b8b6d4a..e70885b 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -264,8 +264,10 @@ void intel_guc_init_params(struct intel_guc *guc) /* If GuC submission is enabled, set up additional parameters here */ if (USES_GUC_SUBMISSION(dev_priv)) { - u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; - u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool); + u32 ads = intel_guc_ggtt_offset(guc, + guc->ads_vma) >> PAGE_SHIFT; + u32 pgs = intel_guc_ggtt_offset(guc, + dev_priv->guc.stage_desc_pool); u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16; params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; @@ -413,7 +415,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; /* any value greater than GUC_POWER_D0 */ data[1] = GUC_POWER_D1; - data[2] = guc_ggtt_offset(guc->shared_data); + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -436,7 +438,7 @@ int intel_guc_reset_engine(struct intel_guc *guc, data[3] = 0; data[4] = 0; data[5] = guc->execbuf_client->stage_id; - data[6] = guc_ggtt_offset(guc->shared_data); + data[6] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -458,7 +460,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; data[1] = GUC_POWER_D0; - data[2] = guc_ggtt_offset(guc->shared_data); + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 9e0a97e..b7e2a18 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -101,13 +101,19 @@ static inline void intel_guc_notify(struct intel_guc *guc) guc->notify(guc); } -/* +/* intel_guc_ggtt_offset() - Get the GGTT offset of @vma. + * @guc: intel guc. + * @vma: i915 graphics virtual memory area. + * * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP), * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM. + * + * Return: GGTT offset that meets the GuC gfx address requirement. */ -static inline u32 guc_ggtt_offset(struct i915_vma *vma) +static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc, + struct i915_vma *vma) { u32 offset = i915_ggtt_offset(vma); diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c index ac62753..7215594 100644 --- a/drivers/gpu/drm/i915/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/intel_guc_ads.c @@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc) blob->reg_state.white_list[engine->guc_id].count = 0; } - /* - * The GuC requires a "Golden Context" when it reinitialises - * engines after a reset. Here we use the Render ring default - * context, which must already exist and be pinned in the GGTT, - * so its address won't change after we've told the GuC where - * to find it. Note that we have to skip our header (1 page), - * because our GuC shared data is there. - */ - blob->ads.golden_context_lrca = - guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + - skipped_offset; /* * The GuC expects us to exclude the portion of the context image that @@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc) blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size; - base = guc_ggtt_offset(vma); + base = intel_guc_ggtt_offset(guc, vma); blob->ads.scheduler_policies = base + ptr_offset(blob, policies); blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer); blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state); + /* + * The GuC requires a "Golden Context" when it reinitialises + * engines after a reset. Here we use the Render ring default + * context, which must already exist and be pinned in the GGTT, + * so its address won't change after we've told the GuC where + * to find it. Note that we have to skip our header (1 page), + * because our GuC shared data is there. + */ + vma = dev_priv->kernel_context->engine[RCS].state; + blob->ads.golden_context_lrca = intel_guc_ggtt_offset(guc, vma) + + skipped_offset; + kunmap(page); return 0; diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c index 24ad557..0a0d3d5 100644 --- a/drivers/gpu/drm/i915/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/intel_guc_ct.c @@ -156,7 +156,8 @@ static int ctch_init(struct intel_guc *guc, err = PTR_ERR(blob); goto err_vma; } - DRM_DEBUG_DRIVER("CT: vma base=%#x\n", guc_ggtt_offset(ctch->vma)); + DRM_DEBUG_DRIVER("CT: vma base=%#x\n", + intel_guc_ggtt_offset(guc, ctch->vma)); /* store pointers to desc and cmds */ for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) { @@ -202,7 +203,7 @@ static int ctch_open(struct intel_guc *guc, } /* vma should be already allocated and map'ed */ - base = guc_ggtt_offset(ctch->vma); + base = intel_guc_ggtt_offset(guc, ctch->vma); /* (re)initialize descriptors * cmds buffers are in the second half of the blob page diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c index 3b09329..178d339 100644 --- a/drivers/gpu/drm/i915/intel_guc_fw.c +++ b/drivers/gpu/drm/i915/intel_guc_fw.c @@ -165,7 +165,7 @@ static int guc_xfer_ucode(struct intel_guc *guc, struct i915_vma *vma) I915_WRITE(DMA_COPY_SIZE, guc_fw->header_size + guc_fw->ucode_size); /* Set the source address for the new blob */ - offset = guc_ggtt_offset(vma) + guc_fw->header_offset; + offset = intel_guc_ggtt_offset(guc, 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_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 2ffc966..4efe564 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -549,7 +549,7 @@ int intel_guc_log_create(struct intel_guc *guc) (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) | (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT); - offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */ + offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT; /* in pages */ guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; return 0; diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index 1f3a878..c56e4f4 100644 --- a/drivers/gpu/drm/i915/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/intel_guc_submission.c @@ -375,8 +375,8 @@ static void guc_stage_desc_init(struct intel_guc *guc, lrc->context_desc = lower_32_bits(ce->lrc_desc); /* The state page is after PPHWSP */ - lrc->ring_lrca = - guc_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE; + lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) + + LRC_STATE_PN * PAGE_SIZE; /* XXX: In direct submission, the GuC wants the HW context id * here. In proxy submission, it wants the stage id @@ -384,7 +384,7 @@ static void guc_stage_desc_init(struct intel_guc *guc, lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) | (guc_engine_id << GUC_ELC_ENGINE_OFFSET); - lrc->ring_begin = guc_ggtt_offset(ce->ring->vma); + lrc->ring_begin = intel_guc_ggtt_offset(guc, 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; @@ -400,7 +400,7 @@ static void guc_stage_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 = guc_ggtt_offset(client->vma); + gfx_addr = intel_guc_ggtt_offset(guc, client->vma); desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) + client->doorbell_offset; desc->db_trigger_cpu = ptr_to_u64(__get_doorbell(client)); @@ -596,7 +596,7 @@ static void inject_preempt_context(struct work_struct *work) data[3] = engine->guc_id; data[4] = guc->execbuf_client->priority; data[5] = guc->execbuf_client->stage_id; - data[6] = guc_ggtt_offset(guc->shared_data); + data[6] = intel_guc_ggtt_offset(guc, guc->shared_data); if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) { execlists_clear_active(&engine->execlists, diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index 8ed0518..aed9c1c 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -137,7 +137,8 @@ static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma) intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); /* Set the source address for the uCode */ - offset = guc_ggtt_offset(vma) + huc_fw->header_offset; + offset = intel_guc_ggtt_offset(&dev_priv->guc, vma) + + huc_fw->header_offset; I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset)); I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF); @@ -213,7 +214,8 @@ int intel_huc_auth(struct intel_huc *huc) } ret = intel_guc_auth_huc(guc, - guc_ggtt_offset(vma) + huc->fw.rsa_offset); + intel_guc_ggtt_offset(guc, vma) + + huc->fw.rsa_offset); if (ret) { DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret); goto out;
GuC related exported functions should start with "intel_guc_" prefix and pass intel_guc as the first parameter since its guc related. Current guc_ggtt_offset() failed to follow this code convention. This patch renames the guc_ggtt_offset to intel_guc_ggtt_offset and updates the related code to pass intel_guc pointer to this function call. so that we have a unified coding style for GuC code. Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Jackie Li <yaodong.li@intel.com> --- drivers/gpu/drm/i915/intel_guc.c | 12 +++++++----- drivers/gpu/drm/i915/intel_guc.h | 10 ++++++++-- drivers/gpu/drm/i915/intel_guc_ads.c | 25 +++++++++++++------------ drivers/gpu/drm/i915/intel_guc_ct.c | 5 +++-- drivers/gpu/drm/i915/intel_guc_fw.c | 2 +- drivers/gpu/drm/i915/intel_guc_log.c | 2 +- drivers/gpu/drm/i915/intel_guc_submission.c | 10 +++++----- drivers/gpu/drm/i915/intel_huc.c | 6 ++++-- 8 files changed, 42 insertions(+), 30 deletions(-)