| Message ID | 1510358798-21566-9-git-send-email-sujaritha.sundaresan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 11 Nov 2017 01:06:38 +0100, Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> wrote: > Placing the call to intel_guc_init after i915_gem_contexts_init, > based on the dependency within i915_gem_init. > > Will move the function if required, depending on the review > comments. > > Suggested by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 889ae88..c877a5d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -36,6 +36,7 @@ > #include "intel_frontbuffer.h" > #include "intel_mocs.h" > #include "i915_gemfs.h" > +#include "intel_guc.h" > #include <linux/dma-fence-array.h> > #include <linux/kthread.h> > #include <linux/reservation.h> > @@ -4972,6 +4973,7 @@ bool intel_sanitize_semaphores(struct > drm_i915_private *dev_priv, int value) > int i915_gem_init(struct drm_i915_private *dev_priv) > { > + struct intel_guc *guc = &dev_priv->guc; > int ret; > mutex_lock(&dev_priv->drm.struct_mutex); > @@ -5015,6 +5017,18 @@ int i915_gem_init(struct drm_i915_private > *dev_priv) > if (ret) > goto out_unlock; > + ret = intel_guc_init(guc); > + > + if (i915_modparams.enable_guc) { > + /* > + * This is stuff we need to have available at fw load time > + * if we are planning to enable submission later > + */ > + ret = intel_guc_init(guc); > + if (ret) > + goto err_shared; > + } > + Why there are two calls to guc_init ? Also note that this approach breaks previous idea to hide GuC/HuC internals to the rest of the driver by exposing only high level "uc" functions. > ret = intel_engines_init(dev_priv); > if (ret) > goto out_unlock; > @@ -5035,7 +5049,8 @@ int i915_gem_init(struct drm_i915_private > *dev_priv) > out_unlock: > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > mutex_unlock(&dev_priv->drm.struct_mutex); > - > +err_shared: > + intel_guc_fini(guc); > return ret; > } > @@ -5192,6 +5207,7 @@ void i915_gem_load_cleanup(struct drm_i915_private > *dev_priv) > rcu_barrier(); > i915_gemfs_fini(dev_priv); > + intel_guc_fini(&dev_priv->guc); > } > int i915_gem_freeze(struct drm_i915_private *dev_priv)
On 11/12/2017 09:22 AM, Michal Wajdeczko wrote: > On Sat, 11 Nov 2017 01:06:38 +0100, Sujaritha Sundaresan > <sujaritha.sundaresan@intel.com> wrote: > >> Placing the call to intel_guc_init after i915_gem_contexts_init, >> based on the dependency within i915_gem_init. >> >> Will move the function if required, depending on the review >> comments. >> >> Suggested by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index 889ae88..c877a5d 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -36,6 +36,7 @@ >> #include "intel_frontbuffer.h" >> #include "intel_mocs.h" >> #include "i915_gemfs.h" >> +#include "intel_guc.h" >> #include <linux/dma-fence-array.h> >> #include <linux/kthread.h> >> #include <linux/reservation.h> >> @@ -4972,6 +4973,7 @@ bool intel_sanitize_semaphores(struct >> drm_i915_private *dev_priv, int value) >> int i915_gem_init(struct drm_i915_private *dev_priv) >> { >> + struct intel_guc *guc = &dev_priv->guc; >> int ret; >> mutex_lock(&dev_priv->drm.struct_mutex); >> @@ -5015,6 +5017,18 @@ int i915_gem_init(struct drm_i915_private >> *dev_priv) >> if (ret) >> goto out_unlock; >> + ret = intel_guc_init(guc); >> + >> + if (i915_modparams.enable_guc) { >> + /* >> + * This is stuff we need to have available at fw load time >> + * if we are planning to enable submission later >> + */ >> + ret = intel_guc_init(guc); >> + if (ret) >> + goto err_shared; >> + } >> + > > Why there are two calls to guc_init ? > > Also note that this approach breaks previous idea to hide GuC/HuC > internals > to the rest of the driver by exposing only high level "uc" functions. > I will call intel_guc_init (dev_priv) inside intel_uc_init(dev_priv). I will send the corrected patch asap. I'm also considering splitting the series. I will split the module parameter changes and the decoupling changes into two series. >> ret = intel_engines_init(dev_priv); >> if (ret) >> goto out_unlock; >> @@ -5035,7 +5049,8 @@ int i915_gem_init(struct drm_i915_private >> *dev_priv) >> out_unlock: >> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); >> mutex_unlock(&dev_priv->drm.struct_mutex); >> - >> +err_shared: >> + intel_guc_fini(guc); >> return ret; >> } >> @@ -5192,6 +5207,7 @@ void i915_gem_load_cleanup(struct >> drm_i915_private *dev_priv) >> rcu_barrier(); >> i915_gemfs_fini(dev_priv); >> + intel_guc_fini(&dev_priv->guc); >> } >> int i915_gem_freeze(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 889ae88..c877a5d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -36,6 +36,7 @@ #include "intel_frontbuffer.h" #include "intel_mocs.h" #include "i915_gemfs.h" +#include "intel_guc.h" #include <linux/dma-fence-array.h> #include <linux/kthread.h> #include <linux/reservation.h> @@ -4972,6 +4973,7 @@ bool intel_sanitize_semaphores(struct drm_i915_private *dev_priv, int value) int i915_gem_init(struct drm_i915_private *dev_priv) { + struct intel_guc *guc = &dev_priv->guc; int ret; mutex_lock(&dev_priv->drm.struct_mutex); @@ -5015,6 +5017,18 @@ int i915_gem_init(struct drm_i915_private *dev_priv) if (ret) goto out_unlock; + ret = intel_guc_init(guc); + + if (i915_modparams.enable_guc) { + /* + * This is stuff we need to have available at fw load time + * if we are planning to enable submission later + */ + ret = intel_guc_init(guc); + if (ret) + goto err_shared; + } + ret = intel_engines_init(dev_priv); if (ret) goto out_unlock; @@ -5035,7 +5049,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv) out_unlock: intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); mutex_unlock(&dev_priv->drm.struct_mutex); - +err_shared: + intel_guc_fini(guc); return ret; } @@ -5192,6 +5207,7 @@ void i915_gem_load_cleanup(struct drm_i915_private *dev_priv) rcu_barrier(); i915_gemfs_fini(dev_priv); + intel_guc_fini(&dev_priv->guc); } int i915_gem_freeze(struct drm_i915_private *dev_priv)
Placing the call to intel_guc_init after i915_gem_contexts_init, based on the dependency within i915_gem_init. Will move the function if required, depending on the review comments. Suggested by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)