| Message ID | 1508309222-26406-6-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/10/2017 07:46, Sagar Arun Kamble wrote: > With guc_log_level parameter sanitized we can remove the GuC submission > checks from flush_guc_logs and i915_guc_log_register/unregister and > intel_uc_fini_hw. It is important to note that GuC log runtime/relay > channel data has to be freed during driver unregister. > > Freeing of that data can't be gated by guc_log_level check because If we > free GuC log runtime only when log level >=0 then it will not be destroyed > when logging is disabled after enabling before driver unload and driver > will try to destroy during cleanup/fini_hw where relay will be NULL > already. > > With this patch GuC interrupts are enabled first after GuC load if logging > is enabled. Earlier they were enabled only when submission was getting > enabled. > > GuC to Host interrupts will be needed for GuC CT buffer recv mechanism and > hence we will be adding support to control that interrupt based on ref. > taken by Log or CT recv feature. To prepare for that all interrupt updates > are now gated by GuC log level checks. > > v2: Rebase. Updated check in i915_guc_log_unregister to be based on > guc_log_level. (Michal Wajdeczko) > > v3: Updated guc_log_unregister again. Made all GuC log related functions > depend only guc_log_level. Updated uC init w.r.t enabling of GuC > interrupts. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_guc.c | 3 ++- > drivers/gpu/drm/i915/intel_guc_log.c | 12 ++++-------- > drivers/gpu/drm/i915/intel_uc.c | 21 ++++++++++++--------- > 3 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c > index 31f25e5..959057a 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -274,7 +274,8 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) > if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > return 0; > > - intel_disable_guc_interrupts(guc); > + if (i915_modparams.guc_log_level >= 0) > + intel_disable_guc_interrupts(guc); I don't like this as much since we have a general direction of having less usage of modparams sprinkled around. Ideally we would take a snapshot after sanitizing so it remains immutable for the lifetime of the driver. For this particular case it would be nice I think if in the guc object you would store something like bool guc->interrupts_needed. Whether or not then to act on that from inside intel_(en|dis)ble_guc_interrupts would be open for discussion. Your thoughts? > > ctx = dev_priv->kernel_context; > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index 200f0a1..f87e9f5 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc *guc) > > static void guc_flush_logs(struct intel_guc *guc) > { > - if (!i915_modparams.enable_guc_submission || > - (i915_modparams.guc_log_level < 0)) > + if (i915_modparams.guc_log_level < 0) This would then become guc->log_level. > return; > > /* First disable the interrupts, will be renabled afterwards */ > @@ -645,8 +644,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) > > void i915_guc_log_register(struct drm_i915_private *dev_priv) > { > - if (!i915_modparams.enable_guc_submission || > - (i915_modparams.guc_log_level < 0)) > + if (i915_modparams.guc_log_level < 0) > return; > > mutex_lock(&dev_priv->drm.struct_mutex); > @@ -656,12 +654,10 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv) > > void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > { > - if (!i915_modparams.enable_guc_submission) > - return; > - > mutex_lock(&dev_priv->drm.struct_mutex); > /* GuC logging is currently the only user of Guc2Host interrupts */ > - intel_disable_guc_interrupts(&dev_priv->guc); > + if (i915_modparams.guc_log_level >= 0) > + intel_disable_guc_interrupts(&dev_priv->guc); > guc_log_runtime_destroy(&dev_priv->guc); > mutex_unlock(&dev_priv->drm.struct_mutex); > } > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 8feefcd..95c5ec4 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > if (ret) > goto err_log_capture; > > + if (i915_modparams.guc_log_level >= 0) > + intel_enable_guc_interrupts(guc); > + > ret = guc_enable_communication(guc); > if (ret) > - goto err_log_capture; > + goto err_interrupts; > > intel_huc_auth(&dev_priv->huc); > if (i915_modparams.enable_guc_submission) { > - if (i915_modparams.guc_log_level >= 0) > - intel_enable_guc_interrupts(guc); > - > ret = i915_guc_submission_enable(dev_priv); > if (ret) > - goto err_interrupts; > + goto err_comm; > } > > dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n", > @@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > * nonfatal error (i.e. it doesn't prevent driver load, but > * marks the GPU as wedged until reset). > */ > -err_interrupts: > +err_comm: > guc_disable_communication(guc); > - intel_disable_guc_interrupts(guc); > +err_interrupts: > + if (i915_modparams.guc_log_level >= 0) > + intel_disable_guc_interrupts(guc); > err_log_capture: > guc_capture_load_err_log(guc); > err_submission: > @@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > > guc_disable_communication(&dev_priv->guc); > > - if (i915_modparams.enable_guc_submission) { > + if (i915_modparams.guc_log_level >= 0) > intel_disable_guc_interrupts(&dev_priv->guc); > + > + if (i915_modparams.enable_guc_submission) > i915_guc_submission_fini(dev_priv); > - } > > i915_ggtt_disable_guc(dev_priv); > } > Open for discussion I think, but it feels even visually like to much modparams. Regards, Tvrtko
On 10/18/2017 6:37 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 07:46, Sagar Arun Kamble wrote: >> With guc_log_level parameter sanitized we can remove the GuC submission >> checks from flush_guc_logs and i915_guc_log_register/unregister and >> intel_uc_fini_hw. It is important to note that GuC log runtime/relay >> channel data has to be freed during driver unregister. >> >> Freeing of that data can't be gated by guc_log_level check because If we >> free GuC log runtime only when log level >=0 then it will not be >> destroyed >> when logging is disabled after enabling before driver unload and driver >> will try to destroy during cleanup/fini_hw where relay will be NULL >> already. >> >> With this patch GuC interrupts are enabled first after GuC load if >> logging >> is enabled. Earlier they were enabled only when submission was getting >> enabled. >> >> GuC to Host interrupts will be needed for GuC CT buffer recv >> mechanism and >> hence we will be adding support to control that interrupt based on ref. >> taken by Log or CT recv feature. To prepare for that all interrupt >> updates >> are now gated by GuC log level checks. >> >> v2: Rebase. Updated check in i915_guc_log_unregister to be based on >> guc_log_level. (Michal Wajdeczko) >> >> v3: Updated guc_log_unregister again. Made all GuC log related functions >> depend only guc_log_level. Updated uC init w.r.t enabling of GuC >> interrupts. >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc.c | 3 ++- >> drivers/gpu/drm/i915/intel_guc_log.c | 12 ++++-------- >> drivers/gpu/drm/i915/intel_uc.c | 21 ++++++++++++--------- >> 3 files changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc.c >> b/drivers/gpu/drm/i915/intel_guc.c >> index 31f25e5..959057a 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.c >> +++ b/drivers/gpu/drm/i915/intel_guc.c >> @@ -274,7 +274,8 @@ int intel_guc_suspend(struct drm_i915_private >> *dev_priv) >> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) >> return 0; >> - intel_disable_guc_interrupts(guc); >> + if (i915_modparams.guc_log_level >= 0) >> + intel_disable_guc_interrupts(guc); > > I don't like this as much since we have a general direction of having > less usage of modparams sprinkled around. Ideally we would take a > snapshot after sanitizing so it remains immutable for the lifetime of > the driver. Yes. Will update. > > For this particular case it would be nice I think if in the guc object > you would store something like bool guc->interrupts_needed. > > Whether or not then to act on that from inside > intel_(en|dis)ble_guc_interrupts would be open for discussion. ok > > Your thoughts? > >> ctx = dev_priv->kernel_context; >> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c >> b/drivers/gpu/drm/i915/intel_guc_log.c >> index 200f0a1..f87e9f5 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc >> *guc) >> static void guc_flush_logs(struct intel_guc *guc) >> { >> - if (!i915_modparams.enable_guc_submission || >> - (i915_modparams.guc_log_level < 0)) >> + if (i915_modparams.guc_log_level < 0) > > This would then become guc->log_level. yes > >> return; >> /* First disable the interrupts, will be renabled afterwards */ >> @@ -645,8 +644,7 @@ int i915_guc_log_control(struct drm_i915_private >> *dev_priv, u64 control_val) >> void i915_guc_log_register(struct drm_i915_private *dev_priv) >> { >> - if (!i915_modparams.enable_guc_submission || >> - (i915_modparams.guc_log_level < 0)) >> + if (i915_modparams.guc_log_level < 0) >> return; >> mutex_lock(&dev_priv->drm.struct_mutex); >> @@ -656,12 +654,10 @@ void i915_guc_log_register(struct >> drm_i915_private *dev_priv) >> void i915_guc_log_unregister(struct drm_i915_private *dev_priv) >> { >> - if (!i915_modparams.enable_guc_submission) >> - return; >> - >> mutex_lock(&dev_priv->drm.struct_mutex); >> /* GuC logging is currently the only user of Guc2Host >> interrupts */ >> - intel_disable_guc_interrupts(&dev_priv->guc); >> + if (i915_modparams.guc_log_level >= 0) >> + intel_disable_guc_interrupts(&dev_priv->guc); >> guc_log_runtime_destroy(&dev_priv->guc); >> mutex_unlock(&dev_priv->drm.struct_mutex); >> } >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 8feefcd..95c5ec4 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> if (ret) >> goto err_log_capture; >> + if (i915_modparams.guc_log_level >= 0) >> + intel_enable_guc_interrupts(guc); >> + >> ret = guc_enable_communication(guc); >> if (ret) >> - goto err_log_capture; >> + goto err_interrupts; >> intel_huc_auth(&dev_priv->huc); >> if (i915_modparams.enable_guc_submission) { >> - if (i915_modparams.guc_log_level >= 0) >> - intel_enable_guc_interrupts(guc); >> - >> ret = i915_guc_submission_enable(dev_priv); >> if (ret) >> - goto err_interrupts; >> + goto err_comm; >> } >> dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version >> %u.%u])\n", >> @@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> * nonfatal error (i.e. it doesn't prevent driver load, but >> * marks the GPU as wedged until reset). >> */ >> -err_interrupts: >> +err_comm: >> guc_disable_communication(guc); >> - intel_disable_guc_interrupts(guc); >> +err_interrupts: >> + if (i915_modparams.guc_log_level >= 0) >> + intel_disable_guc_interrupts(guc); >> err_log_capture: >> guc_capture_load_err_log(guc); >> err_submission: >> @@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private >> *dev_priv) >> guc_disable_communication(&dev_priv->guc); >> - if (i915_modparams.enable_guc_submission) { >> + if (i915_modparams.guc_log_level >= 0) >> intel_disable_guc_interrupts(&dev_priv->guc); >> + >> + if (i915_modparams.enable_guc_submission) >> i915_guc_submission_fini(dev_priv); >> - } >> i915_ggtt_disable_guc(dev_priv); >> } >> > > Open for discussion I think, but it feels even visually like to much > modparams. yes. will update. Thanks. > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 31f25e5..959057a 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -274,7 +274,8 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) return 0; - intel_disable_guc_interrupts(guc); + if (i915_modparams.guc_log_level >= 0) + intel_disable_guc_interrupts(guc); ctx = dev_priv->kernel_context; diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 200f0a1..f87e9f5 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc *guc) static void guc_flush_logs(struct intel_guc *guc) { - if (!i915_modparams.enable_guc_submission || - (i915_modparams.guc_log_level < 0)) + if (i915_modparams.guc_log_level < 0) return; /* First disable the interrupts, will be renabled afterwards */ @@ -645,8 +644,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) void i915_guc_log_register(struct drm_i915_private *dev_priv) { - if (!i915_modparams.enable_guc_submission || - (i915_modparams.guc_log_level < 0)) + if (i915_modparams.guc_log_level < 0) return; mutex_lock(&dev_priv->drm.struct_mutex); @@ -656,12 +654,10 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv) void i915_guc_log_unregister(struct drm_i915_private *dev_priv) { - if (!i915_modparams.enable_guc_submission) - return; - mutex_lock(&dev_priv->drm.struct_mutex); /* GuC logging is currently the only user of Guc2Host interrupts */ - intel_disable_guc_interrupts(&dev_priv->guc); + if (i915_modparams.guc_log_level >= 0) + intel_disable_guc_interrupts(&dev_priv->guc); guc_log_runtime_destroy(&dev_priv->guc); mutex_unlock(&dev_priv->drm.struct_mutex); } diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 8feefcd..95c5ec4 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (ret) goto err_log_capture; + if (i915_modparams.guc_log_level >= 0) + intel_enable_guc_interrupts(guc); + ret = guc_enable_communication(guc); if (ret) - goto err_log_capture; + goto err_interrupts; intel_huc_auth(&dev_priv->huc); if (i915_modparams.enable_guc_submission) { - if (i915_modparams.guc_log_level >= 0) - intel_enable_guc_interrupts(guc); - ret = i915_guc_submission_enable(dev_priv); if (ret) - goto err_interrupts; + goto err_comm; } dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n", @@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) * nonfatal error (i.e. it doesn't prevent driver load, but * marks the GPU as wedged until reset). */ -err_interrupts: +err_comm: guc_disable_communication(guc); - intel_disable_guc_interrupts(guc); +err_interrupts: + if (i915_modparams.guc_log_level >= 0) + intel_disable_guc_interrupts(guc); err_log_capture: guc_capture_load_err_log(guc); err_submission: @@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) guc_disable_communication(&dev_priv->guc); - if (i915_modparams.enable_guc_submission) { + if (i915_modparams.guc_log_level >= 0) intel_disable_guc_interrupts(&dev_priv->guc); + + if (i915_modparams.enable_guc_submission) i915_guc_submission_fini(dev_priv); - } i915_ggtt_disable_guc(dev_priv); }
With guc_log_level parameter sanitized we can remove the GuC submission checks from flush_guc_logs and i915_guc_log_register/unregister and intel_uc_fini_hw. It is important to note that GuC log runtime/relay channel data has to be freed during driver unregister. Freeing of that data can't be gated by guc_log_level check because If we free GuC log runtime only when log level >=0 then it will not be destroyed when logging is disabled after enabling before driver unload and driver will try to destroy during cleanup/fini_hw where relay will be NULL already. With this patch GuC interrupts are enabled first after GuC load if logging is enabled. Earlier they were enabled only when submission was getting enabled. GuC to Host interrupts will be needed for GuC CT buffer recv mechanism and hence we will be adding support to control that interrupt based on ref. taken by Log or CT recv feature. To prepare for that all interrupt updates are now gated by GuC log level checks. v2: Rebase. Updated check in i915_guc_log_unregister to be based on guc_log_level. (Michal Wajdeczko) v3: Updated guc_log_unregister again. Made all GuC log related functions depend only guc_log_level. Updated uC init w.r.t enabling of GuC interrupts. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_guc.c | 3 ++- drivers/gpu/drm/i915/intel_guc_log.c | 12 ++++-------- drivers/gpu/drm/i915/intel_uc.c | 21 ++++++++++++--------- 3 files changed, 18 insertions(+), 18 deletions(-)