| Message ID | 20170502103243.54940-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 02, 2017 at 10:32:42AM +0000, Michal Wajdeczko wrote: > It is safer to setup valid send function after successful GuC > hardware initialization. In addition we prepare placeholder > where we can setup any alternate GuC communication mechanism. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_uc.h | 1 + > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 900e376..5957a95 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -99,7 +99,7 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) > struct intel_guc *guc = &dev_priv->guc; > > mutex_init(&guc->send_mutex); > - guc->send = intel_guc_send_mmio; > + guc->send = intel_guc_send_nop; > } > > static void fetch_uc_fw(struct drm_i915_private *dev_priv, > @@ -252,13 +252,27 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv) > __intel_uc_fw_fini(&dev_priv->huc.fw); > } > > +static int guc_enable_communication(struct intel_guc *guc) > +{ > + /* XXX: placeholder for alternate setup */ > + guc->send = intel_guc_send_mmio; > + return 0; > +} > + > +static void guc_disable_communication(struct intel_guc *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; > int ret, attempts; > > if (!i915.enable_guc_loading) > return 0; > > + guc_disable_communication(guc); > gen9_reset_guc_interrupts(dev_priv); > > /* We need to notify the guc whenever we change the GGTT */ > @@ -308,6 +322,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > if (ret) > goto err_submission; > > + ret = guc_enable_communication(guc); > + if (ret) > + goto err_submission; > + > intel_guc_auth_huc(dev_priv); > if (i915.enable_guc_submission) { > if (i915.guc_log_level >= 0) > @@ -330,6 +348,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); > gen9_disable_guc_interrupts(dev_priv); > err_submission: > if (i915.enable_guc_submission) > @@ -364,6 +383,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > i915_ggtt_disable_guc(dev_priv); > } > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len) > +{ > + WARN(1, "Unexpected send: action=%#x\n", *action); > + return -ENOSYS; For internal errors we use -ENODEV. I fixed that whilst applying as I'm sure no one else but checkpatch cares for a path that should never occur. Applied and pushed, thanks for the patch and review. -Chris
On Tue, May 02, 2017 at 10:32:42AM +0000, Michal Wajdeczko wrote: > It is safer to setup valid send function after successful GuC > hardware initialization. In addition we prepare placeholder > where we can setup any alternate GuC communication mechanism. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_uc.h | 1 + > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 900e376..5957a95 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -99,7 +99,7 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) > struct intel_guc *guc = &dev_priv->guc; > > mutex_init(&guc->send_mutex); > - guc->send = intel_guc_send_mmio; > + guc->send = intel_guc_send_nop; > } > > static void fetch_uc_fw(struct drm_i915_private *dev_priv, > @@ -252,13 +252,27 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv) > __intel_uc_fw_fini(&dev_priv->huc.fw); > } > > +static int guc_enable_communication(struct intel_guc *guc) > +{ > + /* XXX: placeholder for alternate setup */ > + guc->send = intel_guc_send_mmio; > + return 0; > +} > + > +static void guc_disable_communication(struct intel_guc *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; > int ret, attempts; > > if (!i915.enable_guc_loading) > return 0; > > + guc_disable_communication(guc); > gen9_reset_guc_interrupts(dev_priv); > > /* We need to notify the guc whenever we change the GGTT */ > @@ -308,6 +322,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > if (ret) > goto err_submission; > > + ret = guc_enable_communication(guc); > + if (ret) > + goto err_submission; > + > intel_guc_auth_huc(dev_priv); > if (i915.enable_guc_submission) { > if (i915.guc_log_level >= 0) > @@ -330,6 +348,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); > gen9_disable_guc_interrupts(dev_priv); > err_submission: > if (i915.enable_guc_submission) > @@ -364,6 +383,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > i915_ggtt_disable_guc(dev_priv); > } > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len) > +{ > + WARN(1, "Unexpected send: action=%#x\n", *action); > + return -ENOSYS; > +} > + > /* > * This function implements the MMIO based host to GuC interface. > */ > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index 2f0229d..1e0eecd 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -227,6 +227,7 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > int intel_guc_sample_forcewake(struct intel_guc *guc); > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); Was there a reason for exporting the nop? Its a purely internal BUG_ON() function that should only be set when disabling the guc. Afaict, it should be private. -Chris
On Wed, May 03, 2017 at 11:47:12AM +0100, Chris Wilson wrote: > On Tue, May 02, 2017 at 10:32:42AM +0000, Michal Wajdeczko wrote: > > It is safer to setup valid send function after successful GuC > > hardware initialization. In addition we prepare placeholder > > where we can setup any alternate GuC communication mechanism. > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > --- > > drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_uc.h | 1 + > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > index 900e376..5957a95 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -99,7 +99,7 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) > > struct intel_guc *guc = &dev_priv->guc; > > > > mutex_init(&guc->send_mutex); > > - guc->send = intel_guc_send_mmio; > > + guc->send = intel_guc_send_nop; > > } > > > > static void fetch_uc_fw(struct drm_i915_private *dev_priv, > > @@ -252,13 +252,27 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv) > > __intel_uc_fw_fini(&dev_priv->huc.fw); > > } > > > > +static int guc_enable_communication(struct intel_guc *guc) > > +{ > > + /* XXX: placeholder for alternate setup */ > > + guc->send = intel_guc_send_mmio; > > + return 0; > > +} > > + > > +static void guc_disable_communication(struct intel_guc *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; > > int ret, attempts; > > > > if (!i915.enable_guc_loading) > > return 0; > > > > + guc_disable_communication(guc); > > gen9_reset_guc_interrupts(dev_priv); > > > > /* We need to notify the guc whenever we change the GGTT */ > > @@ -308,6 +322,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > > if (ret) > > goto err_submission; > > > > + ret = guc_enable_communication(guc); > > + if (ret) > > + goto err_submission; > > + > > intel_guc_auth_huc(dev_priv); > > if (i915.enable_guc_submission) { > > if (i915.guc_log_level >= 0) > > @@ -330,6 +348,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); > > gen9_disable_guc_interrupts(dev_priv); > > err_submission: > > if (i915.enable_guc_submission) > > @@ -364,6 +383,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > > i915_ggtt_disable_guc(dev_priv); > > } > > > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len) > > +{ > > + WARN(1, "Unexpected send: action=%#x\n", *action); > > + return -ENOSYS; > > +} > > + > > /* > > * This function implements the MMIO based host to GuC interface. > > */ > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > index 2f0229d..1e0eecd 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -227,6 +227,7 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > > int intel_guc_sample_forcewake(struct intel_guc *guc); > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); > > Was there a reason for exporting the nop? Its a purely internal BUG_ON() > function that should only be set when disabling the guc. Afaict, it > should be private. By looking only at this patch, agree, this nop function should be private. But at the same time, this function, to some extend, is similar to the send_mmio function, that we agree to keep it exposed, as it also may be needed in other code paths (like upcoming alternate GuC communication implementation) in case of the recovery or other plumbing. I should mention that in the commit message to avoid your concern ;) -Michal
On Wed, May 03, 2017 at 05:54:43PM +0200, Michal Wajdeczko wrote: > On Wed, May 03, 2017 at 11:47:12AM +0100, Chris Wilson wrote: > > On Tue, May 02, 2017 at 10:32:42AM +0000, Michal Wajdeczko wrote: > > > It is safer to setup valid send function after successful GuC > > > hardware initialization. In addition we prepare placeholder > > > where we can setup any alternate GuC communication mechanism. > > > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++++++++++++++++++++- > > > drivers/gpu/drm/i915/intel_uc.h | 1 + > > > 2 files changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > > index 900e376..5957a95 100644 > > > --- a/drivers/gpu/drm/i915/intel_uc.c > > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > > @@ -99,7 +99,7 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) > > > struct intel_guc *guc = &dev_priv->guc; > > > > > > mutex_init(&guc->send_mutex); > > > - guc->send = intel_guc_send_mmio; > > > + guc->send = intel_guc_send_nop; > > > } > > > > > > static void fetch_uc_fw(struct drm_i915_private *dev_priv, > > > @@ -252,13 +252,27 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv) > > > __intel_uc_fw_fini(&dev_priv->huc.fw); > > > } > > > > > > +static int guc_enable_communication(struct intel_guc *guc) > > > +{ > > > + /* XXX: placeholder for alternate setup */ > > > + guc->send = intel_guc_send_mmio; > > > + return 0; > > > +} > > > + > > > +static void guc_disable_communication(struct intel_guc *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; > > > int ret, attempts; > > > > > > if (!i915.enable_guc_loading) > > > return 0; > > > > > > + guc_disable_communication(guc); > > > gen9_reset_guc_interrupts(dev_priv); > > > > > > /* We need to notify the guc whenever we change the GGTT */ > > > @@ -308,6 +322,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > > > if (ret) > > > goto err_submission; > > > > > > + ret = guc_enable_communication(guc); > > > + if (ret) > > > + goto err_submission; > > > + > > > intel_guc_auth_huc(dev_priv); > > > if (i915.enable_guc_submission) { > > > if (i915.guc_log_level >= 0) > > > @@ -330,6 +348,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); > > > gen9_disable_guc_interrupts(dev_priv); > > > err_submission: > > > if (i915.enable_guc_submission) > > > @@ -364,6 +383,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > > > i915_ggtt_disable_guc(dev_priv); > > > } > > > > > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len) > > > +{ > > > + WARN(1, "Unexpected send: action=%#x\n", *action); > > > + return -ENOSYS; > > > +} > > > + > > > /* > > > * This function implements the MMIO based host to GuC interface. > > > */ > > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > > index 2f0229d..1e0eecd 100644 > > > --- a/drivers/gpu/drm/i915/intel_uc.h > > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > > @@ -227,6 +227,7 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > > > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > > > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > > > int intel_guc_sample_forcewake(struct intel_guc *guc); > > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); > > > > Was there a reason for exporting the nop? Its a purely internal BUG_ON() > > function that should only be set when disabling the guc. Afaict, it > > should be private. > > By looking only at this patch, agree, this nop function should be private. > > But at the same time, this function, to some extend, is similar to the send_mmio > function, that we agree to keep it exposed, as it also may be needed in other > code paths (like upcoming alternate GuC communication implementation) in case > of the recovery or other plumbing. > > I should mention that in the commit message to avoid your concern ;) Always a benefit to mention the grand design if things look out of place in individual patches. I'll be surprised if you can conjure up a situation where we want to call intel_guc_send_nop() :) -Chris
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 900e376..5957a95 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -99,7 +99,7 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) struct intel_guc *guc = &dev_priv->guc; mutex_init(&guc->send_mutex); - guc->send = intel_guc_send_mmio; + guc->send = intel_guc_send_nop; } static void fetch_uc_fw(struct drm_i915_private *dev_priv, @@ -252,13 +252,27 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv) __intel_uc_fw_fini(&dev_priv->huc.fw); } +static int guc_enable_communication(struct intel_guc *guc) +{ + /* XXX: placeholder for alternate setup */ + guc->send = intel_guc_send_mmio; + return 0; +} + +static void guc_disable_communication(struct intel_guc *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; int ret, attempts; if (!i915.enable_guc_loading) return 0; + guc_disable_communication(guc); gen9_reset_guc_interrupts(dev_priv); /* We need to notify the guc whenever we change the GGTT */ @@ -308,6 +322,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (ret) goto err_submission; + ret = guc_enable_communication(guc); + if (ret) + goto err_submission; + intel_guc_auth_huc(dev_priv); if (i915.enable_guc_submission) { if (i915.guc_log_level >= 0) @@ -330,6 +348,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); gen9_disable_guc_interrupts(dev_priv); err_submission: if (i915.enable_guc_submission) @@ -364,6 +383,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) i915_ggtt_disable_guc(dev_priv); } +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len) +{ + WARN(1, "Unexpected send: action=%#x\n", *action); + return -ENOSYS; +} + /* * This function implements the MMIO based host to GuC interface. */ diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 2f0229d..1e0eecd 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -227,6 +227,7 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv); int intel_uc_init_hw(struct drm_i915_private *dev_priv); void intel_uc_fini_hw(struct drm_i915_private *dev_priv); int intel_guc_sample_forcewake(struct intel_guc *guc); +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); static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) {