On Thu, 06 Oct 2016, Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> 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. All, please stop the discussion and just do one or the other. I'll ack either approach. http://dilbert.com/strip/1998-11-10 BR, Jani. > >> >> 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 >> > >> -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
