[PATCH] drm/i915: Add a parameter to disable SAGV

[PATCH] drm/i915: Add a parameter to disable SAGV

[Azhar Shaikh]

SAGV handling is currently broken which can result in system hangs.
Add a parameter to disable SAGV, till the SAGV handling is fixed.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 3 +++
 drivers/gpu/drm/i915/i915_params.h | 3 ++-
 drivers/gpu/drm/i915/intel_pm.c    | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 08108ce5be21..6aede52240b0 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -167,6 +167,9 @@ struct i915_params i915_modparams __read_mostly = {
 i915_param_named_unsafe(enable_dp_mst, bool, 0600,
 	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
 
+i915_param_named_unsafe(disable_sagv, bool, 0600,
+	"Disable SAGV (default: false)");
+
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
 i915_param_named_unsafe(inject_load_failure, uint, 0400,
 	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 430f5f9d0ff4..ff3c7d5dee2d 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -69,7 +69,8 @@
 	param(bool, nuclear_pageflip, false) \
 	param(bool, enable_dp_mst, true) \
 	param(bool, enable_dpcd_backlight, false) \
-	param(bool, enable_gvt, false)
+	param(bool, enable_gvt, false) \
+	param(bool, disable_sagv, false)
 
 #define MEMBER(T, member, ...) T member;
 struct i915_params {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 21dac6ebc202..0b1a6cbf45aa 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3693,7 +3693,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	int level, latency;
 	int sagv_block_time_us;
 
-	if (!intel_has_sagv(dev_priv))
+	if (!intel_has_sagv(dev_priv) || i915_modparams.disable_sagv)
 		return false;
 
 	if (IS_GEN9(dev_priv))
-- 
1.9.1

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

✓ Fi.CI.BAT: success for drm/i915: Add a parameter to disable SAGV

[Patchwork]

== Series Details ==

Series: drm/i915: Add a parameter to disable SAGV
URL   : https://patchwork.freedesktop.org/series/38992/
State : success

== Summary ==

Series 38992v1 drm/i915: Add a parameter to disable SAGV
https://patchwork.freedesktop.org/api/1.0/series/38992/revisions/1/mbox/

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> FAIL       (fi-skl-6700k2) fdo#104108
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:411s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:424s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:377s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:485s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:478s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:484s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:464s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:453s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:391s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:558s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:417s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:281s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:506s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:406s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:412s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:450s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:487s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:492s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:585s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:421s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:499s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:521s
fi-skl-6700k2    total:288  pass:263  dwarn:0   dfail:0   fail:1   skip:24  time:491s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:486s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:404s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:524s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:391s

7c50cf4f0dc70112cd82f4475308299df810f302 drm-tip: 2018y-02m-26d-22h-51m-08s UTC integration manifest
58b2b3f7d935 drm/i915: Add a parameter to disable SAGV

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8166/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add a parameter to disable SAGV

[Rodrigo Vivi]

On Mon, Feb 26, 2018 at 03:45:51PM -0800, Azhar Shaikh wrote:
> SAGV handling is currently broken which can result in system hangs.
> Add a parameter to disable SAGV, till the SAGV handling is fixed.

Not just handling of the limitations we have with SAGV is broken but
also probably some hidden DBUF config issue that is worst when SAGV
is enabled.

But also the handling broken is not a good reason for the parameter
itself. But the good part of this is the ability to debug display
hard hangs issues with SAGV requirements.

I'm in favor of the paramenter. We just need a different justification
here.

> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 3 +++
>  drivers/gpu/drm/i915/i915_params.h | 3 ++-
>  drivers/gpu/drm/i915/intel_pm.c    | 2 +-
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 08108ce5be21..6aede52240b0 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -167,6 +167,9 @@ struct i915_params i915_modparams __read_mostly = {
>  i915_param_named_unsafe(enable_dp_mst, bool, 0600,
>  	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
>  
> +i915_param_named_unsafe(disable_sagv, bool, 0600,
> +	"Disable SAGV (default: false)");
> +

I understand that SAGV is default to enable and enabled by BIOS so we
actually need to disable it.

However the name will cause the same old confusion as disable power well disable....

So I'm in favor of bool i915.enable_sagv defaults to enable
and use i915.enable_sagv=0 when we need to disable it.

>  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
>  i915_param_named_unsafe(inject_load_failure, uint, 0400,
>  	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 430f5f9d0ff4..ff3c7d5dee2d 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -69,7 +69,8 @@
>  	param(bool, nuclear_pageflip, false) \
>  	param(bool, enable_dp_mst, true) \
>  	param(bool, enable_dpcd_backlight, false) \
> -	param(bool, enable_gvt, false)
> +	param(bool, enable_gvt, false) \
> +	param(bool, disable_sagv, false)
>  
>  #define MEMBER(T, member, ...) T member;
>  struct i915_params {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 21dac6ebc202..0b1a6cbf45aa 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3693,7 +3693,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
>  	int level, latency;
>  	int sagv_block_time_us;
>  
> -	if (!intel_has_sagv(dev_priv))
> +	if (!intel_has_sagv(dev_priv) || i915_modparams.disable_sagv)
>  		return false;
>  
>  	if (IS_GEN9(dev_priv))
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Add a parameter to disable SAGV

[Patchwork]

== Series Details ==

Series: drm/i915: Add a parameter to disable SAGV
URL   : https://patchwork.freedesktop.org/series/38992/
State : success

== Summary ==

---- Possible new issues:

Test kms_atomic_transition:
        Subgroup plane-all-modeset-transition-fencing:
                pass       -> DMESG-WARN (shard-apl)
Test kms_cursor_legacy:
        Subgroup cursora-vs-flipa-atomic:
                pass       -> DMESG-WARN (shard-apl)
        Subgroup short-flip-after-cursor-toggle:
                pass       -> DMESG-WARN (shard-apl)
Test kms_plane_multiple:
        Subgroup atomic-pipe-a-tiling-none:
                pass       -> DMESG-WARN (shard-apl)
        Subgroup legacy-pipe-a-tiling-x:
                pass       -> DMESG-WARN (shard-apl)
Test kms_properties:
        Subgroup connector-properties-atomic:
                pass       -> DMESG-WARN (shard-apl)
Test kms_vblank:
        Subgroup pipe-a-query-forked-busy-hang:
                dmesg-warn -> PASS       (shard-hsw)

---- Known issues:

Test gem_eio:
        Subgroup in-flight-contexts:
                pass       -> INCOMPLETE (shard-apl) fdo#104945 +1
Test kms_chv_cursor_fail:
        Subgroup pipe-b-64x64-bottom-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
Test kms_color:
        Subgroup pipe-a-ctm-green-to-red:
                pass       -> DMESG-WARN (shard-apl) fdo#104852
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-varying-size:
                pass       -> DMESG-WARN (shard-apl) fdo#102670
Test kms_flip:
        Subgroup flip-vs-absolute-wf_vblank:
                fail       -> PASS       (shard-hsw) fdo#100368
        Subgroup modeset-vs-vblank-race:
                fail       -> PASS       (shard-hsw) fdo#103060
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-pri-indfb-multidraw:
                fail       -> PASS       (shard-apl) fdo#103167
        Subgroup fbc-1p-primscrn-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-apl) fdo#101623

fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#104852 https://bugs.freedesktop.org/show_bug.cgi?id=104852
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-apl        total:3357 pass:1759 dwarn:9   dfail:0   fail:8   skip:1579 time:11390s
shard-hsw        total:3460 pass:1767 dwarn:1   dfail:0   fail:1   skip:1690 time:11695s
shard-snb        total:3460 pass:1359 dwarn:1   dfail:0   fail:1   skip:2099 time:6573s
Blacklisted hosts:
shard-kbl        total:3374 pass:1894 dwarn:2   dfail:0   fail:8   skip:1468 time:9034s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8166/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add a parameter to disable SAGV

[Jani Nikula]

On Mon, 26 Feb 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Mon, Feb 26, 2018 at 03:45:51PM -0800, Azhar Shaikh wrote:
>> SAGV handling is currently broken which can result in system hangs.
>> Add a parameter to disable SAGV, till the SAGV handling is fixed.
>
> Not just handling of the limitations we have with SAGV is broken but
> also probably some hidden DBUF config issue that is worst when SAGV
> is enabled.
>
> But also the handling broken is not a good reason for the parameter
> itself. But the good part of this is the ability to debug display
> hard hangs issues with SAGV requirements.
>
> I'm in favor of the paramenter. We just need a different justification
> here.

*cringe* at adding a parameter to workaround issues.

>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
>> Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_params.c | 3 +++
>>  drivers/gpu/drm/i915/i915_params.h | 3 ++-
>>  drivers/gpu/drm/i915/intel_pm.c    | 2 +-
>>  3 files changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 08108ce5be21..6aede52240b0 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -167,6 +167,9 @@ struct i915_params i915_modparams __read_mostly = {
>>  i915_param_named_unsafe(enable_dp_mst, bool, 0600,
>>  	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
>>  
>> +i915_param_named_unsafe(disable_sagv, bool, 0600,
>> +	"Disable SAGV (default: false)");
>> +
>
> I understand that SAGV is default to enable and enabled by BIOS so we
> actually need to disable it.
>
> However the name will cause the same old confusion as disable power well disable....
>
> So I'm in favor of bool i915.enable_sagv defaults to enable
> and use i915.enable_sagv=0 when we need to disable it.

Agreed, if you must use a parameter.

BR,
Jani.



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

*cringe* at adding a parameter to workaround issues.

[Marc Herbert]

Hi Jani,

> *cringe* at adding a parameter to workaround issues.

I understand that *each* parameter has the potential to *multiply* the total
number of configurations and that the resulting combinatorial explosion is
absolutely not scalable and sustainable from a validation perspective. No
one should expect to get support here when options like this one are set to
a non-default value.

When something breaks on the other hand, transparent _test_ knobs like this
one have proved invaluable countless times to help troubleshoot and isolate
issues. It's at least 10 times more productive to ask a non-expert in some
opposite timezone "please test again after rebooting with this parameter"
compared to "test again after applying this patch, recompiling, etc." -
assuming the latter has any chance of success at all.  I'm speaking from
actual experience as we are routinely experiencing both type of situations.

I hope the "unsafe" part of "i915_param_named_unsafe" provides a permanent
solution to both problems by making a clear distinction between the only one
single true supported configuration on one hand versus test datapoints
on the other hand.  Same for "tainted", sysfs or else.

Marc

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

Re: *cringe* at adding a parameter to workaround issues.

[Jani Nikula]

On Thu, 01 Mar 2018, Marc Herbert <Marc.Herbert@intel.com> wrote:
> Hi Jani,
>
>> *cringe* at adding a parameter to workaround issues.
>
> I understand that *each* parameter has the potential to *multiply* the total
> number of configurations and that the resulting combinatorial explosion is
> absolutely not scalable and sustainable from a validation perspective. No
> one should expect to get support here when options like this one are set to
> a non-default value.
>
> When something breaks on the other hand, transparent _test_ knobs like this
> one have proved invaluable countless times to help troubleshoot and isolate
> issues. It's at least 10 times more productive to ask a non-expert in some
> opposite timezone "please test again after rebooting with this parameter"
> compared to "test again after applying this patch, recompiling, etc." -
> assuming the latter has any chance of success at all.  I'm speaking from
> actual experience as we are routinely experiencing both type of situations.

Yes, I do understand, and that's why it's a "cringe", not a "nak".

The flip side are bug reports that we still get regardless of warnings
in dmesg and kernel taint when people try out parameters that they read
about in random forums, and expect support. And lack of bug reports when
people silently workaround their issues using module parameters.

> I hope the "unsafe" part of "i915_param_named_unsafe" provides a permanent
> solution to both problems by making a clear distinction between the only one
> single true supported configuration on one hand versus test datapoints
> on the other hand.  Same for "tainted", sysfs or else.

This is what I hoped too when I added support for the "unsafe"
parameters. :) Now I wish we could move this stuff to debugfs and flip
debugfs options as easily as module parameters. I think this is the
primary reason we have so many debugging module parameters: they are
more convenient than debugfs.

BR,
Jani.


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