| Message ID | 20190517162227.6436-6-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GuC fixes | expand |
Quoting Michal Wajdeczko (2019-05-17 17:22:25) > We may skip reset preparation steps if GuC is already sanitized. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 86edfa5ad72e..36c53a42927c 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915) > if (!USES_GUC(i915)) > return; > > + if (!intel_guc_is_alive(guc)) > + return; Does it not replace "if (!USES_GUC(i915))"? -Chris
On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-05-17 17:22:25) >> We may skip reset preparation steps if GuC is already sanitized. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> drivers/gpu/drm/i915/intel_uc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 86edfa5ad72e..36c53a42927c 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private >> *i915) >> if (!USES_GUC(i915)) >> return; >> >> + if (!intel_guc_is_alive(guc)) >> + return; > > Does it not replace "if (!USES_GUC(i915))"? Yes it can, as we will never fetch/upload fw if we don't plan to use it ;) Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded or intel_guc_is_started to better describe what this function is reporting, as one can think that intel_guc_is_alive is actually checking that GuC fw is responsive, which in general might not be the same as "loaded" Michal
Quoting Michal Wajdeczko (2019-05-17 18:11:07) > On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2019-05-17 17:22:25) > >> We may skip reset preparation steps if GuC is already sanitized. > >> > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_uc.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_uc.c > >> b/drivers/gpu/drm/i915/intel_uc.c > >> index 86edfa5ad72e..36c53a42927c 100644 > >> --- a/drivers/gpu/drm/i915/intel_uc.c > >> +++ b/drivers/gpu/drm/i915/intel_uc.c > >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private > >> *i915) > >> if (!USES_GUC(i915)) > >> return; > >> > >> + if (!intel_guc_is_alive(guc)) > >> + return; > > > > Does it not replace "if (!USES_GUC(i915))"? > > Yes it can, as we will never fetch/upload fw if we don't plan to use it ;) > > Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded > or intel_guc_is_started to better describe what this function is reporting, > as one can think that intel_guc_is_alive is actually checking that GuC fw > is responsive, which in general might not be the same as "loaded" Either seems reasonable, though there might be good reason to have both :) intel_guc_is_loaded intel_guc_has_started / intel_guc_is_active -chris
On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-05-17 18:11:07) >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25) >> >> We may skip reset preparation steps if GuC is already sanitized. >> >> >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/intel_uc.c | 3 +++ >> >> 1 file changed, 3 insertions(+) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> >> b/drivers/gpu/drm/i915/intel_uc.c >> >> index 86edfa5ad72e..36c53a42927c 100644 >> >> --- a/drivers/gpu/drm/i915/intel_uc.c >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct >> drm_i915_private >> >> *i915) >> >> if (!USES_GUC(i915)) >> >> return; >> >> >> >> + if (!intel_guc_is_alive(guc)) >> >> + return; >> > >> > Does it not replace "if (!USES_GUC(i915))"? >> >> Yes it can, as we will never fetch/upload fw if we don't plan to use it >> ;) >> >> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded >> or intel_guc_is_started to better describe what this function is >> reporting, >> as one can think that intel_guc_is_alive is actually checking that GuC >> fw >> is responsive, which in general might not be the same as "loaded" > > Either seems reasonable, though there might be good reason to have both > :) > > intel_guc_is_loaded > intel_guc_has_started / intel_guc_is_active On GuC load failure, or on reset, we immediately sanitize fw load status, so until we provide real runtime connectivity check, if ever be required, I assume we can stay with just one function: intel_guc_is_loaded, ok? Michal
Quoting Michal Wajdeczko (2019-05-17 19:01:06) > On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2019-05-17 18:11:07) > >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson > >> <chris@chris-wilson.co.uk> wrote: > >> > >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25) > >> >> We may skip reset preparation steps if GuC is already sanitized. > >> >> > >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> >> --- > >> >> drivers/gpu/drm/i915/intel_uc.c | 3 +++ > >> >> 1 file changed, 3 insertions(+) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c > >> >> b/drivers/gpu/drm/i915/intel_uc.c > >> >> index 86edfa5ad72e..36c53a42927c 100644 > >> >> --- a/drivers/gpu/drm/i915/intel_uc.c > >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c > >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct > >> drm_i915_private > >> >> *i915) > >> >> if (!USES_GUC(i915)) > >> >> return; > >> >> > >> >> + if (!intel_guc_is_alive(guc)) > >> >> + return; > >> > > >> > Does it not replace "if (!USES_GUC(i915))"? > >> > >> Yes it can, as we will never fetch/upload fw if we don't plan to use it > >> ;) > >> > >> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded > >> or intel_guc_is_started to better describe what this function is > >> reporting, > >> as one can think that intel_guc_is_alive is actually checking that GuC > >> fw > >> is responsive, which in general might not be the same as "loaded" > > > > Either seems reasonable, though there might be good reason to have both > > :) > > > > intel_guc_is_loaded > > intel_guc_has_started / intel_guc_is_active > > On GuC load failure, or on reset, we immediately sanitize fw load status, > so until we provide real runtime connectivity check, if ever be required, > I assume we can stay with just one function: intel_guc_is_loaded, ok? Fine by me, -Quack
Quoting Michal Wajdeczko (2019-05-17 19:01:06) > On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2019-05-17 18:11:07) > >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson > >> <chris@chris-wilson.co.uk> wrote: > >> > >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25) > >> >> We may skip reset preparation steps if GuC is already sanitized. > >> >> > >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> >> --- > >> >> drivers/gpu/drm/i915/intel_uc.c | 3 +++ > >> >> 1 file changed, 3 insertions(+) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c > >> >> b/drivers/gpu/drm/i915/intel_uc.c > >> >> index 86edfa5ad72e..36c53a42927c 100644 > >> >> --- a/drivers/gpu/drm/i915/intel_uc.c > >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c > >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct > >> drm_i915_private > >> >> *i915) > >> >> if (!USES_GUC(i915)) > >> >> return; > >> >> > >> >> + if (!intel_guc_is_alive(guc)) > >> >> + return; > >> > > >> > Does it not replace "if (!USES_GUC(i915))"? > >> > >> Yes it can, as we will never fetch/upload fw if we don't plan to use it > >> ;) > >> > >> Btw, I'm thinking of renaming intel_guc_is_alive to intel_guc_is_loaded > >> or intel_guc_is_started to better describe what this function is > >> reporting, > >> as one can think that intel_guc_is_alive is actually checking that GuC > >> fw > >> is responsive, which in general might not be the same as "loaded" > > > > Either seems reasonable, though there might be good reason to have both > > :) > > > > intel_guc_is_loaded > > intel_guc_has_started / intel_guc_is_active > > On GuC load failure, or on reset, we immediately sanitize fw load status, > so until we provide real runtime connectivity check, if ever be required, > I assume we can stay with just one function: intel_guc_is_loaded, ok? Would a similar one for huc also work? Would it be reliable enough to replace HUC_STATUS query? (Seems silly to wake the device up just to answer if we've loaded the firmware successfully.) -Chris
On Fri, 17 May 2019 22:08:56 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-05-17 19:01:06) >> On Fri, 17 May 2019 19:14:01 +0200, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >> > Quoting Michal Wajdeczko (2019-05-17 18:11:07) >> >> On Fri, 17 May 2019 18:31:31 +0200, Chris Wilson >> >> <chris@chris-wilson.co.uk> wrote: >> >> >> >> > Quoting Michal Wajdeczko (2019-05-17 17:22:25) >> >> >> We may skip reset preparation steps if GuC is already sanitized. >> >> >> >> >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> >> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> >> --- >> >> >> drivers/gpu/drm/i915/intel_uc.c | 3 +++ >> >> >> 1 file changed, 3 insertions(+) >> >> >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> >> >> b/drivers/gpu/drm/i915/intel_uc.c >> >> >> index 86edfa5ad72e..36c53a42927c 100644 >> >> >> --- a/drivers/gpu/drm/i915/intel_uc.c >> >> >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> >> >> @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct >> >> drm_i915_private >> >> >> *i915) >> >> >> if (!USES_GUC(i915)) >> >> >> return; >> >> >> >> >> >> + if (!intel_guc_is_alive(guc)) >> >> >> + return; >> >> > >> >> > Does it not replace "if (!USES_GUC(i915))"? >> >> >> >> Yes it can, as we will never fetch/upload fw if we don't plan to use >> it >> >> ;) >> >> >> >> Btw, I'm thinking of renaming intel_guc_is_alive to >> intel_guc_is_loaded >> >> or intel_guc_is_started to better describe what this function is >> >> reporting, >> >> as one can think that intel_guc_is_alive is actually checking that >> GuC >> >> fw >> >> is responsive, which in general might not be the same as "loaded" >> > >> > Either seems reasonable, though there might be good reason to have >> both >> > :) >> > >> > intel_guc_is_loaded >> > intel_guc_has_started / intel_guc_is_active >> >> On GuC load failure, or on reset, we immediately sanitize fw load >> status, >> so until we provide real runtime connectivity check, if ever be >> required, >> I assume we can stay with just one function: intel_guc_is_loaded, ok? > > Would a similar one for huc also work? Would it be reliable enough to > replace HUC_STATUS query? (Seems silly to wake the device up just to > answer if we've loaded the firmware successfully.) I'm not sure that we can drop HUC_STATUS query as maybe this bit can go off (who can confirm that?) but we definitely can replace HAS_HUC (btw it should be USES_HUC) with new intel_huc_is_loaded Michal
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 86edfa5ad72e..36c53a42927c 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -499,6 +499,9 @@ void intel_uc_reset_prepare(struct drm_i915_private *i915) if (!USES_GUC(i915)) return; + if (!intel_guc_is_alive(guc)) + return; + guc_stop_communication(guc); __uc_sanitize(i915); }
We may skip reset preparation steps if GuC is already sanitized. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/intel_uc.c | 3 +++ 1 file changed, 3 insertions(+)