| Message ID | 1505382908-6844-3-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 14 Sep 2017 11:55:04 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > From: "Kamble, Sagar A" <sagar.a.kamble@intel.com> > > This patch is moving guc_enable_communication and > guc_disable_communication to intel_guc.c and making it available for > use through intel_guc.h. > Intent is to reuse this function for calling from intel_uc_init_hw > and also as part of intel_uc_fini_hw where it will be coupled with > other teardown related to GuC in the upcoming patch. Hmm, but both intel_uc_init_hw() and intel_uc_fini_hw() shall stay in intel_uc.c and the best place for guc_[enable|disable]_communication() functions is also in intel_uc.c as they provide generic switch between MMIO/CT send mechanism, so I'm not so sure that we need this patch. Michal > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/intel_guc.c | 21 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_guc.h | 2 ++ > drivers/gpu/drm/i915/intel_uc.c | 29 ++++------------------------- > 3 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c > b/drivers/gpu/drm/i915/intel_guc.c > index 5559e00..75bb830 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -181,3 +181,24 @@ void intel_guc_auth_huc(struct intel_guc *guc, > struct intel_huc *huc) > out: > i915_vma_unpin(vma); > } > + > +int intel_guc_enable_communication(struct intel_guc *guc) > +{ > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + > + if (HAS_GUC_CT(dev_priv)) > + return intel_guc_enable_ct(guc); > + > + guc->send = intel_guc_send_mmio; > + return 0; > +} > + > +void intel_guc_disable_communication(struct intel_guc *guc) > +{ > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + > + if (HAS_GUC_CT(dev_priv)) > + intel_guc_disable_ct(guc); > + > + guc->send = intel_guc_send_nop; > +} > diff --git a/drivers/gpu/drm/i915/intel_guc.h > b/drivers/gpu/drm/i915/intel_guc.h > index 9a282aa..8ed0e81 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -149,6 +149,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma > *vma) > int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 > len); > int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 > len); > void intel_guc_auth_huc(struct intel_guc *guc, struct intel_huc *huc); > +int intel_guc_enable_communication(struct intel_guc *guc); > +void intel_guc_disable_communication(struct intel_guc *guc); > /* intel_guc_loader.c */ > int intel_guc_select_fw(struct intel_guc *guc); > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index a3fc4c8..30c004c 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -265,27 +265,6 @@ static void guc_free_load_err_log(struct intel_guc > *guc) > i915_gem_object_put(guc->load_err_log); > } > -static int guc_enable_communication(struct intel_guc *guc) > -{ > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > - > - if (HAS_GUC_CT(dev_priv)) > - return intel_guc_enable_ct(guc); > - > - guc->send = intel_guc_send_mmio; > - return 0; > -} > - > -static void guc_disable_communication(struct intel_guc *guc) > -{ > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > - > - if (HAS_GUC_CT(dev_priv)) > - intel_guc_disable_ct(guc); > - > - guc->send = intel_guc_send_nop; > -} > - > int intel_uc_init_hw(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > @@ -295,7 +274,7 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > if (!i915.enable_guc_loading) > return 0; > - guc_disable_communication(guc); > + intel_guc_disable_communication(guc); > gen9_reset_guc_interrupts(dev_priv); > /* We need to notify the guc whenever we change the GGTT */ > @@ -347,7 +326,7 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > intel_guc_init_send_regs(guc); > - ret = guc_enable_communication(guc); > + ret = intel_guc_enable_communication(guc); > if (ret) > goto err_log_capture; > @@ -373,7 +352,7 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > * marks the GPU as wedged until reset). > */ > err_interrupts: > - guc_disable_communication(guc); > + intel_guc_disable_communication(guc); > gen9_disable_guc_interrupts(dev_priv); > err_log_capture: > guc_capture_load_err_log(guc); > @@ -410,7 +389,7 @@ void intel_uc_fini_hw(struct drm_i915_private > *dev_priv) > if (i915.enable_guc_submission) > i915_guc_submission_disable(dev_priv); > - guc_disable_communication(&dev_priv->guc); > + intel_guc_disable_communication(&dev_priv->guc); > if (i915.enable_guc_submission) { > gen9_disable_guc_interrupts(dev_priv);
On 9/14/2017 9:01 PM, Michal Wajdeczko wrote: > On Thu, 14 Sep 2017 11:55:04 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> From: "Kamble, Sagar A" <sagar.a.kamble@intel.com> >> >> This patch is moving guc_enable_communication and >> guc_disable_communication to intel_guc.c and making it available for >> use through intel_guc.h. >> Intent is to reuse this function for calling from intel_uc_init_hw >> and also as part of intel_uc_fini_hw where it will be coupled with >> other teardown related to GuC in the upcoming patch. > > Hmm, but both intel_uc_init_hw() and intel_uc_fini_hw() shall stay in > intel_uc.c and the best place for guc_[enable|disable]_communication() > functions is also in intel_uc.c as they provide generic switch between > MMIO/CT send mechanism, so I'm not so sure that we need this patch. > > Michal Hi Michal, Plan was to disable communication while entering rpm/system suspend, reset, unload and enable back when resuming. If we don't want to update these mechanisms across rpm/system suspend or reset will remove this patch then. > >> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc.c | 21 +++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_guc.h | 2 ++ >> drivers/gpu/drm/i915/intel_uc.c | 29 ++++------------------------- >> 3 files changed, 27 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc.c >> b/drivers/gpu/drm/i915/intel_guc.c >> index 5559e00..75bb830 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.c >> +++ b/drivers/gpu/drm/i915/intel_guc.c >> @@ -181,3 +181,24 @@ void intel_guc_auth_huc(struct intel_guc *guc, >> struct intel_huc *huc) >> out: >> i915_vma_unpin(vma); >> } >> + >> +int intel_guc_enable_communication(struct intel_guc *guc) >> +{ >> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >> + >> + if (HAS_GUC_CT(dev_priv)) >> + return intel_guc_enable_ct(guc); >> + >> + guc->send = intel_guc_send_mmio; >> + return 0; >> +} >> + >> +void intel_guc_disable_communication(struct intel_guc *guc) >> +{ >> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >> + >> + if (HAS_GUC_CT(dev_priv)) >> + intel_guc_disable_ct(guc); >> + >> + guc->send = intel_guc_send_nop; >> +} >> diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> index 9a282aa..8ed0e81 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -149,6 +149,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma >> *vma) >> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 >> len); >> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, >> u32 len); >> void intel_guc_auth_huc(struct intel_guc *guc, struct intel_huc *huc); >> +int intel_guc_enable_communication(struct intel_guc *guc); >> +void intel_guc_disable_communication(struct intel_guc *guc); >> /* intel_guc_loader.c */ >> int intel_guc_select_fw(struct intel_guc *guc); >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index a3fc4c8..30c004c 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -265,27 +265,6 @@ static void guc_free_load_err_log(struct >> intel_guc *guc) >> i915_gem_object_put(guc->load_err_log); >> } >> -static int guc_enable_communication(struct intel_guc *guc) >> -{ >> - struct drm_i915_private *dev_priv = guc_to_i915(guc); >> - >> - if (HAS_GUC_CT(dev_priv)) >> - return intel_guc_enable_ct(guc); >> - >> - guc->send = intel_guc_send_mmio; >> - return 0; >> -} >> - >> -static void guc_disable_communication(struct intel_guc *guc) >> -{ >> - struct drm_i915_private *dev_priv = guc_to_i915(guc); >> - >> - if (HAS_GUC_CT(dev_priv)) >> - intel_guc_disable_ct(guc); >> - >> - guc->send = intel_guc_send_nop; >> -} >> - >> int intel_uc_init_hw(struct drm_i915_private *dev_priv) >> { >> struct intel_guc *guc = &dev_priv->guc; >> @@ -295,7 +274,7 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> if (!i915.enable_guc_loading) >> return 0; >> - guc_disable_communication(guc); >> + intel_guc_disable_communication(guc); >> gen9_reset_guc_interrupts(dev_priv); >> /* We need to notify the guc whenever we change the GGTT */ >> @@ -347,7 +326,7 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> intel_guc_init_send_regs(guc); >> - ret = guc_enable_communication(guc); >> + ret = intel_guc_enable_communication(guc); >> if (ret) >> goto err_log_capture; >> @@ -373,7 +352,7 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> * marks the GPU as wedged until reset). >> */ >> err_interrupts: >> - guc_disable_communication(guc); >> + intel_guc_disable_communication(guc); >> gen9_disable_guc_interrupts(dev_priv); >> err_log_capture: >> guc_capture_load_err_log(guc); >> @@ -410,7 +389,7 @@ void intel_uc_fini_hw(struct drm_i915_private >> *dev_priv) >> if (i915.enable_guc_submission) >> i915_guc_submission_disable(dev_priv); >> - guc_disable_communication(&dev_priv->guc); >> + intel_guc_disable_communication(&dev_priv->guc); >> if (i915.enable_guc_submission) { >> gen9_disable_guc_interrupts(dev_priv);
On Thu, 14 Sep 2017 17:56:17 +0200, Kamble, Sagar A <sagar.a.kamble@intel.com> wrote: > > > On 9/14/2017 9:01 PM, Michal Wajdeczko wrote: >> On Thu, 14 Sep 2017 11:55:04 +0200, Sagar Arun Kamble >> <sagar.a.kamble@intel.com> wrote: >> >>> From: "Kamble, Sagar A" <sagar.a.kamble@intel.com> >>> >>> This patch is moving guc_enable_communication and >>> guc_disable_communication to intel_guc.c and making it available for >>> use through intel_guc.h. >>> Intent is to reuse this function for calling from intel_uc_init_hw >>> and also as part of intel_uc_fini_hw where it will be coupled with >>> other teardown related to GuC in the upcoming patch. >> >> Hmm, but both intel_uc_init_hw() and intel_uc_fini_hw() shall stay in >> intel_uc.c and the best place for guc_[enable|disable]_communication() >> functions is also in intel_uc.c as they provide generic switch between >> MMIO/CT send mechanism, so I'm not so sure that we need this patch. >> >> Michal > Hi Michal, > > Plan was to disable communication while entering rpm/system suspend, > reset, unload > and enable back when resuming. If we don't want to update these > mechanisms across rpm/system suspend or reset will > remove this patch then. We should disable any communication with GuC, as Guc will be unavailable, but since suspend/resume/unload code should stay (or be defined) in intel_uc.c file, I see no point of moving related functions out of that file. Michal
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 5559e00..75bb830 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -181,3 +181,24 @@ void intel_guc_auth_huc(struct intel_guc *guc, struct intel_huc *huc) out: i915_vma_unpin(vma); } + +int intel_guc_enable_communication(struct intel_guc *guc) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + + if (HAS_GUC_CT(dev_priv)) + return intel_guc_enable_ct(guc); + + guc->send = intel_guc_send_mmio; + return 0; +} + +void intel_guc_disable_communication(struct intel_guc *guc) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + + if (HAS_GUC_CT(dev_priv)) + intel_guc_disable_ct(guc); + + guc->send = intel_guc_send_nop; +} diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 9a282aa..8ed0e81 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -149,6 +149,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len); void intel_guc_auth_huc(struct intel_guc *guc, struct intel_huc *huc); +int intel_guc_enable_communication(struct intel_guc *guc); +void intel_guc_disable_communication(struct intel_guc *guc); /* intel_guc_loader.c */ int intel_guc_select_fw(struct intel_guc *guc); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index a3fc4c8..30c004c 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -265,27 +265,6 @@ static void guc_free_load_err_log(struct intel_guc *guc) i915_gem_object_put(guc->load_err_log); } -static int guc_enable_communication(struct intel_guc *guc) -{ - struct drm_i915_private *dev_priv = guc_to_i915(guc); - - if (HAS_GUC_CT(dev_priv)) - return intel_guc_enable_ct(guc); - - guc->send = intel_guc_send_mmio; - return 0; -} - -static void guc_disable_communication(struct intel_guc *guc) -{ - struct drm_i915_private *dev_priv = guc_to_i915(guc); - - if (HAS_GUC_CT(dev_priv)) - intel_guc_disable_ct(guc); - - guc->send = intel_guc_send_nop; -} - int intel_uc_init_hw(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; @@ -295,7 +274,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (!i915.enable_guc_loading) return 0; - guc_disable_communication(guc); + intel_guc_disable_communication(guc); gen9_reset_guc_interrupts(dev_priv); /* We need to notify the guc whenever we change the GGTT */ @@ -347,7 +326,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) intel_guc_init_send_regs(guc); - ret = guc_enable_communication(guc); + ret = intel_guc_enable_communication(guc); if (ret) goto err_log_capture; @@ -373,7 +352,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) * marks the GPU as wedged until reset). */ err_interrupts: - guc_disable_communication(guc); + intel_guc_disable_communication(guc); gen9_disable_guc_interrupts(dev_priv); err_log_capture: guc_capture_load_err_log(guc); @@ -410,7 +389,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) if (i915.enable_guc_submission) i915_guc_submission_disable(dev_priv); - guc_disable_communication(&dev_priv->guc); + intel_guc_disable_communication(&dev_priv->guc); if (i915.enable_guc_submission) { gen9_disable_guc_interrupts(dev_priv);