[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.

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test drv_module_reload_basic:
                skip       -> PASS       (fi-skl-6770hq)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (fi-skl-6770hq)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-ilk-650)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> FAIL       (fi-ivb-3520m)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> FAIL       (fi-ivb-3520m)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> FAIL       (fi-ivb-3520m)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq)
Test vgem_basic:
        Subgroup unload:
                skip       -> PASS       (fi-kbl-7200u)
                pass       -> SKIP       (fi-byt-n2820)
                skip       -> PASS       (fi-hsw-4770)
Test vgem_reload_basic:
                pass       -> FAIL       (fi-byt-n2820)

fi-bdw-5557u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-bsw-n3050     total:248  pass:205  dwarn:0   dfail:0   fail:0   skip:43 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:213  dwarn:2   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:209  dwarn:0   dfail:0   fail:2   skip:37 
fi-hsw-4770      total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:248  pass:183  dwarn:2   dfail:0   fail:2   skip:61 
fi-ivb-3520m     total:248  pass:219  dwarn:0   dfail:0   fail:3   skip:26 
fi-ivb-3770      total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:248  pass:223  dwarn:0   dfail:0   fail:0   skip:25 
fi-skl-6260u     total:248  pass:233  dwarn:0   dfail:0   fail:0   skip:15 
fi-skl-6700hq    total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:248  pass:222  dwarn:1   dfail:0   fail:0   skip:25 
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:210  dwarn:0   dfail:0   fail:0   skip:38 

Results at /archive/results/CI_IGT_test/Patchwork_2695/

14740bb25ec36fe4ce8042af3eb48aeb45e5bc13 drm-intel-nightly: 2016y-10m-12d-16h-18m-24s UTC integration manifest
babaf2a 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: failure for drm/i915/guc: Sanitory checks for platform that dont have GuC (rev3)

[Saarinen, Jani]

