[1/1] drm/i915/guc: Sanitory checks for platform that dont have GuC
diff mbox

Message ID 1476488825-5673-1-git-send-email-anusha.srivatsa@intel.com
State New
Headers show

Commit Message

Srivatsa, Anusha Oct. 14, 2016, 11:47 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)
v4: Added the blank line(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, 11 insertions(+), 5 deletions(-)

Comments

Saarinen, Jani Oct. 17, 2016, 6:03 a.m. UTC | #1
> == Series Details ==

> 

> Series: series starting with [1/1] drm/i915/guc: Sanitory checks for platform

> that dont have GuC

> URL   : https://patchwork.freedesktop.org/series/13815/

> State : warning

> 

> == Summary ==

> 

> Series 13815v1 Series without cover letter

> https://patchwork.freedesktop.org/api/1.0/series/13815/revisions/1/mbox/

> 

> Test drv_module_reload_basic:

>                 pass       -> SKIP       (fi-skl-6770hq)

Known unstable still. 

> Test kms_flip:

>         Subgroup basic-flip-vs-wf_vblank:

>                 dmesg-warn -> PASS       (fi-skl-6770hq)

>                 dmesg-warn -> PASS       (fi-skl-6700k)

>         Subgroup basic-plain-flip:

>                 pass       -> DMESG-WARN (fi-ilk-650)

dmesg	
[  395.814348] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun
https://bugs.freedesktop.org/show_bug.cgi?id=98251

[  395.814464] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun

> Test kms_pipe_crc_basic:

>         Subgroup read-crc-pipe-a-frame-sequence:

>                 dmesg-warn -> PASS       (fi-ilk-650)

>         Subgroup read-crc-pipe-b:

>                 dmesg-warn -> PASS       (fi-ilk-650)

>         Subgroup suspend-read-crc-pipe-c:

>                 incomplete -> PASS       (fi-skl-6700k)

> Test vgem_basic:

>         Subgroup unload:

>                 pass       -> SKIP       (fi-hsw-4770)

>                 skip       -> PASS       (fi-skl-6770hq)

Still unstable.

> 

> fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15

> fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42

> fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30

> fi-byt-j1900     total:246  pass:213  dwarn:1   dfail:0   fail:1   skip:31

> fi-byt-n2820     total:246  pass:210  dwarn:0   dfail:0   fail:1   skip:35

> fi-hsw-4770      total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23

> fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22

> fi-ilk-650       total:246  pass:183  dwarn:1   dfail:0   fail:2   skip:60

> fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25

> fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25

> fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24

> fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14

> fi-skl-6700hq    total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23

> fi-skl-6700k     total:246  pass:221  dwarn:1   dfail:0   fail:0   skip:24

> fi-skl-6770hq    total:246  pass:229  dwarn:1   dfail:0   fail:1   skip:15

> fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36

> fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37

> 

> Results at /archive/results/CI_IGT_test/Patchwork_2730/

> 

> 38ecbe9082e477953fe165265c76e6c6061aeaf6 drm-intel-nightly: 2016y-10m-

> 14d-16h-23m-48s UTC integration manifest

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

> 


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Paulo Zanoni Oct. 18, 2016, 2:09 p.m. UTC | #2
Em Sex, 2016-10-14 às 16:47 -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)
> v4: Added the blank line(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, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 678b51a..94c3ad9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -719,11 +719,17 @@ 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);
> +	}
> +

Previously you were removing the blank line at this spot, leaving the
code without any empty lines between the two "if" statements. Now in
this version, instead of just not removing the blank line, you added an
extra one, so there are two blank lines between the two "if"
statements. Again, this is not the usual coding style.

Anyway, I amended your patch, added a R-B tag and merged it. Thanks for
the patch.

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

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 678b51a..94c3ad9 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -719,11 +719,17 @@  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;