| Message ID | 20180717125320.6046-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ville Syrjala (2018-07-17 13:53:20) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > If there's no guc don't try to initialize it even if the user asked for > it. > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I like it for less debug spam, so suits me (I might even wish for greater reductions in noise about unused fw...) Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Tue, 17 Jul 2018 14:53:20 +0200, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > If there's no guc don't try to initialize it even if the user asked for > it. > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 7c95697e1a35..2765808b01e0 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -106,6 +106,11 @@ static void sanitize_options_early(struct > drm_i915_private *i915) > struct intel_uc_fw *guc_fw = &i915->guc.fw; > struct intel_uc_fw *huc_fw = &i915->huc.fw; > + if (!HAS_GUC(i915)) { > + i915_modparams.enable_guc = 0; > + return; > + } > + This will silently switch from user requested GuC-submission to execlist-mode which we wanted to stop. If user don't know what is available on given platform then auto(-1) mode should be used instead. If user has decided to explicitly specify invalid enable_guc !0 mode on non-GuC platform why do we want to ignore that and continue as nothing happened? Michal ps. what is your expectation if there is GuC HW but no FW was defined? > /* A negative value means "use platform default" */ > if (i915_modparams.enable_guc < 0) > i915_modparams.enable_guc = __get_platform_enable_guc(i915);
On Tue, Jul 17, 2018 at 03:26:18PM +0200, Michal Wajdeczko wrote: > On Tue, 17 Jul 2018 14:53:20 +0200, Ville Syrjala > <ville.syrjala@linux.intel.com> wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > If there's no guc don't try to initialize it even if the user asked for > > it. > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_uc.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c > > b/drivers/gpu/drm/i915/intel_uc.c > > index 7c95697e1a35..2765808b01e0 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -106,6 +106,11 @@ static void sanitize_options_early(struct > > drm_i915_private *i915) > > struct intel_uc_fw *guc_fw = &i915->guc.fw; > > struct intel_uc_fw *huc_fw = &i915->huc.fw; > > + if (!HAS_GUC(i915)) { > > + i915_modparams.enable_guc = 0; > > + return; > > + } > > + > > This will silently switch from user requested GuC-submission to > execlist-mode which we wanted to stop. > > If user don't know what is available on given platform then auto(-1) > mode should be used instead. If user has decided to explicitly specify > invalid enable_guc !0 mode on non-GuC platform why do we want to ignore > that and continue as nothing happened? If we want to fail then we should at least fail nicer and tell the user they're trying something that's not possible. > > Michal > > ps. what is your expectation if there is GuC HW but no FW was defined? I just bury my head in the sand whenever a guc approaches. > > > /* A negative value means "use platform default" */ > > if (i915_modparams.enable_guc < 0) > > i915_modparams.enable_guc = __get_platform_enable_guc(i915);
On Tue, 17 Jul 2018 16:22:01 +0200, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Jul 17, 2018 at 03:26:18PM +0200, Michal Wajdeczko wrote: >> On Tue, 17 Jul 2018 14:53:20 +0200, Ville Syrjala >> <ville.syrjala@linux.intel.com> wrote: >> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > If there's no guc don't try to initialize it even if the user asked >> for >> > it. >> > >> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_uc.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_uc.c >> > b/drivers/gpu/drm/i915/intel_uc.c >> > index 7c95697e1a35..2765808b01e0 100644 >> > --- a/drivers/gpu/drm/i915/intel_uc.c >> > +++ b/drivers/gpu/drm/i915/intel_uc.c >> > @@ -106,6 +106,11 @@ static void sanitize_options_early(struct >> > drm_i915_private *i915) >> > struct intel_uc_fw *guc_fw = &i915->guc.fw; >> > struct intel_uc_fw *huc_fw = &i915->huc.fw; >> > + if (!HAS_GUC(i915)) { >> > + i915_modparams.enable_guc = 0; >> > + return; >> > + } >> > + >> >> This will silently switch from user requested GuC-submission to >> execlist-mode which we wanted to stop. >> >> If user don't know what is available on given platform then auto(-1) >> mode should be used instead. If user has decided to explicitly specify >> invalid enable_guc !0 mode on non-GuC platform why do we want to ignore >> that and continue as nothing happened? > > If we want to fail then we should at least fail nicer and tell the user > they're trying something that's not possible. > One should see this warning message: DRM_WARN("Incompatible option detected: %s=%d, %s!\n", "enable_guc", i915_modparams.enable_guc, !HAS_GUC(i915) ? "no GuC hardware" : "no GuC firmware"); and after we finally fix -EIO support it will be followed by: "Failed to initialize GPU, declaring it wedged!\n" Michal
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 7c95697e1a35..2765808b01e0 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -106,6 +106,11 @@ static void sanitize_options_early(struct drm_i915_private *i915) struct intel_uc_fw *guc_fw = &i915->guc.fw; struct intel_uc_fw *huc_fw = &i915->huc.fw; + if (!HAS_GUC(i915)) { + i915_modparams.enable_guc = 0; + return; + } + /* A negative value means "use platform default" */ if (i915_modparams.enable_guc < 0) i915_modparams.enable_guc = __get_platform_enable_guc(i915);