| Message ID | 20210624070516.21893-48-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GuC submission support | expand |
On 24/06/2021 10:05, Matthew Brost wrote: > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Unblock GuC submission on Gen11+ platforms. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- > 4 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index fae01dc8e1b9..77981788204f 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -54,6 +54,7 @@ struct intel_guc { > struct ida guc_ids; > struct list_head guc_id_list; > > + bool submission_supported; > bool submission_selected; > > struct i915_vma *ads_vma; > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a427336ce916..405339202280 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct intel_guc *guc) > /* Note: By the time we're here, GuC may have already been reset */ > } > > +static bool __guc_submission_supported(struct intel_guc *guc) > +{ > + /* GuC submission is unavailable for pre-Gen11 */ > + return intel_guc_is_supported(guc) && > + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; > +} > + > static bool __guc_submission_selected(struct intel_guc *guc) > { > struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct intel_guc *guc) > > void intel_guc_submission_init_early(struct intel_guc *guc) > { > + guc->submission_supported = __guc_submission_supported(guc); > guc->submission_selected = __guc_submission_selected(guc); > } > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > index a2a3fad72be1..be767eb6ff71 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc, > > static inline bool intel_guc_submission_is_supported(struct intel_guc *guc) > { > - /* XXX: GuC submission is unavailable for now */ > - return false; > + return guc->submission_supported; > } > > static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc) > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 7a69c3c027e9..61be0aa81492 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct intel_uc *uc) > return; > } > > - /* Default: enable HuC authentication only */ > - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > + /* Intermediate platforms are HuC authentication only */ > + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > + drm_dbg(&i915->drm, "Disabling GuC only due to old platform\n"); This comment does not seem accurate, given that DG1 is barely out, and ADL is not out yet. How about: "Disabling GuC on untested platforms"? > + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > + return; > + } > + > + /* Default: enable HuC authentication and GuC submission */ > + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | ENABLE_GUC_SUBMISSION; This seems to be in contradiction with the GuC submission plan which states: "Not enabled by default on any current platforms but can be enabled via modparam enable_guc". When you rework the patch, could you please add a warning when the user force-enables the GuC Command Submission? Something like: "WARNING: The user force-enabled the experimental GuC command submission backend using i915.enable_guc. Please disable it if experiencing stability issues. No bug reports will be accepted on this backend". This should allow you to work on the backend, while communicating clearly to users that it is not ready just yet. Once it has matured, the warning can be removed. Cheers, Martin > } > > /* Reset GuC providing us with fresh state for both GuC and HuC. > @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc) > if (i915_inject_probe_failure(uc_to_gt(uc)->i915)) > return -ENOMEM; > > - /* XXX: GuC submission is unavailable for now */ > - GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); > - > ret = intel_guc_init(guc); > if (ret) > return ret; >
On Wed, Jun 30, 2021 at 11:22:38AM +0300, Martin Peres wrote: > > > On 24/06/2021 10:05, Matthew Brost wrote: > > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > > Unblock GuC submission on Gen11+ platforms. > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- > > 4 files changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > > index fae01dc8e1b9..77981788204f 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > > @@ -54,6 +54,7 @@ struct intel_guc { > > struct ida guc_ids; > > struct list_head guc_id_list; > > + bool submission_supported; > > bool submission_selected; > > struct i915_vma *ads_vma; > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index a427336ce916..405339202280 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct intel_guc *guc) > > /* Note: By the time we're here, GuC may have already been reset */ > > } > > +static bool __guc_submission_supported(struct intel_guc *guc) > > +{ > > + /* GuC submission is unavailable for pre-Gen11 */ > > + return intel_guc_is_supported(guc) && > > + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; > > +} > > + > > static bool __guc_submission_selected(struct intel_guc *guc) > > { > > struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > > @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct intel_guc *guc) > > void intel_guc_submission_init_early(struct intel_guc *guc) > > { > > + guc->submission_supported = __guc_submission_supported(guc); > > guc->submission_selected = __guc_submission_selected(guc); > > } > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > > index a2a3fad72be1..be767eb6ff71 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > > @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc, > > static inline bool intel_guc_submission_is_supported(struct intel_guc *guc) > > { > > - /* XXX: GuC submission is unavailable for now */ > > - return false; > > + return guc->submission_supported; > > } > > static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > index 7a69c3c027e9..61be0aa81492 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct intel_uc *uc) > > return; > > } > > - /* Default: enable HuC authentication only */ > > - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > > + /* Intermediate platforms are HuC authentication only */ > > + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > > + drm_dbg(&i915->drm, "Disabling GuC only due to old platform\n"); > > This comment does not seem accurate, given that DG1 is barely out, and ADL > is not out yet. How about: > > "Disabling GuC on untested platforms"? This isn't my comment but it seems right to me. AFAIK this describes the current PR but it is subject to change (i.e. we may enable GuC on DG1 by default at some point). > > > + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > > + return; > > + } > > + > > + /* Default: enable HuC authentication and GuC submission */ > > + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | ENABLE_GUC_SUBMISSION; > > This seems to be in contradiction with the GuC submission plan which states: > > "Not enabled by default on any current platforms but can be enabled via > modparam enable_guc". > I don't believe any current platform gets this point where GuC submission would be enabled by default. The first would be ADL-P which isn't out yet. > When you rework the patch, could you please add a warning when the user > force-enables the GuC Command Submission? Something like: > > "WARNING: The user force-enabled the experimental GuC command submission > backend using i915.enable_guc. Please disable it if experiencing stability > issues. No bug reports will be accepted on this backend". > > This should allow you to work on the backend, while communicating clearly to > users that it is not ready just yet. Once it has matured, the warning can be > removed. This is a good idea but the only issue I see this message blowing up CI. We plan to enable GuC submission, via a modparam, on several platforms (e.g. TGL) where TGL isn't the PR in CI. I think if is a debug level message CI should be happy but I'll double check on this. Matt > > Cheers, > Martin > > > } > > /* Reset GuC providing us with fresh state for both GuC and HuC. > > @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc) > > if (i915_inject_probe_failure(uc_to_gt(uc)->i915)) > > return -ENOMEM; > > - /* XXX: GuC submission is unavailable for now */ > > - GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); > > - > > ret = intel_guc_init(guc); > > if (ret) > > return ret; > >
On 6/30/2021 01:22, Martin Peres wrote: > On 24/06/2021 10:05, Matthew Brost wrote: >> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> Unblock GuC submission on Gen11+ platforms. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- >> 4 files changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> index fae01dc8e1b9..77981788204f 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> @@ -54,6 +54,7 @@ struct intel_guc { >> struct ida guc_ids; >> struct list_head guc_id_list; >> + bool submission_supported; >> bool submission_selected; >> struct i915_vma *ads_vma; >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> index a427336ce916..405339202280 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >> @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct >> intel_guc *guc) >> /* Note: By the time we're here, GuC may have already been >> reset */ >> } >> +static bool __guc_submission_supported(struct intel_guc *guc) >> +{ >> + /* GuC submission is unavailable for pre-Gen11 */ >> + return intel_guc_is_supported(guc) && >> + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; >> +} >> + >> static bool __guc_submission_selected(struct intel_guc *guc) >> { >> struct drm_i915_private *i915 = guc_to_gt(guc)->i915; >> @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct >> intel_guc *guc) >> void intel_guc_submission_init_early(struct intel_guc *guc) >> { >> + guc->submission_supported = __guc_submission_supported(guc); >> guc->submission_selected = __guc_submission_selected(guc); >> } >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >> index a2a3fad72be1..be767eb6ff71 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >> @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc >> *guc, >> static inline bool intel_guc_submission_is_supported(struct >> intel_guc *guc) >> { >> - /* XXX: GuC submission is unavailable for now */ >> - return false; >> + return guc->submission_supported; >> } >> static inline bool intel_guc_submission_is_wanted(struct >> intel_guc *guc) >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index 7a69c3c027e9..61be0aa81492 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >> intel_uc *uc) >> return; >> } >> - /* Default: enable HuC authentication only */ >> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >> + /* Intermediate platforms are HuC authentication only */ >> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >> + drm_dbg(&i915->drm, "Disabling GuC only due to old >> platform\n"); > > This comment does not seem accurate, given that DG1 is barely out, and > ADL is not out yet. How about: > > "Disabling GuC on untested platforms"? > Just because something is not in the shops yet does not mean it is new. Technology is always obsolete by the time it goes on sale. And the issue is not a lack of testing, it is a question of whether we are allowed to change the default on something that has already started being used by customers or not (including pre-release beta customers). I.e. it is basically a political decision not an engineering decision. >> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >> + return; >> + } >> + >> + /* Default: enable HuC authentication and GuC submission */ >> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | >> ENABLE_GUC_SUBMISSION; > > This seems to be in contradiction with the GuC submission plan which > states: > > "Not enabled by default on any current platforms but can be enabled > via modparam enable_guc". All current platforms have already been explicitly tested for above. This is setting the default on newer platforms - ADL-P and later. For which the official expectation is to have GuC enabled. > > When you rework the patch, could you please add a warning when the > user force-enables the GuC Command Submission? There already is one. If you set the module parameter then the kernel is tainted. That means 'here be dragons' - you have done something officially not supported to your kernel so all bets are off, if it blows up it is your own problem. > Something like: > > "WARNING: The user force-enabled the experimental GuC command > submission backend using i915.enable_guc. Please disable it if > experiencing stability issues. No bug reports will be accepted on this > backend". > > This should allow you to work on the backend, while communicating > clearly to users that it is not ready just yet. Once it has matured, > the warning can be removed. The fact that ADL-P is not on the shelves in your local retail store should be sufficient to ensure that users are aware that ADL-P support is not entirely mature yet. And in many ways, not just GuC based submission. John. > > Cheers, > Martin > >> } >> /* Reset GuC providing us with fresh state for both GuC and HuC. >> @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc) >> if (i915_inject_probe_failure(uc_to_gt(uc)->i915)) >> return -ENOMEM; >> - /* XXX: GuC submission is unavailable for now */ >> - GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); >> - >> ret = intel_guc_init(guc); >> if (ret) >> return ret; >>
On Wed, 30 Jun 2021 11:58:25 -0700 John Harrison <john.c.harrison@intel.com> wrote: > On 6/30/2021 01:22, Martin Peres wrote: > > On 24/06/2021 10:05, Matthew Brost wrote: > >> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> > >> Unblock GuC submission on Gen11+ platforms. > >> > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Signed-off-by: Matthew Brost <matthew.brost@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > >> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > >> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- > >> 4 files changed, 19 insertions(+), 7 deletions(-) > >> ... > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> index 7a69c3c027e9..61be0aa81492 100644 > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct > >> intel_uc *uc) > >> return; > >> } > >> - /* Default: enable HuC authentication only */ > >> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > >> + /* Intermediate platforms are HuC authentication only */ > >> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > >> + drm_dbg(&i915->drm, "Disabling GuC only due to old > >> platform\n"); > > > > This comment does not seem accurate, given that DG1 is barely out, and > > ADL is not out yet. How about: > > > > "Disabling GuC on untested platforms"? > > > Just because something is not in the shops yet does not mean it is new. > Technology is always obsolete by the time it goes on sale. That is a very good reason to not use terminology like "new", "old", "current", "modern" etc. at all. End users like me definitely do not share your interpretation of "old". Thanks, pq > And the issue is not a lack of testing, it is a question of whether we > are allowed to change the default on something that has already started > being used by customers or not (including pre-release beta customers). > I.e. it is basically a political decision not an engineering decision.
On 30/06/2021 21:00, Matthew Brost wrote: > On Wed, Jun 30, 2021 at 11:22:38AM +0300, Martin Peres wrote: >> >> >> On 24/06/2021 10:05, Matthew Brost wrote: >>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> >>> Unblock GuC submission on Gen11+ platforms. >>> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- >>> 4 files changed, 19 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >>> index fae01dc8e1b9..77981788204f 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >>> @@ -54,6 +54,7 @@ struct intel_guc { >>> struct ida guc_ids; >>> struct list_head guc_id_list; >>> + bool submission_supported; >>> bool submission_selected; >>> struct i915_vma *ads_vma; >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> index a427336ce916..405339202280 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct intel_guc *guc) >>> /* Note: By the time we're here, GuC may have already been reset */ >>> } >>> +static bool __guc_submission_supported(struct intel_guc *guc) >>> +{ >>> + /* GuC submission is unavailable for pre-Gen11 */ >>> + return intel_guc_is_supported(guc) && >>> + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; >>> +} >>> + >>> static bool __guc_submission_selected(struct intel_guc *guc) >>> { >>> struct drm_i915_private *i915 = guc_to_gt(guc)->i915; >>> @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct intel_guc *guc) >>> void intel_guc_submission_init_early(struct intel_guc *guc) >>> { >>> + guc->submission_supported = __guc_submission_supported(guc); >>> guc->submission_selected = __guc_submission_selected(guc); >>> } >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >>> index a2a3fad72be1..be767eb6ff71 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h >>> @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc, >>> static inline bool intel_guc_submission_is_supported(struct intel_guc *guc) >>> { >>> - /* XXX: GuC submission is unavailable for now */ >>> - return false; >>> + return guc->submission_supported; >>> } >>> static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc) >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>> index 7a69c3c027e9..61be0aa81492 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct intel_uc *uc) >>> return; >>> } >>> - /* Default: enable HuC authentication only */ >>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>> + /* Intermediate platforms are HuC authentication only */ >>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>> + drm_dbg(&i915->drm, "Disabling GuC only due to old platform\n"); >> >> This comment does not seem accurate, given that DG1 is barely out, and ADL >> is not out yet. How about: >> >> "Disabling GuC on untested platforms"? > > This isn't my comment but it seems right to me. AFAIK this describes the > current PR but it is subject to change (i.e. we may enable GuC on DG1 by > default at some point). Well, it's pretty bad PR to say that DG1 and ADL are old when they are not even out ;) But seriously, fix this sentence, it makes no sense at all unless you are really trying to confuse non-native speakers (and annoy language purists too). > >> >>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>> + return; >>> + } >>> + >>> + /* Default: enable HuC authentication and GuC submission */ >>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | ENABLE_GUC_SUBMISSION; >> >> This seems to be in contradiction with the GuC submission plan which states: >> >> "Not enabled by default on any current platforms but can be enabled via >> modparam enable_guc". >> > > I don't believe any current platform gets this point where GuC > submission would be enabled by default. The first would be ADL-P which > isn't out yet. Isn't that exactly what the line above does? > >> When you rework the patch, could you please add a warning when the user >> force-enables the GuC Command Submission? Something like: >> >> "WARNING: The user force-enabled the experimental GuC command submission >> backend using i915.enable_guc. Please disable it if experiencing stability >> issues. No bug reports will be accepted on this backend". >> >> This should allow you to work on the backend, while communicating clearly to >> users that it is not ready just yet. Once it has matured, the warning can be >> removed. > > This is a good idea but the only issue I see this message blowing up CI. > We plan to enable GuC submission, via a modparam, on several platforms > (e.g. TGL) where TGL isn't the PR in CI. I think if is a debug level > message CI should be happy but I'll double check on this. Some taints would be problematic. The only issue you may have is the IGT reload tests which could give you a dmesg-warn if you were to use any level under level 5. So put is as an info message and you'll be good :) In case of doubt, just ask Petri / Adrinael, he is your local IGT maintainer. Martin > > Matt > >> >> Cheers, >> Martin >> >>> } >>> /* Reset GuC providing us with fresh state for both GuC and HuC. >>> @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc) >>> if (i915_inject_probe_failure(uc_to_gt(uc)->i915)) >>> return -ENOMEM; >>> - /* XXX: GuC submission is unavailable for now */ >>> - GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); >>> - >>> ret = intel_guc_init(guc); >>> if (ret) >>> return ret; >>>
On 01/07/2021 11:14, Pekka Paalanen wrote: > On Wed, 30 Jun 2021 11:58:25 -0700 > John Harrison <john.c.harrison@intel.com> wrote: > >> On 6/30/2021 01:22, Martin Peres wrote: >>> On 24/06/2021 10:05, Matthew Brost wrote: >>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> >>>> Unblock GuC submission on Gen11+ platforms. >>>> >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- >>>> 4 files changed, 19 insertions(+), 7 deletions(-) >>>> > > ... > >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>> index 7a69c3c027e9..61be0aa81492 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >>>> intel_uc *uc) >>>> return; >>>> } >>>> - /* Default: enable HuC authentication only */ >>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>> + /* Intermediate platforms are HuC authentication only */ >>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old >>>> platform\n"); >>> >>> This comment does not seem accurate, given that DG1 is barely out, and >>> ADL is not out yet. How about: >>> >>> "Disabling GuC on untested platforms"? >>> >> Just because something is not in the shops yet does not mean it is new. >> Technology is always obsolete by the time it goes on sale. > > That is a very good reason to not use terminology like "new", "old", > "current", "modern" etc. at all. > > End users like me definitely do not share your interpretation of "old". Yep, old and new is relative. In the end, what matters is the validation effort, which is why I was proposing "untested platforms". Also, remember that you are not writing these messages for Intel engineers, but instead are writing for Linux *users*. Cheers, Martin > > > Thanks, > pq > > >> And the issue is not a lack of testing, it is a question of whether we >> are allowed to change the default on something that has already started >> being used by customers or not (including pre-release beta customers). >> I.e. it is basically a political decision not an engineering decision. >
On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> wrote: > > On 01/07/2021 11:14, Pekka Paalanen wrote: > > On Wed, 30 Jun 2021 11:58:25 -0700 > > John Harrison <john.c.harrison@intel.com> wrote: > > > >> On 6/30/2021 01:22, Martin Peres wrote: > >>> On 24/06/2021 10:05, Matthew Brost wrote: > >>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >>>> > >>>> Unblock GuC submission on Gen11+ platforms. > >>>> > >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > >>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- > >>>> 4 files changed, 19 insertions(+), 7 deletions(-) > >>>> > > > > ... > > > >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>> index 7a69c3c027e9..61be0aa81492 100644 > >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct > >>>> intel_uc *uc) > >>>> return; > >>>> } > >>>> - /* Default: enable HuC authentication only */ > >>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > >>>> + /* Intermediate platforms are HuC authentication only */ > >>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > >>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old > >>>> platform\n"); > >>> > >>> This comment does not seem accurate, given that DG1 is barely out, and > >>> ADL is not out yet. How about: > >>> > >>> "Disabling GuC on untested platforms"? > >>> > >> Just because something is not in the shops yet does not mean it is new. > >> Technology is always obsolete by the time it goes on sale. > > > > That is a very good reason to not use terminology like "new", "old", > > "current", "modern" etc. at all. > > > > End users like me definitely do not share your interpretation of "old". > > Yep, old and new is relative. In the end, what matters is the validation > effort, which is why I was proposing "untested platforms". > > Also, remember that you are not writing these messages for Intel > engineers, but instead are writing for Linux *users*. It's drm_dbg. Users don't read this stuff, at least not users with no clue what the driver does and stuff like that. -Daniel
On Thu, 1 Jul 2021 21:28:06 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> wrote: > > > > On 01/07/2021 11:14, Pekka Paalanen wrote: > > > On Wed, 30 Jun 2021 11:58:25 -0700 > > > John Harrison <john.c.harrison@intel.com> wrote: > > > > > >> On 6/30/2021 01:22, Martin Peres wrote: > > >>> On 24/06/2021 10:05, Matthew Brost wrote: > > >>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > >>>> > > >>>> Unblock GuC submission on Gen11+ platforms. > > >>>> > > >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > >>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > >>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > >>>> --- > > >>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > > >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > > >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > > >>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- > > >>>> 4 files changed, 19 insertions(+), 7 deletions(-) > > >>>> > > > > > > ... > > > > > >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > >>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > >>>> index 7a69c3c027e9..61be0aa81492 100644 > > >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > >>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct > > >>>> intel_uc *uc) > > >>>> return; > > >>>> } > > >>>> - /* Default: enable HuC authentication only */ > > >>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > > >>>> + /* Intermediate platforms are HuC authentication only */ > > >>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > > >>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old > > >>>> platform\n"); > > >>> > > >>> This comment does not seem accurate, given that DG1 is barely out, and > > >>> ADL is not out yet. How about: > > >>> > > >>> "Disabling GuC on untested platforms"? > > >>> > > >> Just because something is not in the shops yet does not mean it is new. > > >> Technology is always obsolete by the time it goes on sale. > > > > > > That is a very good reason to not use terminology like "new", "old", > > > "current", "modern" etc. at all. > > > > > > End users like me definitely do not share your interpretation of "old". > > > > Yep, old and new is relative. In the end, what matters is the validation > > effort, which is why I was proposing "untested platforms". > > > > Also, remember that you are not writing these messages for Intel > > engineers, but instead are writing for Linux *users*. > > It's drm_dbg. Users don't read this stuff, at least not users with no > clue what the driver does and stuff like that. If I had a problem, I would read it, and I have no clue what anything of that is. Thanks, pq
On 02/07/2021 10:29, Pekka Paalanen wrote: > On Thu, 1 Jul 2021 21:28:06 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> wrote: >>> >>> On 01/07/2021 11:14, Pekka Paalanen wrote: >>>> On Wed, 30 Jun 2021 11:58:25 -0700 >>>> John Harrison <john.c.harrison@intel.com> wrote: >>>> >>>>> On 6/30/2021 01:22, Martin Peres wrote: >>>>>> On 24/06/2021 10:05, Matthew Brost wrote: >>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>>>>> >>>>>>> Unblock GuC submission on Gen11+ platforms. >>>>>>> >>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- >>>>>>> 4 files changed, 19 insertions(+), 7 deletions(-) >>>>>>> >>>> >>>> ... >>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>> index 7a69c3c027e9..61be0aa81492 100644 >>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >>>>>>> intel_uc *uc) >>>>>>> return; >>>>>>> } >>>>>>> - /* Default: enable HuC authentication only */ >>>>>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>>> + /* Intermediate platforms are HuC authentication only */ >>>>>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>>>>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old >>>>>>> platform\n"); >>>>>> >>>>>> This comment does not seem accurate, given that DG1 is barely out, and >>>>>> ADL is not out yet. How about: >>>>>> >>>>>> "Disabling GuC on untested platforms"? >>>>>> >>>>> Just because something is not in the shops yet does not mean it is new. >>>>> Technology is always obsolete by the time it goes on sale. >>>> >>>> That is a very good reason to not use terminology like "new", "old", >>>> "current", "modern" etc. at all. >>>> >>>> End users like me definitely do not share your interpretation of "old". >>> >>> Yep, old and new is relative. In the end, what matters is the validation >>> effort, which is why I was proposing "untested platforms". >>> >>> Also, remember that you are not writing these messages for Intel >>> engineers, but instead are writing for Linux *users*. >> >> It's drm_dbg. Users don't read this stuff, at least not users with no >> clue what the driver does and stuff like that. > > If I had a problem, I would read it, and I have no clue what anything > of that is. Exactly. This level of defense for what is clearly a bad *debug* message (at the very least, the grammar) makes no sense at all! I don't want to hear arguments like "Not my patch" from a developer literally sending the patch to the ML and who added his SoB to the patch, playing with words, or minimizing the problem of having such a message. All of the above are just clear signals for the community to get off your playground, which is frankly unacceptable. Your email address does not matter. In the spirit of collaboration, your response should have been "Good catch, how about XXXX or YYYY?". This would not have wasted everyone's time in an attempt to just have it your way. My level of confidence in this GuC transition was already low, but you guys are working hard to shoot yourself in the foot. Trust should be earned! Martin > > > Thanks, > pq >
On 01/07/2021 21:24, Martin Peres wrote: [...] >> >>> >>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>> + return; >>>> + } >>>> + >>>> + /* Default: enable HuC authentication and GuC submission */ >>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | >>>> ENABLE_GUC_SUBMISSION; >>> >>> This seems to be in contradiction with the GuC submission plan which >>> states: >>> >>> "Not enabled by default on any current platforms but can be enabled via >>> modparam enable_guc". >>> >> >> I don't believe any current platform gets this point where GuC >> submission would be enabled by default. The first would be ADL-P which >> isn't out yet. > > Isn't that exactly what the line above does? In case you missed this crucial part of the review. Please answer the above question. Cheers, Martin
On 02.07.2021 10:13, Martin Peres wrote: > On 01/07/2021 21:24, Martin Peres wrote: > [...] >>> >>>> >>>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>> + return; >>>>> + } >>>>> + >>>>> + /* Default: enable HuC authentication and GuC submission */ >>>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | >>>>> ENABLE_GUC_SUBMISSION; >>>> >>>> This seems to be in contradiction with the GuC submission plan which >>>> states: >>>> >>>> "Not enabled by default on any current platforms but can be enabled via >>>> modparam enable_guc". >>>> >>> >>> I don't believe any current platform gets this point where GuC >>> submission would be enabled by default. The first would be ADL-P which >>> isn't out yet. >> >> Isn't that exactly what the line above does? > > In case you missed this crucial part of the review. Please answer the > above question. I guess there is some misunderstanding here, and I must admit I had similar doubt, but if you look beyond patch diff and check function code you will find that the very condition is: /* Don't enable GuC/HuC on pre-Gen12 */ if (GRAPHICS_VER(i915) < 12) { i915->params.enable_guc = 0; return; } so all pre-Gen12 platforms will continue to have GuC/HuC disabled. Thanks, Michal
On 02/07/2021 16:06, Michal Wajdeczko wrote: > > > On 02.07.2021 10:13, Martin Peres wrote: >> On 01/07/2021 21:24, Martin Peres wrote: >> [...] >>>> >>>>> >>>>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + /* Default: enable HuC authentication and GuC submission */ >>>>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | >>>>>> ENABLE_GUC_SUBMISSION; >>>>> >>>>> This seems to be in contradiction with the GuC submission plan which >>>>> states: >>>>> >>>>> "Not enabled by default on any current platforms but can be enabled via >>>>> modparam enable_guc". >>>>> >>>> >>>> I don't believe any current platform gets this point where GuC >>>> submission would be enabled by default. The first would be ADL-P which >>>> isn't out yet. >>> >>> Isn't that exactly what the line above does? >> >> In case you missed this crucial part of the review. Please answer the >> above question. > > I guess there is some misunderstanding here, and I must admit I had > similar doubt, but if you look beyond patch diff and check function code > you will find that the very condition is: > > /* Don't enable GuC/HuC on pre-Gen12 */ > if (GRAPHICS_VER(i915) < 12) { > i915->params.enable_guc = 0; > return; > } > > so all pre-Gen12 platforms will continue to have GuC/HuC disabled. Thanks Michal, but then the problem is the other way: how can one enable it on gen11? I like what Daniele was going for here: separating the capability from the user-requested value, but then it seems the patch stopped half way. How about never touching the parameter, and having a AND between the two values to get the effective enable_guc? Right now, the code is really confusing :s Thanks, Martin > > Thanks, > Michal >
On 02.07.2021 15:12, Martin Peres wrote: > On 02/07/2021 16:06, Michal Wajdeczko wrote: >> >> >> On 02.07.2021 10:13, Martin Peres wrote: >>> On 01/07/2021 21:24, Martin Peres wrote: >>> [...] >>>>> >>>>>> >>>>>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>>> + return; >>>>>>> + } >>>>>>> + >>>>>>> + /* Default: enable HuC authentication and GuC submission */ >>>>>>> + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | >>>>>>> ENABLE_GUC_SUBMISSION; >>>>>> >>>>>> This seems to be in contradiction with the GuC submission plan which >>>>>> states: >>>>>> >>>>>> "Not enabled by default on any current platforms but can be >>>>>> enabled via >>>>>> modparam enable_guc". >>>>>> >>>>> >>>>> I don't believe any current platform gets this point where GuC >>>>> submission would be enabled by default. The first would be ADL-P which >>>>> isn't out yet. >>>> >>>> Isn't that exactly what the line above does? >>> >>> In case you missed this crucial part of the review. Please answer the >>> above question. >> >> I guess there is some misunderstanding here, and I must admit I had >> similar doubt, but if you look beyond patch diff and check function code >> you will find that the very condition is: >> >> /* Don't enable GuC/HuC on pre-Gen12 */ >> if (GRAPHICS_VER(i915) < 12) { >> i915->params.enable_guc = 0; >> return; >> } >> >> so all pre-Gen12 platforms will continue to have GuC/HuC disabled. > > Thanks Michal, but then the problem is the other way: how can one enable > it on gen11? this code here converts default GuC auto mode (enable_guc=-1) into per platform desired (tested) GuC/HuC enables. to override that default, you may still use enable_guc=1 to explicitly enable GuC submission and since we also have this code: +static bool __guc_submission_supported(struct intel_guc *guc) +{ + /* GuC submission is unavailable for pre-Gen11 */ + return intel_guc_is_supported(guc) && + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; +} it should work on any Gen11+. Michal > > I like what Daniele was going for here: separating the capability from > the user-requested value, but then it seems the patch stopped half way. > How about never touching the parameter, and having a AND between the two > values to get the effective enable_guc? > > Right now, the code is really confusing :s > > Thanks, > Martin > >> >> Thanks, >> Michal >>
On 02.07.2021 10:09, Martin Peres wrote: > On 02/07/2021 10:29, Pekka Paalanen wrote: >> On Thu, 1 Jul 2021 21:28:06 +0200 >> Daniel Vetter <daniel@ffwll.ch> wrote: >> >>> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> >>> wrote: >>>> >>>> On 01/07/2021 11:14, Pekka Paalanen wrote: >>>>> On Wed, 30 Jun 2021 11:58:25 -0700 >>>>> John Harrison <john.c.harrison@intel.com> wrote: >>>>> >>>>>> On 6/30/2021 01:22, Martin Peres wrote: >>>>>>> On 24/06/2021 10:05, Matthew Brost wrote: >>>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>>>>>> >>>>>>>> Unblock GuC submission on Gen11+ platforms. >>>>>>>> >>>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>>>>> Signed-off-by: Daniele Ceraolo Spurio >>>>>>>> <daniele.ceraolospurio@intel.com> >>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 >>>>>>>> +++++++++----- >>>>>>>> 4 files changed, 19 insertions(+), 7 deletions(-) >>>>>>>> >>>>> >>>>> ... >>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>> index 7a69c3c027e9..61be0aa81492 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >>>>>>>> intel_uc *uc) >>>>>>>> return; >>>>>>>> } >>>>>>>> - /* Default: enable HuC authentication only */ >>>>>>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>>>> + /* Intermediate platforms are HuC authentication only */ >>>>>>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>>>>>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old >>>>>>>> platform\n"); >>>>>>> >>>>>>> This comment does not seem accurate, given that DG1 is barely >>>>>>> out, and >>>>>>> ADL is not out yet. How about: >>>>>>> >>>>>>> "Disabling GuC on untested platforms"? >>>>>>> >>>>>> Just because something is not in the shops yet does not mean it is >>>>>> new. >>>>>> Technology is always obsolete by the time it goes on sale. >>>>> >>>>> That is a very good reason to not use terminology like "new", "old", >>>>> "current", "modern" etc. at all. >>>>> >>>>> End users like me definitely do not share your interpretation of >>>>> "old". >>>> >>>> Yep, old and new is relative. In the end, what matters is the >>>> validation >>>> effort, which is why I was proposing "untested platforms". >>>> >>>> Also, remember that you are not writing these messages for Intel >>>> engineers, but instead are writing for Linux *users*. >>> >>> It's drm_dbg. Users don't read this stuff, at least not users with no >>> clue what the driver does and stuff like that. >> >> If I had a problem, I would read it, and I have no clue what anything >> of that is. > > Exactly. > > This level of defense for what is clearly a bad *debug* message (at the > very least, the grammar) makes no sense at all! > > I don't want to hear arguments like "Not my patch" from a developer > literally sending the patch to the ML and who added his SoB to the > patch, playing with words, or minimizing the problem of having such a > message. Agree that 'not my patch' is never a good excuse, but equally we can't blame original patch author as patch was updated few times since then. Maybe to avoid confusions and simplify reviews, we could split this patch into two smaller: first one that really unblocks GuC submission on all Gen11+ (see __guc_submission_supported) and second one that updates defaults for Gen12+ (see uc_expand_default_options), as original patch (from ~2019) evolved more than what title/commit message says. Then we can fix all messaging and make sure it's clear and understood. Thanks, Michal > > All of the above are just clear signals for the community to get off > your playground, which is frankly unacceptable. Your email address does > not matter. > > In the spirit of collaboration, your response should have been "Good > catch, how about XXXX or YYYY?". This would not have wasted everyone's > time in an attempt to just have it your way. > > My level of confidence in this GuC transition was already low, but you > guys are working hard to shoot yourself in the foot. Trust should be > earned! > > Martin > >> >> >> Thanks, >> pq >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 02/07/2021 18:07, Michal Wajdeczko wrote: > > > On 02.07.2021 10:09, Martin Peres wrote: >> On 02/07/2021 10:29, Pekka Paalanen wrote: >>> On Thu, 1 Jul 2021 21:28:06 +0200 >>> Daniel Vetter <daniel@ffwll.ch> wrote: >>> >>>> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> >>>> wrote: >>>>> >>>>> On 01/07/2021 11:14, Pekka Paalanen wrote: >>>>>> On Wed, 30 Jun 2021 11:58:25 -0700 >>>>>> John Harrison <john.c.harrison@intel.com> wrote: >>>>>> >>>>>>> On 6/30/2021 01:22, Martin Peres wrote: >>>>>>>> On 24/06/2021 10:05, Matthew Brost wrote: >>>>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>>>>>>> >>>>>>>>> Unblock GuC submission on Gen11+ platforms. >>>>>>>>> >>>>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>>>>>> Signed-off-by: Daniele Ceraolo Spurio >>>>>>>>> <daniele.ceraolospurio@intel.com> >>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 >>>>>>>>> +++++++++----- >>>>>>>>> 4 files changed, 19 insertions(+), 7 deletions(-) >>>>>>>>> >>>>>> >>>>>> ... >>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>> index 7a69c3c027e9..61be0aa81492 100644 >>>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >>>>>>>>> intel_uc *uc) >>>>>>>>> return; >>>>>>>>> } >>>>>>>>> - /* Default: enable HuC authentication only */ >>>>>>>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>>>>> + /* Intermediate platforms are HuC authentication only */ >>>>>>>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>>>>>>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old >>>>>>>>> platform\n"); >>>>>>>> >>>>>>>> This comment does not seem accurate, given that DG1 is barely >>>>>>>> out, and >>>>>>>> ADL is not out yet. How about: >>>>>>>> >>>>>>>> "Disabling GuC on untested platforms"? >>>>>>>> >>>>>>> Just because something is not in the shops yet does not mean it is >>>>>>> new. >>>>>>> Technology is always obsolete by the time it goes on sale. >>>>>> >>>>>> That is a very good reason to not use terminology like "new", "old", >>>>>> "current", "modern" etc. at all. >>>>>> >>>>>> End users like me definitely do not share your interpretation of >>>>>> "old". >>>>> >>>>> Yep, old and new is relative. In the end, what matters is the >>>>> validation >>>>> effort, which is why I was proposing "untested platforms". >>>>> >>>>> Also, remember that you are not writing these messages for Intel >>>>> engineers, but instead are writing for Linux *users*. >>>> >>>> It's drm_dbg. Users don't read this stuff, at least not users with no >>>> clue what the driver does and stuff like that. >>> >>> If I had a problem, I would read it, and I have no clue what anything >>> of that is. >> >> Exactly. >> >> This level of defense for what is clearly a bad *debug* message (at the >> very least, the grammar) makes no sense at all! >> >> I don't want to hear arguments like "Not my patch" from a developer >> literally sending the patch to the ML and who added his SoB to the >> patch, playing with words, or minimizing the problem of having such a >> message. > > Agree that 'not my patch' is never a good excuse, but equally we can't > blame original patch author as patch was updated few times since then. I never wanted to blame the author here, I was only speaking about the handling of feedback on the patch. > > Maybe to avoid confusions and simplify reviews, we could split this > patch into two smaller: first one that really unblocks GuC submission on > all Gen11+ (see __guc_submission_supported) and second one that updates > defaults for Gen12+ (see uc_expand_default_options), as original patch > (from ~2019) evolved more than what title/commit message says. Both work for me, as long as it is a collaborative effort. Cheers, Martin > > Then we can fix all messaging and make sure it's clear and understood. > > Thanks, > Michal > >> >> All of the above are just clear signals for the community to get off >> your playground, which is frankly unacceptable. Your email address does >> not matter. >> >> In the spirit of collaboration, your response should have been "Good >> catch, how about XXXX or YYYY?". This would not have wasted everyone's >> time in an attempt to just have it your way. >> >> My level of confidence in this GuC transition was already low, but you >> guys are working hard to shoot yourself in the foot. Trust should be >> earned! >> >> Martin >> >>> >>> >>> Thanks, >>> pq >>> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/3/2021 01:21, Martin Peres wrote: > On 02/07/2021 18:07, Michal Wajdeczko wrote: >> On 02.07.2021 10:09, Martin Peres wrote: >>> On 02/07/2021 10:29, Pekka Paalanen wrote: >>>> On Thu, 1 Jul 2021 21:28:06 +0200 >>>> Daniel Vetter <daniel@ffwll.ch> wrote: >>>> >>>>> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> >>>>> wrote: >>>>>> >>>>>> On 01/07/2021 11:14, Pekka Paalanen wrote: >>>>>>> On Wed, 30 Jun 2021 11:58:25 -0700 >>>>>>> John Harrison <john.c.harrison@intel.com> wrote: >>>>>>>> On 6/30/2021 01:22, Martin Peres wrote: >>>>>>>>> On 24/06/2021 10:05, Matthew Brost wrote: >>>>>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>>>>>>>> >>>>>>>>>> Unblock GuC submission on Gen11+ platforms. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>>>>>>> Signed-off-by: Daniele Ceraolo Spurio >>>>>>>>>> <daniele.ceraolospurio@intel.com> >>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 >>>>>>>>>> +++++++++----- >>>>>>>>>> 4 files changed, 19 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> ... >>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>> index 7a69c3c027e9..61be0aa81492 100644 >>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >>>>>>>>>> intel_uc *uc) >>>>>>>>>> return; >>>>>>>>>> } >>>>>>>>>> - /* Default: enable HuC authentication only */ >>>>>>>>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>>>>>> + /* Intermediate platforms are HuC authentication only */ >>>>>>>>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>>>>>>>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old >>>>>>>>>> platform\n"); >>>>>>>>> >>>>>>>>> This comment does not seem accurate, given that DG1 is barely >>>>>>>>> out, and >>>>>>>>> ADL is not out yet. How about: >>>>>>>>> >>>>>>>>> "Disabling GuC on untested platforms"? >>>>>>>> Just because something is not in the shops yet does not mean it is >>>>>>>> new. >>>>>>>> Technology is always obsolete by the time it goes on sale. >>>>>>> >>>>>>> That is a very good reason to not use terminology like "new", >>>>>>> "old", >>>>>>> "current", "modern" etc. at all. >>>>>>> >>>>>>> End users like me definitely do not share your interpretation of >>>>>>> "old". >>>>>> >>>>>> Yep, old and new is relative. In the end, what matters is the >>>>>> validation >>>>>> effort, which is why I was proposing "untested platforms". >>>>>> >>>>>> Also, remember that you are not writing these messages for Intel >>>>>> engineers, but instead are writing for Linux *users*. >>>>> >>>>> It's drm_dbg. Users don't read this stuff, at least not users with no >>>>> clue what the driver does and stuff like that. >>>> >>>> If I had a problem, I would read it, and I have no clue what anything >>>> of that is. >>> >>> Exactly. I don't see how replacing 'old' for 'untested' helps anybody to understand anything. Untested just implies we can't be bothered to test stuff before publishing it. And as previously stated, this is purely a political decision not a technical one. Sure, change the message to be 'Disabling GuC submission but enabling HuC loading via GuC on platform XXX' if that makes it clearer what is going on. Or just drop the message completely. It's simply explaining what the default option is for the current platform which you can also get by reading the code. However, I disagree that 'untested' is the correct message. Quite a lot of testing has been happening on TGL+ with GuC submission enabled. >>> >>> This level of defense for what is clearly a bad *debug* message (at the >>> very least, the grammar) makes no sense at all! >>> >>> I don't want to hear arguments like "Not my patch" from a developer >>> literally sending the patch to the ML and who added his SoB to the >>> patch, playing with words, or minimizing the problem of having such a >>> message. >> >> Agree that 'not my patch' is never a good excuse, but equally we can't >> blame original patch author as patch was updated few times since then. > > I never wanted to blame the author here, I was only speaking about the > handling of feedback on the patch. > >> >> Maybe to avoid confusions and simplify reviews, we could split this >> patch into two smaller: first one that really unblocks GuC submission on >> all Gen11+ (see __guc_submission_supported) and second one that updates >> defaults for Gen12+ (see uc_expand_default_options), as original patch >> (from ~2019) evolved more than what title/commit message says. > > Both work for me, as long as it is a collaborative effort. I'm not seeing how splitting the patch up fixes the complaints about the debug message. And to be clear, no-one is actually arguing for a code change as such? The issue is just about the text of the debug message? Or did I miss something somewhere? John. > > Cheers, > Martin > >> >> Then we can fix all messaging and make sure it's clear and understood. >> >> Thanks, >> Michal >> >>> >>> All of the above are just clear signals for the community to get off >>> your playground, which is frankly unacceptable. Your email address does >>> not matter. >>> >>> In the spirit of collaboration, your response should have been "Good >>> catch, how about XXXX or YYYY?". This would not have wasted everyone's >>> time in an attempt to just have it your way. >>> >>> My level of confidence in this GuC transition was already low, but you >>> guys are working hard to shoot yourself in the foot. Trust should be >>> earned! >>> >>> Martin >>> >>>> >>>> >>>> Thanks, >>>> pq >>>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 6 Jul 2021 17:57:35 -0700 John Harrison <john.c.harrison@intel.com> wrote: > On 7/3/2021 01:21, Martin Peres wrote: > > On 02/07/2021 18:07, Michal Wajdeczko wrote: > >> On 02.07.2021 10:09, Martin Peres wrote: > >>> On 02/07/2021 10:29, Pekka Paalanen wrote: > >>>> On Thu, 1 Jul 2021 21:28:06 +0200 > >>>> Daniel Vetter <daniel@ffwll.ch> wrote: > >>>> > >>>>> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> > >>>>> wrote: > >>>>>> > >>>>>> On 01/07/2021 11:14, Pekka Paalanen wrote: > >>>>>>> On Wed, 30 Jun 2021 11:58:25 -0700 > >>>>>>> John Harrison <john.c.harrison@intel.com> wrote: > >>>>>>>> On 6/30/2021 01:22, Martin Peres wrote: > >>>>>>>>> On 24/06/2021 10:05, Matthew Brost wrote: > >>>>>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >>>>>>>>>> > >>>>>>>>>> Unblock GuC submission on Gen11+ platforms. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >>>>>>>>>> Signed-off-by: Daniele Ceraolo Spurio > >>>>>>>>>> <daniele.ceraolospurio@intel.com> > >>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > >>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 > >>>>>>>>>> +++++++++----- > >>>>>>>>>> 4 files changed, 19 insertions(+), 7 deletions(-) > >>>>>>> > >>>>>>> ... > >>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>>>>>> index 7a69c3c027e9..61be0aa81492 100644 > >>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct > >>>>>>>>>> intel_uc *uc) > >>>>>>>>>> return; > >>>>>>>>>> } > >>>>>>>>>> - /* Default: enable HuC authentication only */ > >>>>>>>>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > >>>>>>>>>> + /* Intermediate platforms are HuC authentication only */ > >>>>>>>>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > >>>>>>>>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old > >>>>>>>>>> platform\n"); > >>>>>>>>> > >>>>>>>>> This comment does not seem accurate, given that DG1 is barely > >>>>>>>>> out, and > >>>>>>>>> ADL is not out yet. How about: > >>>>>>>>> > >>>>>>>>> "Disabling GuC on untested platforms"? > >>>>>>>> Just because something is not in the shops yet does not mean it is > >>>>>>>> new. > >>>>>>>> Technology is always obsolete by the time it goes on sale. > >>>>>>> > >>>>>>> That is a very good reason to not use terminology like "new", > >>>>>>> "old", > >>>>>>> "current", "modern" etc. at all. > >>>>>>> > >>>>>>> End users like me definitely do not share your interpretation of > >>>>>>> "old". > >>>>>> > >>>>>> Yep, old and new is relative. In the end, what matters is the > >>>>>> validation > >>>>>> effort, which is why I was proposing "untested platforms". > >>>>>> > >>>>>> Also, remember that you are not writing these messages for Intel > >>>>>> engineers, but instead are writing for Linux *users*. > >>>>> > >>>>> It's drm_dbg. Users don't read this stuff, at least not users with no > >>>>> clue what the driver does and stuff like that. > >>>> > >>>> If I had a problem, I would read it, and I have no clue what anything > >>>> of that is. > >>> > >>> Exactly. > I don't see how replacing 'old' for 'untested' helps anybody to > understand anything. Untested just implies we can't be bothered to test > stuff before publishing it. And as previously stated, this is purely a > political decision not a technical one. Sure, change the message to be > 'Disabling GuC submission but enabling HuC loading via GuC on platform > XXX' if that makes it clearer what is going on. Or just drop the message > completely. It's simply explaining what the default option is for the > current platform which you can also get by reading the code. However, I > disagree that 'untested' is the correct message. Quite a lot of testing > has been happening on TGL+ with GuC submission enabled. Hi, it seems to me that "untested" was just a wrong guess, nothing more. It was presented with "how about?", not as an exact demand. You don't have to attack that word, just use another phrasing that is both correct and not misleading to the majority of tech savvy people. Thanks, pq
On 07.07.2021 02:57, John Harrison wrote: > On 7/3/2021 01:21, Martin Peres wrote: >> On 02/07/2021 18:07, Michal Wajdeczko wrote: >>> On 02.07.2021 10:09, Martin Peres wrote: >>>> On 02/07/2021 10:29, Pekka Paalanen wrote: >>>>> On Thu, 1 Jul 2021 21:28:06 +0200 >>>>> Daniel Vetter <daniel@ffwll.ch> wrote: >>>>> >>>>>> On Thu, Jul 1, 2021 at 8:27 PM Martin Peres <martin.peres@free.fr> >>>>>> wrote: >>>>>>> >>>>>>> On 01/07/2021 11:14, Pekka Paalanen wrote: >>>>>>>> On Wed, 30 Jun 2021 11:58:25 -0700 >>>>>>>> John Harrison <john.c.harrison@intel.com> wrote: >>>>>>>>> On 6/30/2021 01:22, Martin Peres wrote: >>>>>>>>>> On 24/06/2021 10:05, Matthew Brost wrote: >>>>>>>>>>> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>>>>>>>>>> >>>>>>>>>>> Unblock GuC submission on Gen11+ platforms. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>>>>>>>>> Signed-off-by: Daniele Ceraolo Spurio >>>>>>>>>>> <daniele.ceraolospurio@intel.com> >>>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com> >>>>>>>>>>> --- >>>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + >>>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ >>>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- >>>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 >>>>>>>>>>> +++++++++----- >>>>>>>>>>> 4 files changed, 19 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> ... >>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>>> index 7a69c3c027e9..61be0aa81492 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>>>>>>> @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct >>>>>>>>>>> intel_uc *uc) >>>>>>>>>>> return; >>>>>>>>>>> } >>>>>>>>>>> - /* Default: enable HuC authentication only */ >>>>>>>>>>> - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; >>>>>>>>>>> + /* Intermediate platforms are HuC authentication only */ >>>>>>>>>>> + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { >>>>>>>>>>> + drm_dbg(&i915->drm, "Disabling GuC only due to old >>>>>>>>>>> platform\n"); >>>>>>>>>> >>>>>>>>>> This comment does not seem accurate, given that DG1 is barely >>>>>>>>>> out, and >>>>>>>>>> ADL is not out yet. How about: >>>>>>>>>> >>>>>>>>>> "Disabling GuC on untested platforms"? >>>>>>>>> Just because something is not in the shops yet does not mean it is >>>>>>>>> new. >>>>>>>>> Technology is always obsolete by the time it goes on sale. >>>>>>>> >>>>>>>> That is a very good reason to not use terminology like "new", >>>>>>>> "old", >>>>>>>> "current", "modern" etc. at all. >>>>>>>> >>>>>>>> End users like me definitely do not share your interpretation of >>>>>>>> "old". >>>>>>> >>>>>>> Yep, old and new is relative. In the end, what matters is the >>>>>>> validation >>>>>>> effort, which is why I was proposing "untested platforms". >>>>>>> >>>>>>> Also, remember that you are not writing these messages for Intel >>>>>>> engineers, but instead are writing for Linux *users*. >>>>>> >>>>>> It's drm_dbg. Users don't read this stuff, at least not users with no >>>>>> clue what the driver does and stuff like that. >>>>> >>>>> If I had a problem, I would read it, and I have no clue what anything >>>>> of that is. >>>> >>>> Exactly. > I don't see how replacing 'old' for 'untested' helps anybody to > understand anything. Untested just implies we can't be bothered to test > stuff before publishing it. And as previously stated, this is purely a > political decision not a technical one. Sure, change the message to be > 'Disabling GuC submission but enabling HuC loading via GuC on platform > XXX' if that makes it clearer what is going on. Or just drop the message > completely. It's simply explaining what the default option is for the > current platform which you can also get by reading the code. However, I > disagree that 'untested' is the correct message. Quite a lot of testing > has been happening on TGL+ with GuC submission enabled. > >>>> >>>> This level of defense for what is clearly a bad *debug* message (at the >>>> very least, the grammar) makes no sense at all! >>>> >>>> I don't want to hear arguments like "Not my patch" from a developer >>>> literally sending the patch to the ML and who added his SoB to the >>>> patch, playing with words, or minimizing the problem of having such a >>>> message. >>> >>> Agree that 'not my patch' is never a good excuse, but equally we can't >>> blame original patch author as patch was updated few times since then. >> >> I never wanted to blame the author here, I was only speaking about the >> handling of feedback on the patch. >> >>> >>> Maybe to avoid confusions and simplify reviews, we could split this >>> patch into two smaller: first one that really unblocks GuC submission on >>> all Gen11+ (see __guc_submission_supported) and second one that updates >>> defaults for Gen12+ (see uc_expand_default_options), as original patch >>> (from ~2019) evolved more than what title/commit message says. >> >> Both work for me, as long as it is a collaborative effort. > I'm not seeing how splitting the patch up fixes the complaints about the > debug message. I assume it's not just about debug message (but still related) With separate patches you can explain in commit messages: patch1: why (from technical point of view) we unblock GuC submission only for Gen11+ (as pre-Gen11 are also using the same GuC firmware so one can assume GuC submission will work there too), patch2: why (from "political" point of view) we want to turn on GuC submission by default only on selected Gen12+ platforms (as one could wonder why we don't enable GuC submission for Gen11+ since it should work there too). Then it should be easy to find proper wording for any debug message we may want to add. > > And to be clear, no-one is actually arguing for a code change as such? > The issue is just about the text of the debug message? Or did I miss > something somewhere? Change is trivial is hard to complain, what is missing, IMHO, is good rationale why we are making GuC submission enabling so selective. Michal > > John. > > >> >> Cheers, >> Martin >> >>> >>> Then we can fix all messaging and make sure it's clear and understood. >>> >>> Thanks, >>> Michal >>> >>>> >>>> All of the above are just clear signals for the community to get off >>>> your playground, which is frankly unacceptable. Your email address does >>>> not matter. >>>> >>>> In the spirit of collaboration, your response should have been "Good >>>> catch, how about XXXX or YYYY?". This would not have wasted everyone's >>>> time in an attempt to just have it your way. >>>> >>>> My level of confidence in this GuC transition was already low, but you >>>> guys are working hard to shoot yourself in the foot. Trust should be >>>> earned! >>>> >>>> Martin >>>> >>>>> >>>>> >>>>> Thanks, >>>>> pq >>>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Jun 24, 2021 at 12:05:16AM -0700, Matthew Brost wrote: > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Unblock GuC submission on Gen11+ platforms. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Matthew Brost <matthew.brost@intel.com> Updating debug message per feedback, with that: Reviewed-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 + > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++++++ > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 3 +-- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +++++++++----- > 4 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index fae01dc8e1b9..77981788204f 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -54,6 +54,7 @@ struct intel_guc { > struct ida guc_ids; > struct list_head guc_id_list; > > + bool submission_supported; > bool submission_selected; > > struct i915_vma *ads_vma; > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a427336ce916..405339202280 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct intel_guc *guc) > /* Note: By the time we're here, GuC may have already been reset */ > } > > +static bool __guc_submission_supported(struct intel_guc *guc) > +{ > + /* GuC submission is unavailable for pre-Gen11 */ > + return intel_guc_is_supported(guc) && > + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; > +} > + > static bool __guc_submission_selected(struct intel_guc *guc) > { > struct drm_i915_private *i915 = guc_to_gt(guc)->i915; > @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct intel_guc *guc) > > void intel_guc_submission_init_early(struct intel_guc *guc) > { > + guc->submission_supported = __guc_submission_supported(guc); > guc->submission_selected = __guc_submission_selected(guc); > } > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > index a2a3fad72be1..be767eb6ff71 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h > @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc, > > static inline bool intel_guc_submission_is_supported(struct intel_guc *guc) > { > - /* XXX: GuC submission is unavailable for now */ > - return false; > + return guc->submission_supported; > } > > static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc) > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 7a69c3c027e9..61be0aa81492 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct intel_uc *uc) > return; > } > > - /* Default: enable HuC authentication only */ > - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > + /* Intermediate platforms are HuC authentication only */ > + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { > + drm_dbg(&i915->drm, "Disabling GuC only due to old platform\n"); > + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; > + return; > + } > + > + /* Default: enable HuC authentication and GuC submission */ > + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | ENABLE_GUC_SUBMISSION; > } > > /* Reset GuC providing us with fresh state for both GuC and HuC. > @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc) > if (i915_inject_probe_failure(uc_to_gt(uc)->i915)) > return -ENOMEM; > > - /* XXX: GuC submission is unavailable for now */ > - GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); > - > ret = intel_guc_init(guc); > if (ret) > return ret; > -- > 2.28.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index fae01dc8e1b9..77981788204f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -54,6 +54,7 @@ struct intel_guc { struct ida guc_ids; struct list_head guc_id_list; + bool submission_supported; bool submission_selected; struct i915_vma *ads_vma; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a427336ce916..405339202280 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2042,6 +2042,13 @@ void intel_guc_submission_disable(struct intel_guc *guc) /* Note: By the time we're here, GuC may have already been reset */ } +static bool __guc_submission_supported(struct intel_guc *guc) +{ + /* GuC submission is unavailable for pre-Gen11 */ + return intel_guc_is_supported(guc) && + INTEL_GEN(guc_to_gt(guc)->i915) >= 11; +} + static bool __guc_submission_selected(struct intel_guc *guc) { struct drm_i915_private *i915 = guc_to_gt(guc)->i915; @@ -2054,6 +2061,7 @@ static bool __guc_submission_selected(struct intel_guc *guc) void intel_guc_submission_init_early(struct intel_guc *guc) { + guc->submission_supported = __guc_submission_supported(guc); guc->submission_selected = __guc_submission_selected(guc); } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h index a2a3fad72be1..be767eb6ff71 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h @@ -37,8 +37,7 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc, static inline bool intel_guc_submission_is_supported(struct intel_guc *guc) { - /* XXX: GuC submission is unavailable for now */ - return false; + return guc->submission_supported; } static inline bool intel_guc_submission_is_wanted(struct intel_guc *guc) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 7a69c3c027e9..61be0aa81492 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -34,8 +34,15 @@ static void uc_expand_default_options(struct intel_uc *uc) return; } - /* Default: enable HuC authentication only */ - i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; + /* Intermediate platforms are HuC authentication only */ + if (IS_DG1(i915) || IS_ALDERLAKE_S(i915)) { + drm_dbg(&i915->drm, "Disabling GuC only due to old platform\n"); + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC; + return; + } + + /* Default: enable HuC authentication and GuC submission */ + i915->params.enable_guc = ENABLE_GUC_LOAD_HUC | ENABLE_GUC_SUBMISSION; } /* Reset GuC providing us with fresh state for both GuC and HuC. @@ -313,9 +320,6 @@ static int __uc_init(struct intel_uc *uc) if (i915_inject_probe_failure(uc_to_gt(uc)->i915)) return -ENOMEM; - /* XXX: GuC submission is unavailable for now */ - GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); - ret = intel_guc_init(guc); if (ret) return ret;