diff mbox

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

Message ID 1476314034-5644-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Srivatsa, Anusha Oct. 12, 2016, 11:13 p.m. UTC
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(-)

Comments

Saarinen, Jani Oct. 13, 2016, 6:11 a.m. UTC | #1
> == 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
Zanoni, Paulo R Oct. 13, 2016, 2 p.m. UTC | #2
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)) {
diff mbox

Patch

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