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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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