> == Series Details ==
> 
> Series: drm/i915/guc: Sanitory checks for platform that dont have GuC (rev3)
> URL   : https://patchwork.freedesktop.org/series/13358/
> State : failure
> 
> == Summary ==
> 
> Series 13358v3 drm/i915/guc: Sanitory checks for platform that dont have
> GuC
> https://patchwork.freedesktop.org/api/1.0/series/13358/revisions/3/mbox/
> 
> Test drv_module_reload_basic:
>                 skip       -> PASS       (fi-skl-6770hq)
> Test kms_flip:
>         Subgroup basic-flip-vs-modeset:
>                 dmesg-warn -> PASS       (fi-skl-6770hq)
> Test kms_pipe_crc_basic:
>         Subgroup read-crc-pipe-b-frame-sequence:
>                 pass       -> DMESG-WARN (fi-ilk-650)
	
 [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun
 [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun

>         Subgroup suspend-read-crc-pipe-a:
>                 pass       -> DMESG-WARN (fi-ilk-650)
 [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun
 [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun

>                 pass       -> FAIL       (fi-ivb-3520m)
	
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Oct 12 23:44:55 2016
IGT-Version: 1.16-gdcab159 
Stack trace:
  #0 [__igt_fail_assert+0x101]
  #1 [igt_system_suspend_autoresume+0x9f]
  #2 [__real_main183+0x30f]
  #3 [main+0x23]
  #4 [__libc_start_main+0xf0]
  #5 [_start+0x29]
Subtest suspend-read-crc-pipe-A: FAIL (20.295s)

Stderr	
rtcwake: write error
(kms_pipe_crc_basic:11232) igt-aux-CRITICAL: Test assertion failure function igt_system_suspend_autoresume, file igt_aux.c:651:
(kms_pipe_crc_basic:11232) igt-aux-CRITICAL: Failed assertion: system("rtcwake -s 15 -m mem") == 0
(kms_pipe_crc_basic:11232) igt-aux-CRITICAL: This failure means that something is wrong with the rtcwake tool or how your distro is set up. This is not a i915.ko or i-g-t bug.

>         Subgroup suspend-read-crc-pipe-b:
>                 pass       -> FAIL       (fi-ivb-3520m)
Stdout	
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Oct 12 23:45:16 2016
IGT-Version: 1.16-gdcab159 
Stack trace:
  #0 [__igt_fail_assert+0x101]
  #1 [igt_system_suspend_autoresume+0x9f]
  #2 [__real_main183+0x30f]
  #3 [main+0x23]
  #4 [__libc_start_main+0xf0]
  #5 [_start+0x29]
Subtest suspend-read-crc-pipe-B: FAIL (20.263s)

rtcwake: write error
(kms_pipe_crc_basic:11241) igt-aux-CRITICAL: Test assertion failure function igt_system_suspend_autoresume, file igt_aux.c:651:
(kms_pipe_crc_basic:11241) igt-aux-CRITICAL: Failed assertion: system("rtcwake -s 15 -m mem") == 0
(kms_pipe_crc_basic:11241) igt-aux-CRITICAL: This failure means that something is wrong with the rtcwake tool or how your distro is set up. This is not a i915.ko or i-g-t bug.

>         Subgroup suspend-read-crc-pipe-c:
>                 pass       -> FAIL       (fi-ivb-3520m)
Stdout
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Oct 12 23:45:37 2016
IGT-Version: 1.16-gdcab159
Stack trace:
  #0 [__igt_fail_assert+0x101]
  #1 [igt_system_suspend_autoresume+0x9f]
  #2 [__real_main183+0x30f]
  #3 [main+0x23]
  #4 [__libc_start_main+0xf0]
  #5 [_start+0x29]
Subtest suspend-read-crc-pipe-C: FAIL (20.218s)

	
rtcwake: write error
(kms_pipe_crc_basic:11252) igt-aux-CRITICAL: Test assertion failure function igt_system_suspend_autoresume, file igt_aux.c:651:
(kms_pipe_crc_basic:11252) igt-aux-CRITICAL: Failed assertion: system("rtcwake -s 15 -m mem") == 0
(kms_pipe_crc_basic:11252) igt-aux-CRITICAL: This failure means that something is wrong with the rtcwake tool or how your distro is set up. This is not a i915.ko or i-g-t bug.
Subtest suspend-read-crc-pipe-C failed.

> Test kms_psr_sink_crc:
>         Subgroup psr_basic:
>                 dmesg-warn -> PASS       (fi-skl-6700hq)
> Test vgem_basic:
>         Subgroup unload:
>                 skip       -> PASS       (fi-kbl-7200u)
>                 pass       -> SKIP       (fi-byt-n2820)
>                 skip       -> PASS       (fi-hsw-4770)
> Test vgem_reload_basic:
>                 pass       -> FAIL       (fi-byt-n2820)
> 
> fi-bdw-5557u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16
> fi-bsw-n3050     total:248  pass:205  dwarn:0   dfail:0   fail:0   skip:43
> fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31
> fi-byt-j1900     total:248  pass:213  dwarn:2   dfail:0   fail:1   skip:32
> fi-byt-n2820     total:248  pass:209  dwarn:0   dfail:0   fail:2   skip:37
> fi-hsw-4770      total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23
> fi-hsw-4770r     total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23
> fi-ilk-650       total:248  pass:183  dwarn:2   dfail:0   fail:2   skip:61
> fi-ivb-3520m     total:248  pass:219  dwarn:0   dfail:0   fail:3   skip:26
> fi-ivb-3770      total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26
> fi-kbl-7200u     total:248  pass:223  dwarn:0   dfail:0   fail:0   skip:25
> fi-skl-6260u     total:248  pass:233  dwarn:0   dfail:0   fail:0   skip:15
> fi-skl-6700hq    total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23
> fi-skl-6700k     total:248  pass:222  dwarn:1   dfail:0   fail:0   skip:25
> 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:210  dwarn:0   dfail:0   fail:0   skip:38
> 
> Results at /archive/results/CI_IGT_test/Patchwork_2695/
> 
> 14740bb25ec36fe4ce8042af3eb48aeb45e5bc13 drm-intel-nightly: 2016y-10m-
> 12d-16h-18m-24s UTC integration manifest babaf2a drm/i915/guc: Sanitory
> checks for platform that dont have GuC
> 

Jani Saarinen
_______________________________________________
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

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

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

[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

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

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

[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

[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