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

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

[Anusha Srivatsa]

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.

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);
 
+	}
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev)) {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915/guc: Sanitory checks for platform that dont have GuC (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Sanitory checks for platform that dont have GuC (rev2)
URL   : https://patchwork.freedesktop.org/series/13358/
State : warning

== Summary ==

Series 13358v2 drm/i915/guc: Sanitory checks for platform that dont have GuC
https://patchwork.freedesktop.org/api/1.0/series/13358/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-3:
                pass       -> DMESG-WARN (fi-ilk-650)

fi-bdw-5557u     total:248  pass:231  dwarn:0   dfail:0   fail:0   skip:17 
fi-bsw-n3050     total:248  pass:204  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:214  dwarn:1   dfail:0   fail:1   skip:32 
fi-hsw-4770      total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-hsw-4770r     total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-ilk-650       total:248  pass:184  dwarn:1   dfail:0   fail:2   skip:61 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:207  dwarn:0   dfail:0   fail:0   skip:41 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:221  dwarn:1   dfail:0   fail:0   skip:26 
fi-skl-6770hq    total:248  pass:231  dwarn:1   dfail:0   fail:1   skip:15 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:209  dwarn:0   dfail:0   fail:0   skip:39 

Results at /archive/results/CI_IGT_test/Patchwork_2668/

e37a15c8d775e79dddc8345a0f6afdcfe1f607d9 drm-intel-nightly: 2016y-10m-10d-14h-33m-29s UTC integration manifest
79c7ff7 drm/i915/guc: Sanitory checks for platform that dont have GuC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915/guc: Sanitory checks for platform that dont have GuC (rev2)

[Saarinen, Jani]

> == Summary ==
> 
> Series 13358v2 drm/i915/guc: Sanitory checks for platform that dont have
> GuC
> https://patchwork.freedesktop.org/api/1.0/series/13358/revisions/2/mbox/
> 
> Test kms_pipe_crc_basic:
>         Subgroup bad-nb-words-3:
>                 pass       -> DMESG-WARN (fi-ilk-650)
Stdout	
IGT-Version: 1.16-g48a9e1e (x86_64) (Linux: 4.8.0-CI-Patchwork_2668+ x86_64)
Subtest bad-nb-words-3: SUCCESS (0.000s)

Stderr	

Environment	
PIGLIT_SOURCE_DIR="/opt/igt/piglit" PIGLIT_PLATFORM="mixed_glx_egl"

Command	/opt/igt/tests/kms_pipe_crc_basic --run-subtest bad-nb-words-3

dmesg	
[  401.433161] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun
[  401.433188] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun


> fi-ilk-650       total:248  pass:184  dwarn:1   dfail:0   fail:2   skip:61
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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.

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);
>  
> +	}

Indentation?

BR,
Jani.

>  	if (!HAS_GUC_UCODE(dev)) {
>  		fw_path = NULL;
>  	} else if (IS_SKYLAKE(dev)) {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paulo Zanoni]

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

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

[Anusha Srivatsa]

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.

v2: Based on Rodrigo's patch and Paulo's suggestion(Paulo, Rodrigo)
v3: Correct the Indentation(Jani, Paulo)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Zanoni Paulo <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>

Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 7ace96b..811080f 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -718,12 +718,16 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	/* 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);
-
+	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);
+	}
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev)) {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paulo Zanoni]

Em Qua, 2016-10-12 às 16:13 -0700, Anusha Srivatsa escreveu:
> 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.
> 
> v2: Based on Rodrigo's patch and Paulo's suggestion(Paulo, Rodrigo)
> v3: Correct the Indentation(Jani, Paulo)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Zanoni Paulo <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> 
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 7ace96b..811080f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -718,12 +718,16 @@ void intel_guc_init(struct drm_device *dev)
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path;
>  
> -	/* 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);
> -
> +	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);
> +	}

Well, the indentation is correct now, but this patch is still removing
the blank line that was supposed to be at this spot, despite the fact
that I pointed this problem in my two previous reviews... Please always
pay attention to the reviews and try to make sure you're actually
following them when submit new versions. It's a good idea if you can
learn the patch format and look at the patch files directly in order to
spot problems like this before submitting.

I know this sounds super annoying since I'm just complaining about
blank lines, but since I expect you to start contributing much more, I
think it's worth trying to teach the coding style that's used by
everybody so the next patches all come in the expected format.

Anyway, I suppose we could amend this fix while applying the patch?

If someone fixes that while applying, we can add:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  	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

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

[Anusha Srivatsa]

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.

Cc: Zanoni Paulo <paulo.r.zanoni@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 7ace96b..15d2d53 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(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);
-
+		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);
+	}
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev)) {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paulo Zanoni]

Em Qua, 2016-10-05 às 16:50 -0700, Anusha Srivatsa escreveu:
> 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.

I'm not exactly sure who did what here, but I think Rodrigo deserves
some credit for his initial work (and possibly debug). Maybe add:
Based-on-patch-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> Cc: Zanoni Paulo <paulo.r.zanoni@intel.com>

I see the Cc tag here but no Cc mail header with my email on it, so
this email wasn't colored blue on my intel-gfx folder. Why didn't I
actually get Cc'd here? Did you use git-send-email? This may be worth
investigating since it may happen again in the future, and you want to
make sure people you Cc actually get Cc'd.


> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 7ace96b..15d2d53 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path;
> -

Please do not remove the blank line above.

> +	if (!HAS_GUC(dev)) {
> +		i915.enable_guc_loading = 0;
> +		i915.enable_guc_submission = 0;
> +	} else {
>  	/* A negative value means "use platform default" */

