On Wed, 2016-09-21 at 18:00 -0300, Paulo Zanoni wrote: > Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu: > > Avoid any kind of GuC handling if GuC is not supported > > on a giving platform. > > > > Besides being useless handling, our driver needs > > to be smarter than the user trying to use an invalid paramenter. > > So the problem is when a platform doesn't support guc and the user > passes i915.enable_guc_something=1, right? 1 is not a problem actually since it means "use if available". There is not firmware and execution continues. 2 is the problem because it means "use guc or fail if not available". But platforms that don't have guc can't fail. driver needs to be smarter than that. > > > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> > > Cc: Christophe Prigent <christophe.prigent@xxxxxxxxx> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > > b/drivers/gpu/drm/i915/intel_guc_loader.c > > index 6fd39ef..da0f5ed 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device *dev) > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > const char *fw_path; > > > > + if (!HAS_GUC(dev)) { > > + i915.enable_guc_loading = 0; > > + i915.enable_guc_submission = 0; > > + fw_path = NULL; > > + return; > > + } > > Instead of this, how about we just patch the code below with: > > if (!HAS_GUC(dev_priv)) { > i915.enable_guc_loading = 0; > i915.enable_guc_submission = 0; > } else { > /* A negative value means "use platform default" */ > if (i915.enable_guc_loading < 0) > i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv); > if (i915.enable_guc_submission < 0) > i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv); > } yeap, this works as well. I just went for the simplest option that minimized at most any interactions for platforms where GuC simply doesn't exist. > > Or we could even go with our current "design pattern" and create > intel_sanitize_guc_options(). This is indeed a very good idea. > > This way we'll be able to avoid adding a second failure code path, > since we already have one for platforms with guc but options disabled. > > > > + > > /* A negative value means "use platform default" */ > > if (i915.enable_guc_loading < 0) > > i915.enable_guc_loading = HAS_GUC_UCODE(dev); _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
