On Thu, 2016-10-06 at 09:36 -0300, Paulo Zanoni wrote: > Em Qua, 2016-10-05 às 23:37 +0000, Vivi, Rodrigo escreveu: > > Hi Daniel, > > > > So, can we close https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > with > > wontfix or notabug? > > > > I don't have a strong side on that actually, but Jani was against it > > it > > seems. > > Just my opinion: > > Considering that we already identified the problem and the fix is > simple, I really think it's better to just fix it. In fact I thought > you were going to submit V2 and I was planning to do the review. > > Fixing this may help reducing future bug triaging time, because if we > keep the problem unfixed we may get the same bug report again and again > and again. It's easy to say "users get to keep all the pieces of the > broken kernel", but we usually have to triage the problems regardless, > discuss what to do, etc. Here it is: https://patchwork.freedesktop.org/patch/113922/ > > > > > Thanks, > > Rodrigo. > > > > On Wed, 2016-10-05 at 15:50 +0200, Daniel Vetter wrote: > > > > > > 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 > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