Bad indentation here.

> -	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);
> -

Please also do not remove the blank line above.

Otherwise, looks good.

> +		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);
> +	}
>  	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

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

[Vivi, Rodrigo]

On Fri, 2016-10-07 at 10:41 -0300, Paulo Zanoni wrote:
> Em Qua, 2016-10-05 às 16:50 -0700, Anusha Srivatsa escreveu:
> > 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.
> 
> I'm not exactly sure who did what here, but I think Rodrigo deserves
> some credit for his initial work (and possibly debug). Maybe add:
> Based-on-patch-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I just drop a quickly patch that day, but she did the work, specially
debugging it after.

> 
> 
> > Cc: Zanoni Paulo <paulo.r.zanoni@intel.com>
> 
> I see the Cc tag here but no Cc mail header with my email on it, so
> this email wasn't colored blue on my intel-gfx folder. Why didn't I
> actually get Cc'd here? Did you use git-send-email? This may be worth
> investigating since it may happen again in the future, and you want to
> make sure people you Cc actually get Cc'd.
> 
> 
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> > b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 7ace96b..15d2d53 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	const char *fw_path;
> > -
> 
> Please do not remove the blank line above.
> 
> > +	if (!HAS_GUC(dev)) {
> > +		i915.enable_guc_loading = 0;
> > +		i915.enable_guc_submission = 0;
> > +	} else {
> >  	/* A negative value means "use platform default" */
> 
> Bad indentation here.
> 
> > -	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);
> > -
> 
> Please also do not remove the blank line above.
> 
> Otherwise, looks good.
> 
> > +		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);
> > +	}
> >  	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

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

[Srivatsa, Anusha]

>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Friday, October 7, 2016 7:06 AM
>To: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Srivatsa, Anusha
><anusha.srivatsa@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Sanitory checks for platform that
>dont have GuC
>
>On Fri, 2016-10-07 at 10:41 -0300, Paulo Zanoni wrote:
>> Em Qua, 2016-10-05 às 16:50 -0700, Anusha Srivatsa escreveu:
>> > 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.
>>
>> I'm not exactly sure who did what here, but I think Rodrigo deserves
>> some credit for his initial work (and possibly debug). Maybe add:
>> Based-on-patch-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>I just drop a quickly patch that day, but she did the work, specially debugging it
>after.

I agree with you Paulo. In fact I was thinking of an exact way of giving credit to both you and Rodrigo since the patch is inspired by his patch and I have taken your suggestion as well. Will update the patch with "Based-on". 
>>
>>
>> > Cc: Zanoni Paulo <paulo.r.zanoni@intel.com>
>>
>> I see the Cc tag here but no Cc mail header with my email on it, so
>> this email wasn't colored blue on my intel-gfx folder. Why didn't I
>> actually get Cc'd here? Did you use git-send-email? This may be worth
>> investigating since it may happen again in the future, and you want to
>> make sure people you Cc actually get Cc'd.

I used git send-email. Will investigate it. 
>>
>> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++++++++------
>> >  1 file changed, 9 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> > b/drivers/gpu/drm/i915/intel_guc_loader.c
>> > index 7ace96b..15d2d53 100644
>> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> > @@ -717,13 +717,16 @@ void intel_guc_init(struct drm_device *dev)
>> >  	struct drm_i915_private *dev_priv = to_i915(dev);
>> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>> >  	const char *fw_path;
>> > -
>>
>> Please do not remove the blank line above.
>>
>> > +	if (!HAS_GUC(dev)) {
>> > +		i915.enable_guc_loading = 0;
>> > +		i915.enable_guc_submission = 0;
>> > +	} else {
>> >  	/* A negative value means "use platform default" */
>>
>> Bad indentation here.
>>
>> > -	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);
>> > -
>>
>> Please also do not remove the blank line above.
>>
>> Otherwise, looks good.

Will fix indentation issue. Thank you.
>> > +		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);
>> > +	}
>> >  	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