On Thu, Sep 22, 2016 at 04:55:07PM +0000, Vivi, Rodrigo wrote: > 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. Not sure it needs to be smarter than that really, since all these debug options auto-taint the kernel if you touch them. As in: You get to keep all the pieces. We can still do some auto-cleanup of modoptions ofc if there's a good need for them. -Daniel > > > > > > > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
