* [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
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, 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; +Cc: Nikula, Jani, intel-gfx, 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; +Cc: Nikula, Jani, intel-gfx
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; +Cc: intel-gfx
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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.