Re: [PATCH] drm/i915/guc: Sanitory checks for platform that dont have GuC

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

Em Ter, 2016-10-11 às 10:36 +0300, Jani Nikula escreveu:
> On Tue, 11 Oct 2016, Anusha Srivatsa <anusha.srivatsa@intel.com>
> wrote:
> > 
> > i915.enable_guc_loading/submission=2 forces the usage of GuC.
> > For platforms that do not have a GuC, asking the kernel to
> > use a GuC should not result in an error state. Do extra checks
> > to see if the platform even has a GuC or not, regardless of the
> > kernel parameter.
> > 
> > Based on Rodriogo's <vivi.rodrigo@intel.com> patch.
> > Have considered Paulo Zanoni's<paulo.r.zanoni@intel.com>
> > suggestions on the implementation.

The correct way to give credit to reviewers is by adding a patch
version changelog, and later including any reviewed-by tags that the
reviewer may give. No need for a sentence like the one above. Just take
a small look at the git log and see how people do the changelogs. Yours
would be something like:

v2: change indentation, add back blank lines (Paulo)

(but I do have to add that the way you changed it was not the way I had
in mind, please see below)

> 
> There's a bug for this, please find it and reference it.
> 
> > 
> > take on the implemenation.
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> > b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 7ace96b..98718db 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -718,12 +718,17 @@ 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;
> > +        } else {
> >  	/* A negative value means "use platform default" */
> >  	if (i915.enable_guc_loading < 0)
> >  		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> >  	if (i915.enable_guc_submission < 0)
> >  		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
> >  
> > +	}

The blank line is usually inserted _after_ the line containing the
closing bracket char. Please see the many examples in the rest of our
code.


> 
> Indentation?

I had already complained about indentation before. It's different now,
but it's still not exactly what I was expecting... The idea was indeed
to align the comment indentation with the rest of the "else" case, but
not the way it was done here. Please see the many examples in the rest
of our code.......



> 
> BR,
> Jani.
> 
> > 
> >  	if (!HAS_GUC_UCODE(dev)) {
> >  		fw_path = NULL;
> >  	} else if (IS_SKYLAKE(dev)) {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx