* [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. @ 2016-09-21 18:22 Rodrigo Vivi 2016-09-21 18:49 ` ✓ Fi.CI.BAT: success for " Patchwork 2016-09-21 21:00 ` [PATCH] " Paulo Zanoni 0 siblings, 2 replies; 9+ messages in thread From: Rodrigo Vivi @ 2016-09-21 18:22 UTC (permalink / raw) To: intel-gfx; +Cc: Jani Nikula, Rodrigo Vivi 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. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Christophe Prigent <christophe.prigent@intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- 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; + } + /* A negative value means "use platform default" */ if (i915.enable_guc_loading < 0) i915.enable_guc_loading = HAS_GUC_UCODE(dev); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Don't try to handle GuC when GuC is not supported. 2016-09-21 18:22 [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported Rodrigo Vivi @ 2016-09-21 18:49 ` Patchwork 2016-09-21 21:00 ` [PATCH] " Paulo Zanoni 1 sibling, 0 replies; 9+ messages in thread From: Patchwork @ 2016-09-21 18:49 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx == Series Details == Series: drm/i915: Don't try to handle GuC when GuC is not supported. URL : https://patchwork.freedesktop.org/series/12753/ State : success == Summary == Series 12753v1 drm/i915: Don't try to handle GuC when GuC is not supported. https://patchwork.freedesktop.org/api/1.0/series/12753/revisions/1/mbox/ Test kms_pipe_crc_basic: Subgroup read-crc-pipe-b: dmesg-warn -> PASS (fi-skl-6770hq) Test kms_psr_sink_crc: Subgroup psr_basic: dmesg-warn -> PASS (fi-skl-6700hq) fi-bdw-5557u total:244 pass:229 dwarn:0 dfail:0 fail:0 skip:15 fi-bsw-n3050 total:244 pass:202 dwarn:0 dfail:0 fail:0 skip:42 fi-byt-n2820 total:244 pass:208 dwarn:0 dfail:0 fail:1 skip:35 fi-hsw-4770k total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22 fi-hsw-4770r total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22 fi-ilk-650 total:244 pass:182 dwarn:0 dfail:0 fail:2 skip:60 fi-ivb-3520m total:244 pass:219 dwarn:0 dfail:0 fail:0 skip:25 fi-ivb-3770 total:244 pass:207 dwarn:0 dfail:0 fail:0 skip:37 fi-skl-6260u total:244 pass:230 dwarn:0 dfail:0 fail:0 skip:14 fi-skl-6700hq total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22 fi-skl-6700k total:244 pass:219 dwarn:1 dfail:0 fail:0 skip:24 fi-skl-6770hq total:244 pass:228 dwarn:1 dfail:0 fail:1 skip:14 fi-snb-2520m total:244 pass:208 dwarn:0 dfail:0 fail:0 skip:36 fi-snb-2600 total:244 pass:207 dwarn:0 dfail:0 fail:0 skip:37 Results at /archive/results/CI_IGT_test/Patchwork_2565/ 463d07a32d87742a73e1ed352a6d6daa3f29d0c2 drm-intel-nightly: 2016y-09m-21d-16h-35m-54s UTC integration manifest 00e8796 drm/i915: Don't try to handle GuC when GuC is not supported. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-09-21 18:22 [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported Rodrigo Vivi 2016-09-21 18:49 ` ✓ Fi.CI.BAT: success for " Patchwork @ 2016-09-21 21:00 ` Paulo Zanoni 2016-09-22 16:55 ` Vivi, Rodrigo 1 sibling, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2016-09-21 21:00 UTC (permalink / raw) To: Rodrigo Vivi, intel-gfx; +Cc: Jani Nikula 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? > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Christophe Prigent <christophe.prigent@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > 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); } Or we could even go with our current "design pattern" and create intel_sanitize_guc_options(). 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-09-21 21:00 ` [PATCH] " Paulo Zanoni @ 2016-09-22 16:55 ` Vivi, Rodrigo 2016-10-05 13:50 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Vivi, Rodrigo @ 2016-09-22 16:55 UTC (permalink / raw) To: Zanoni, Paulo R; +Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org 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@intel.com> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Christophe Prigent <christophe.prigent@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-09-22 16:55 ` Vivi, Rodrigo @ 2016-10-05 13:50 ` Daniel Vetter 2016-10-05 23:37 ` Vivi, Rodrigo 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2016-10-05 13:50 UTC (permalink / raw) To: Vivi, Rodrigo Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Zanoni, Paulo R 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@intel.com> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > Cc: Christophe Prigent <christophe.prigent@intel.com> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-10-05 13:50 ` Daniel Vetter @ 2016-10-05 23:37 ` Vivi, Rodrigo 2016-10-06 12:36 ` Paulo Zanoni 0 siblings, 1 reply; 9+ messages in thread From: Vivi, Rodrigo @ 2016-10-05 23:37 UTC (permalink / raw) To: daniel@ffwll.ch Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Zanoni, Paulo R 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. 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@intel.com> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > Cc: Christophe Prigent <christophe.prigent@intel.com> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-10-05 23:37 ` Vivi, Rodrigo @ 2016-10-06 12:36 ` Paulo Zanoni 2016-10-06 13:31 ` Jani Nikula 2016-10-06 20:56 ` Vivi, Rodrigo 0 siblings, 2 replies; 9+ messages in thread From: Paulo Zanoni @ 2016-10-06 12:36 UTC (permalink / raw) To: Vivi, Rodrigo, daniel@ffwll.ch Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org 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. > > 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@intel.com> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > Cc: Christophe Prigent <christophe.prigent@intel.com> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > --- > > > > > 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-10-06 12:36 ` Paulo Zanoni @ 2016-10-06 13:31 ` Jani Nikula 2016-10-06 20:56 ` Vivi, Rodrigo 1 sibling, 0 replies; 9+ messages in thread From: Jani Nikula @ 2016-10-06 13:31 UTC (permalink / raw) To: Paulo Zanoni, Vivi, Rodrigo, daniel@ffwll.ch Cc: intel-gfx@lists.freedesktop.org On Thu, 06 Oct 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> 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@intel.com> >> > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> > > > > Cc: Christophe Prigent <christophe.prigent@intel.com> >> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 >> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> > > > > --- >> > > > > 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@lists.freedesktop.org >> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > >> -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported. 2016-10-06 12:36 ` Paulo Zanoni 2016-10-06 13:31 ` Jani Nikula @ 2016-10-06 20:56 ` Vivi, Rodrigo 1 sibling, 0 replies; 9+ messages in thread From: Vivi, Rodrigo @ 2016-10-06 20:56 UTC (permalink / raw) To: Zanoni, Paulo R; +Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org 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@intel.com> > > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > > > Cc: Christophe Prigent <christophe.prigent@intel.com> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > --- > > > > > > 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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-10-06 20:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-21 18:22 [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported Rodrigo Vivi 2016-09-21 18:49 ` ✓ Fi.CI.BAT: success for " Patchwork 2016-09-21 21:00 ` [PATCH] " Paulo Zanoni 2016-09-22 16:55 ` Vivi, Rodrigo 2016-10-05 13:50 ` Daniel Vetter 2016-10-05 23:37 ` Vivi, Rodrigo 2016-10-06 12:36 ` Paulo Zanoni 2016-10-06 13:31 ` Jani Nikula 2016-10-06 20:56 ` Vivi, Rodrigo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).