Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
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 > Cc: Anusha Srivatsa > Cc: Christophe Prigent > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > Signed-off-by: Rodrigo Vivi > --- > 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
Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
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 > > Cc: Anusha Srivatsa > > Cc: Christophe Prigent > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > Signed-off-by: Rodrigo Vivi > > --- > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
On Thu, Sep 22, 2016 at 04:55:07PM +, 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 > > > Cc: Anusha Srivatsa > > > Cc: Christophe Prigent > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > Signed-off-by: Rodrigo Vivi > > > --- > > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
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 +, 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 > > > > Cc: Anusha Srivatsa > > > > Cc: Christophe Prigent > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > > Signed-off-by: Rodrigo Vivi > > > > --- > > > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
Em Qua, 2016-10-05 às 23:37 +, 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 +, 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 > > > > > Cc: Anusha Srivatsa > > > > > Cc: Christophe Prigent > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > > > Signed-off-by: Rodrigo Vivi > > > > > --- > > > > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
On Thu, 06 Oct 2016, Paulo Zanoni wrote: > Em Qua, 2016-10-05 às 23:37 +, 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 +, 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 >> > > > > Cc: Anusha Srivatsa >> > > > > Cc: Christophe Prigent >> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 >> > > > > Signed-off-by: Rodrigo Vivi >> > > > > --- >> > > > > 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 ma
Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.
On Thu, 2016-10-06 at 09:36 -0300, Paulo Zanoni wrote: > Em Qua, 2016-10-05 às 23:37 +, 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 +, 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 > > > > > > Cc: Anusha Srivatsa > > > > > > Cc: Christophe Prigent > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573 > > > > > > Signed-off-by: Rodrigo Vivi > > > > > > --- > > > > > > 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); > > > > > > > > __