| Message ID | 20200110162930.88968-2-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ops to intel_uc | expand |
Quoting Michal Wajdeczko (2020-01-10 16:29:27) > Instead of spreading multiple conditionals across the uC code > to find out current mode of uC operation, start using predefined > set of function pointers that reflect that mode. > > Begin with pair of init_hw/fini_hw functions that are responsible > for uC hardware initialization and cleanup. > > v2: drop ops_none, use macro to generate ops helpers > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 45 ++++++++++++++++++++++----- > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 21 +++++++++++-- > 2 files changed, 57 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 3ffc6267f96e..da401e97bba3 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -12,6 +12,19 @@ > > #include "i915_drv.h" > > +static int __uc_check_hw(struct intel_uc *uc); > +static int __uc_init_hw(struct intel_uc *uc); > +static void __uc_fini_hw(struct intel_uc *uc); > + > +static const struct intel_uc_ops uc_ops_off = { > + .init_hw = __uc_check_hw, > +}; > + > +static const struct intel_uc_ops uc_ops_on = { > + .init_hw = __uc_init_hw, > + .fini_hw = __uc_fini_hw, > +}; > + > /* Reset GuC providing us with fresh state for both GuC and HuC. > */ > static int __intel_uc_reset_hw(struct intel_uc *uc) > @@ -89,6 +102,11 @@ void intel_uc_init_early(struct intel_uc *uc) > intel_huc_init_early(&uc->huc); > > __confirm_options(uc); > + > + if (intel_uc_uses_guc(uc)) > + uc->ops = &uc_ops_on; > + else > + uc->ops = &uc_ops_off; > } > > void intel_uc_driver_late_release(struct intel_uc *uc) > @@ -380,24 +398,37 @@ static bool uc_is_wopcm_locked(struct intel_uc *uc) > (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & GUC_WOPCM_OFFSET_VALID); > } > > -int intel_uc_init_hw(struct intel_uc *uc) > +static int __uc_check_hw(struct intel_uc *uc) > +{ > + if (!intel_uc_supports_guc(uc)) > + return 0; > + > + /* > + * We can silently continue without GuC only if it was never enabled > + * before on this system after reboot, otherwise we risk GPU hangs. > + * To check if GuC was loaded before we look at WOPCM registers. > + */ > + if (uc_is_wopcm_locked(uc)) > + return -EIO; > + > + return 0; > +} > + > +static int __uc_init_hw(struct intel_uc *uc) > { > struct drm_i915_private *i915 = uc_to_gt(uc)->i915; > struct intel_guc *guc = &uc->guc; > struct intel_huc *huc = &uc->huc; > int ret, attempts; > > - if (!intel_uc_supports_guc(uc)) > - return 0; > + GEM_BUG_ON(!intel_uc_supports_guc(uc)); > + GEM_BUG_ON(!intel_uc_uses_guc(uc)); > > /* > * We can silently continue without GuC only if it was never enabled > * before on this system after reboot, otherwise we risk GPU hangs. > * To check if GuC was loaded before we look at WOPCM registers. > */ This comment still required? Shouldn't there be a call here to __uc_check_hw() instead? > - if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc)) > - return 0; > - > if (!intel_uc_fw_is_available(&guc->fw)) { > ret = uc_is_wopcm_locked(uc) || > intel_uc_fw_is_overridden(&guc->fw) || > @@ -495,7 +526,7 @@ int intel_uc_init_hw(struct intel_uc *uc) > return -EIO; > }
On Fri, 10 Jan 2020 17:57:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2020-01-10 16:29:27) >> Instead of spreading multiple conditionals across the uC code >> to find out current mode of uC operation, start using predefined >> set of function pointers that reflect that mode. >> >> Begin with pair of init_hw/fini_hw functions that are responsible >> for uC hardware initialization and cleanup. >> >> v2: drop ops_none, use macro to generate ops helpers >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 45 ++++++++++++++++++++++----- >> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 21 +++++++++++-- >> 2 files changed, 57 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index 3ffc6267f96e..da401e97bba3 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -12,6 +12,19 @@ >> >> #include "i915_drv.h" >> >> +static int __uc_check_hw(struct intel_uc *uc); >> +static int __uc_init_hw(struct intel_uc *uc); >> +static void __uc_fini_hw(struct intel_uc *uc); >> + >> +static const struct intel_uc_ops uc_ops_off = { >> + .init_hw = __uc_check_hw, >> +}; >> + >> +static const struct intel_uc_ops uc_ops_on = { >> + .init_hw = __uc_init_hw, >> + .fini_hw = __uc_fini_hw, >> +}; >> + >> /* Reset GuC providing us with fresh state for both GuC and HuC. >> */ >> static int __intel_uc_reset_hw(struct intel_uc *uc) >> @@ -89,6 +102,11 @@ void intel_uc_init_early(struct intel_uc *uc) >> intel_huc_init_early(&uc->huc); >> >> __confirm_options(uc); >> + >> + if (intel_uc_uses_guc(uc)) >> + uc->ops = &uc_ops_on; >> + else >> + uc->ops = &uc_ops_off; >> } >> >> void intel_uc_driver_late_release(struct intel_uc *uc) >> @@ -380,24 +398,37 @@ static bool uc_is_wopcm_locked(struct intel_uc >> *uc) >> (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & >> GUC_WOPCM_OFFSET_VALID); >> } >> >> -int intel_uc_init_hw(struct intel_uc *uc) >> +static int __uc_check_hw(struct intel_uc *uc) >> +{ >> + if (!intel_uc_supports_guc(uc)) >> + return 0; >> + >> + /* >> + * We can silently continue without GuC only if it was never >> enabled >> + * before on this system after reboot, otherwise we risk GPU >> hangs. >> + * To check if GuC was loaded before we look at WOPCM registers. >> + */ >> + if (uc_is_wopcm_locked(uc)) >> + return -EIO; >> + >> + return 0; >> +} >> + >> +static int __uc_init_hw(struct intel_uc *uc) >> { >> struct drm_i915_private *i915 = uc_to_gt(uc)->i915; >> struct intel_guc *guc = &uc->guc; >> struct intel_huc *huc = &uc->huc; >> int ret, attempts; >> >> - if (!intel_uc_supports_guc(uc)) >> - return 0; >> + GEM_BUG_ON(!intel_uc_supports_guc(uc)); >> + GEM_BUG_ON(!intel_uc_uses_guc(uc)); >> >> /* >> * We can silently continue without GuC only if it was never >> enabled >> * before on this system after reboot, otherwise we risk GPU >> hangs. >> * To check if GuC was loaded before we look at WOPCM registers. >> */ > > This comment still required? > Shouldn't there be a call here to __uc_check_hw() instead? ok, will replace below call to uc_is_wopcm_locked() with __uc_check_hw() to avoid redundant (but otherwise still valid) comment > >> - if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc)) >> - return 0; >> - >> if (!intel_uc_fw_is_available(&guc->fw)) { >> ret = uc_is_wopcm_locked(uc) || ^^^^^^^^^^^^^^^^^^ >> intel_uc_fw_is_overridden(&guc->fw) || >> @@ -495,7 +526,7 @@ int intel_uc_init_hw(struct intel_uc *uc) >> return -EIO; >> }
Quoting Michal Wajdeczko (2020-01-10 18:47:50) > On Fri, 10 Jan 2020 17:57:22 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2020-01-10 16:29:27) > >> Instead of spreading multiple conditionals across the uC code > >> to find out current mode of uC operation, start using predefined > >> set of function pointers that reflect that mode. > >> > >> Begin with pair of init_hw/fini_hw functions that are responsible > >> for uC hardware initialization and cleanup. > >> > >> v2: drop ops_none, use macro to generate ops helpers > >> > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 45 ++++++++++++++++++++++----- > >> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 21 +++++++++++-- > >> 2 files changed, 57 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> index 3ffc6267f96e..da401e97bba3 100644 > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> @@ -12,6 +12,19 @@ > >> > >> #include "i915_drv.h" > >> > >> +static int __uc_check_hw(struct intel_uc *uc); > >> +static int __uc_init_hw(struct intel_uc *uc); > >> +static void __uc_fini_hw(struct intel_uc *uc); > >> + > >> +static const struct intel_uc_ops uc_ops_off = { > >> + .init_hw = __uc_check_hw, > >> +}; > >> + > >> +static const struct intel_uc_ops uc_ops_on = { > >> + .init_hw = __uc_init_hw, > >> + .fini_hw = __uc_fini_hw, > >> +}; > >> + > >> /* Reset GuC providing us with fresh state for both GuC and HuC. > >> */ > >> static int __intel_uc_reset_hw(struct intel_uc *uc) > >> @@ -89,6 +102,11 @@ void intel_uc_init_early(struct intel_uc *uc) > >> intel_huc_init_early(&uc->huc); > >> > >> __confirm_options(uc); > >> + > >> + if (intel_uc_uses_guc(uc)) > >> + uc->ops = &uc_ops_on; > >> + else > >> + uc->ops = &uc_ops_off; > >> } > >> > >> void intel_uc_driver_late_release(struct intel_uc *uc) > >> @@ -380,24 +398,37 @@ static bool uc_is_wopcm_locked(struct intel_uc > >> *uc) > >> (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & > >> GUC_WOPCM_OFFSET_VALID); > >> } > >> > >> -int intel_uc_init_hw(struct intel_uc *uc) > >> +static int __uc_check_hw(struct intel_uc *uc) > >> +{ > >> + if (!intel_uc_supports_guc(uc)) > >> + return 0; > >> + > >> + /* > >> + * We can silently continue without GuC only if it was never > >> enabled > >> + * before on this system after reboot, otherwise we risk GPU > >> hangs. > >> + * To check if GuC was loaded before we look at WOPCM registers. > >> + */ > >> + if (uc_is_wopcm_locked(uc)) > >> + return -EIO; > >> + > >> + return 0; > >> +} > >> + > >> +static int __uc_init_hw(struct intel_uc *uc) > >> { > >> struct drm_i915_private *i915 = uc_to_gt(uc)->i915; > >> struct intel_guc *guc = &uc->guc; > >> struct intel_huc *huc = &uc->huc; > >> int ret, attempts; > >> > >> - if (!intel_uc_supports_guc(uc)) > >> - return 0; > >> + GEM_BUG_ON(!intel_uc_supports_guc(uc)); > >> + GEM_BUG_ON(!intel_uc_uses_guc(uc)); > >> > >> /* > >> * We can silently continue without GuC only if it was never > >> enabled > >> * before on this system after reboot, otherwise we risk GPU > >> hangs. > >> * To check if GuC was loaded before we look at WOPCM registers. > >> */ > > > > This comment still required? > > Shouldn't there be a call here to __uc_check_hw() instead? > > ok, will replace below call to uc_is_wopcm_locked() with __uc_check_hw() > to avoid redundant (but otherwise still valid) comment > > > > >> - if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc)) > >> - return 0; > >> - > >> if (!intel_uc_fw_is_available(&guc->fw)) { > >> ret = uc_is_wopcm_locked(uc) || > > ^^^^^^^^^^^^^^^^^^ Ah, I see. I think I still like the chaining up to __uc_check_hw() for the continuity aspect, and only being scared once by the warning. -Chris
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 3ffc6267f96e..da401e97bba3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -12,6 +12,19 @@ #include "i915_drv.h" +static int __uc_check_hw(struct intel_uc *uc); +static int __uc_init_hw(struct intel_uc *uc); +static void __uc_fini_hw(struct intel_uc *uc); + +static const struct intel_uc_ops uc_ops_off = { + .init_hw = __uc_check_hw, +}; + +static const struct intel_uc_ops uc_ops_on = { + .init_hw = __uc_init_hw, + .fini_hw = __uc_fini_hw, +}; + /* Reset GuC providing us with fresh state for both GuC and HuC. */ static int __intel_uc_reset_hw(struct intel_uc *uc) @@ -89,6 +102,11 @@ void intel_uc_init_early(struct intel_uc *uc) intel_huc_init_early(&uc->huc); __confirm_options(uc); + + if (intel_uc_uses_guc(uc)) + uc->ops = &uc_ops_on; + else + uc->ops = &uc_ops_off; } void intel_uc_driver_late_release(struct intel_uc *uc) @@ -380,24 +398,37 @@ static bool uc_is_wopcm_locked(struct intel_uc *uc) (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & GUC_WOPCM_OFFSET_VALID); } -int intel_uc_init_hw(struct intel_uc *uc) +static int __uc_check_hw(struct intel_uc *uc) +{ + if (!intel_uc_supports_guc(uc)) + return 0; + + /* + * We can silently continue without GuC only if it was never enabled + * before on this system after reboot, otherwise we risk GPU hangs. + * To check if GuC was loaded before we look at WOPCM registers. + */ + if (uc_is_wopcm_locked(uc)) + return -EIO; + + return 0; +} + +static int __uc_init_hw(struct intel_uc *uc) { struct drm_i915_private *i915 = uc_to_gt(uc)->i915; struct intel_guc *guc = &uc->guc; struct intel_huc *huc = &uc->huc; int ret, attempts; - if (!intel_uc_supports_guc(uc)) - return 0; + GEM_BUG_ON(!intel_uc_supports_guc(uc)); + GEM_BUG_ON(!intel_uc_uses_guc(uc)); /* * We can silently continue without GuC only if it was never enabled * before on this system after reboot, otherwise we risk GPU hangs. * To check if GuC was loaded before we look at WOPCM registers. */ - if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc)) - return 0; - if (!intel_uc_fw_is_available(&guc->fw)) { ret = uc_is_wopcm_locked(uc) || intel_uc_fw_is_overridden(&guc->fw) || @@ -495,7 +526,7 @@ int intel_uc_init_hw(struct intel_uc *uc) return -EIO; } -void intel_uc_fini_hw(struct intel_uc *uc) +static void __uc_fini_hw(struct intel_uc *uc) { struct intel_guc *guc = &uc->guc; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h index 527995c21196..d5c2d4cf1d38 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h @@ -10,7 +10,15 @@ #include "intel_huc.h" #include "i915_params.h" +struct intel_uc; + +struct intel_uc_ops { + int (*init_hw)(struct intel_uc *uc); + void (*fini_hw)(struct intel_uc *uc); +}; + struct intel_uc { + struct intel_uc_ops const *ops; struct intel_guc guc; struct intel_huc huc; @@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc); void intel_uc_cleanup_firmwares(struct intel_uc *uc); void intel_uc_sanitize(struct intel_uc *uc); void intel_uc_init(struct intel_uc *uc); -int intel_uc_init_hw(struct intel_uc *uc); -void intel_uc_fini_hw(struct intel_uc *uc); void intel_uc_fini(struct intel_uc *uc); void intel_uc_reset_prepare(struct intel_uc *uc); void intel_uc_suspend(struct intel_uc *uc); @@ -64,4 +70,15 @@ static inline bool intel_uc_uses_huc(struct intel_uc *uc) return intel_huc_is_enabled(&uc->huc); } +#define intel_uc_ops_function(_NAME, _OPS, _TYPE, _RET) \ +static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \ +{ \ + if (uc->ops->_OPS) \ + return uc->ops->_OPS(uc); \ + return _RET; \ +} +intel_uc_ops_function(init_hw, init_hw, int, 0); +intel_uc_ops_function(fini_hw, fini_hw, void, ); +#undef intel_uc_ops_function + #endif
Instead of spreading multiple conditionals across the uC code to find out current mode of uC operation, start using predefined set of function pointers that reflect that mode. Begin with pair of init_hw/fini_hw functions that are responsible for uC hardware initialization and cleanup. v2: drop ops_none, use macro to generate ops helpers Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 45 ++++++++++++++++++++++----- drivers/gpu/drm/i915/gt/uc/intel_uc.h | 21 +++++++++++-- 2 files changed, 57 insertions(+), 9 deletions(-)