Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.

[Vivi, Rodrigo]

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 = _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)
> > > > > >  

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.

[Jani Nikula]

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 = _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)
>> > > > >  

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.

[Paulo Zanoni]

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 = _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

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.

[Vivi, Rodrigo]

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 = _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.

[Daniel Vetter]

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 = _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.

[Vivi, Rodrigo]

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 = _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.

[Paulo Zanoni]

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 = _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

[Intel-gfx] [PATCH] drm/i915: Don't try to handle GuC when GuC is not supported.

[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 
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 = _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