[PATCH] drm/i915/psr: enable psr1 on psr2 panels

[PATCH] drm/i915/psr: enable psr1 on psr2 panels

[vathsala nagaraju]

From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
useful in cases where psr2 fails and user wants to enable
psr1 feature for power saving until a fix
is provided for psr2.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 5 +++++
 drivers/gpu/drm/i915/i915_params.h | 1 +
 drivers/gpu/drm/i915/intel_psr.c   | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 08108ce..5b6f5af 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
 	"(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
 	"Default: -1 (use per-chip default)");
 
+i915_param_named_unsafe(force_psr1, int, 0600,
+	"Enable PSR1 on PSR2 Panel "
+	"(0=disabled, 1=enabled) "
+	"Default: -1 (use per-chip default)");
+
 i915_param_named_unsafe(alpha_support, bool, 0400,
 	"Enable alpha quality driver support for latest hardware. "
 	"See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c963603..1f5dd1c 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,6 +44,7 @@
 	param(int, enable_fbc, -1) \
 	param(int, enable_ppgtt, -1) \
 	param(int, enable_psr, -1) \
+	param(int, force_psr1, -1) \
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2d53f73..415e377 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 
 	crtc_state->has_psr = true;
 	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
+	if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
+		crtc_state->has_psr2 = false;
 	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
 }
 
-- 
1.9.1

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

Re: [PATCH] drm/i915/psr: enable psr1 on psr2 panels

[Rodrigo Vivi]

On Fri, Apr 06, 2018 at 12:10:24PM -0700, Dhinakaran Pandiyan wrote:
> 
> 
> 
> On Sat, 2018-04-07 at 00:12 +0530, vathsala nagaraju wrote:
> > From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > 
> > Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
> > useful in cases where psr2 fails and user wants to enable
> > psr1 feature for power saving until a fix
> > is provided for psr2.

The parameters shouldn't be used by users to select a configuration.
They are marked as unsafe. We should only enable the feature when
we are comfortable it doesn't cause trouble.

> 
> 
> We should perhaps make enable_psr=1 enable just PSR1. I am not
> comfortable that we enable PSR2 at all, there are no tests in IGT for
> selective update, seems like nobody really knows exactly how well it
> works. 

Agreed. Probably good for now to avoid PSR2 in all situations and only
allow PSR2 when we are properly testing it.

> 
> 
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c | 5 +++++
> >  drivers/gpu/drm/i915/i915_params.h | 1 +
> >  drivers/gpu/drm/i915/intel_psr.c   | 2 ++
> >  3 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index 08108ce..5b6f5af 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
> >  	"(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
> >  	"Default: -1 (use per-chip default)");
> >  
> > +i915_param_named_unsafe(force_psr1, int, 0600,
> > +	"Enable PSR1 on PSR2 Panel "
> > +	"(0=disabled, 1=enabled) "
> > +	"Default: -1 (use per-chip default)");
> > +
> >  i915_param_named_unsafe(alpha_support, bool, 0400,
> >  	"Enable alpha quality driver support for latest hardware. "
> >  	"See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
> > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > index c963603..1f5dd1c 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -44,6 +44,7 @@
> >  	param(int, enable_fbc, -1) \
> >  	param(int, enable_ppgtt, -1) \
> >  	param(int, enable_psr, -1) \
> > +	param(int, force_psr1, -1) \
> >  	param(int, disable_power_well, -1) \
> >  	param(int, enable_ips, 1) \
> >  	param(int, invert_brightness, 0) \
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2d53f73..415e377 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  
> >  	crtc_state->has_psr = true;
> >  	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> > +	if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
> > +		crtc_state->has_psr2 = false;
> >  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
> >  }
> >  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915/psr: enable psr1 on psr2 panels

[Patchwork]

== Series Details ==

Series: drm/i915/psr: enable psr1 on psr2 panels
URL   : https://patchwork.freedesktop.org/series/41294/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
768ed9ff8e29 drm/i915/psr: enable psr1 on psr2 panels
-:28: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#28: FILE: drivers/gpu/drm/i915/i915_params.c:99:
+i915_param_named_unsafe(force_psr1, int, 0600,
+	"Enable PSR1 on PSR2 Panel "

total: 0 errors, 0 warnings, 1 checks, 26 lines checked

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

Re: [PATCH] drm/i915/psr: enable psr1 on psr2 panels

[Dhinakaran Pandiyan]

On Sat, 2018-04-07 at 00:12 +0530, vathsala nagaraju wrote:
> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
> useful in cases where psr2 fails and user wants to enable
> psr1 feature for power saving until a fix
> is provided for psr2.


We should perhaps make enable_psr=1 enable just PSR1. I am not
comfortable that we enable PSR2 at all, there are no tests in IGT for
selective update, seems like nobody really knows exactly how well it
works. 


> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 5 +++++
>  drivers/gpu/drm/i915/i915_params.h | 1 +
>  drivers/gpu/drm/i915/intel_psr.c   | 2 ++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 08108ce..5b6f5af 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
>  	"(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
>  	"Default: -1 (use per-chip default)");
>  
> +i915_param_named_unsafe(force_psr1, int, 0600,
> +	"Enable PSR1 on PSR2 Panel "
> +	"(0=disabled, 1=enabled) "
> +	"Default: -1 (use per-chip default)");
> +
>  i915_param_named_unsafe(alpha_support, bool, 0400,
>  	"Enable alpha quality driver support for latest hardware. "
>  	"See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index c963603..1f5dd1c 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,6 +44,7 @@
>  	param(int, enable_fbc, -1) \
>  	param(int, enable_ppgtt, -1) \
>  	param(int, enable_psr, -1) \
> +	param(int, force_psr1, -1) \
>  	param(int, disable_power_well, -1) \
>  	param(int, enable_ips, 1) \
>  	param(int, invert_brightness, 0) \
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2d53f73..415e377 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  
>  	crtc_state->has_psr = true;
>  	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> +	if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
> +		crtc_state->has_psr2 = false;
>  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
>  }
>  

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

✓ Fi.CI.BAT: success for drm/i915/psr: enable psr1 on psr2 panels

[Patchwork]

== Series Details ==

Series: drm/i915/psr: enable psr1 on psr2 panels
URL   : https://patchwork.freedesktop.org/series/41294/
State : success

== Summary ==

Series 41294v1 drm/i915/psr: enable psr1 on psr2 panels
https://patchwork.freedesktop.org/api/1.0/series/41294/revisions/1/mbox/

---- Possible new issues:

Test drv_module_reload:
        Subgroup basic-no-display:
                incomplete -> PASS       (fi-elk-e7500)

---- Known issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

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

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:436s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:444s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:383s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:533s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:297s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:513s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:528s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:511s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:411s
fi-cfl-s3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:567s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:514s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:592s
fi-elk-e7500     total:285  pass:226  dwarn:0   dfail:0   fail:0   skip:59  time:423s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:317s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:536s
fi-glk-j4005     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:405s
fi-ilk-650       total:285  pass:224  dwarn:0   dfail:0   fail:1   skip:60  time:426s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:473s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:440s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:475s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-pnv-d510      total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:670s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:449s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:533s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:510s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:502s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:430s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:448s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:572s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:406s

3cbdbfb1ecc8769c8db88e95b1d6ea55c5e87dbf drm-tip: 2018y-04m-06d-15h-15m-21s UTC integration manifest
768ed9ff8e29 drm/i915/psr: enable psr1 on psr2 panels

== Logs ==

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

✗ Fi.CI.IGT: warning for drm/i915/psr: enable psr1 on psr2 panels

[Patchwork]

== Series Details ==

Series: drm/i915/psr: enable psr1 on psr2 panels
URL   : https://patchwork.freedesktop.org/series/41294/
State : warning

== Summary ==

---- Possible new issues:

Test gem_mmap_gtt:
        Subgroup forked-medium-copy-odd:
                dmesg-warn -> PASS       (shard-hsw)
Test gem_pwrite:
        Subgroup big-gtt-backwards:
                pass       -> SKIP       (shard-apl)
Test kms_frontbuffer_tracking:
        Subgroup fbc-2p-pri-indfb-multidraw:
                dmesg-fail -> PASS       (shard-hsw)

---- Known issues:

Test kms_cursor_legacy:
        Subgroup 2x-long-flip-vs-cursor-legacy:
                fail       -> PASS       (shard-hsw) fdo#104873
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                pass       -> FAIL       (shard-snb) fdo#103925
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252

fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-apl        total:2680 pass:1834 dwarn:1   dfail:0   fail:7   skip:837 time:12673s
shard-hsw        total:2680 pass:1784 dwarn:1   dfail:0   fail:3   skip:891 time:11422s
shard-snb        total:2680 pass:1377 dwarn:1   dfail:0   fail:3   skip:1299 time:6945s

== Logs ==

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

Re: [PATCH] drm/i915/psr: enable psr1 on psr2 panels

[vathsala nagaraju]

+ puthik
On Saturday 07 April 2018 12:36 AM, Vivi, Rodrigo wrote:
> On Fri, Apr 06, 2018 at 12:10:24PM -0700, Dhinakaran Pandiyan wrote:
>>
>>
>> On Sat, 2018-04-07 at 00:12 +0530, vathsala nagaraju wrote:
>>> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>>
>>> Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
>>> useful in cases where psr2 fails and user wants to enable
>>> psr1 feature for power saving until a fix
>>> is provided for psr2.
> The parameters shouldn't be used by users to select a configuration.
> They are marked as unsafe. We should only enable the feature when
> we are comfortable it doesn't cause trouble.

The idea was to give user the option to switch to psr1 ,if they want to.

>
>>
>> We should perhaps make enable_psr=1 enable just PSR1. I am not
>> comfortable that we enable PSR2 at all, there are no tests in IGT for
>> selective update, seems like nobody really knows exactly how well it
>> works.
with enable_psr , we are deciding whether to use psr1/psr2.
we can reuse enable_psr.

> Agreed. Probably good for now to avoid PSR2 in all situations and only
> allow PSR2 when we are properly testing it.
>
>>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>> Cc: José Roberto de Souza <jose.souza@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_params.c | 5 +++++
>>>   drivers/gpu/drm/i915/i915_params.h | 1 +
>>>   drivers/gpu/drm/i915/intel_psr.c   | 2 ++
>>>   3 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>>> index 08108ce..5b6f5af 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
>>>      "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
>>>      "Default: -1 (use per-chip default)");
>>>
>>> +i915_param_named_unsafe(force_psr1, int, 0600,
>>> +   "Enable PSR1 on PSR2 Panel "
>>> +   "(0=disabled, 1=enabled) "
>>> +   "Default: -1 (use per-chip default)");
>>> +
>>>   i915_param_named_unsafe(alpha_support, bool, 0400,
>>>      "Enable alpha quality driver support for latest hardware. "
>>>      "See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
>>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>>> index c963603..1f5dd1c 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.h
>>> +++ b/drivers/gpu/drm/i915/i915_params.h
>>> @@ -44,6 +44,7 @@
>>>      param(int, enable_fbc, -1) \
>>>      param(int, enable_ppgtt, -1) \
>>>      param(int, enable_psr, -1) \
>>> +   param(int, force_psr1, -1) \
>>>      param(int, disable_power_well, -1) \
>>>      param(int, enable_ips, 1) \
>>>      param(int, invert_brightness, 0) \
>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>>> index 2d53f73..415e377 100644
>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>> @@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>>>
>>>      crtc_state->has_psr = true;
>>>      crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
>>> +   if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
>>> +           crtc_state->has_psr2 = false;
>>>      DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
>>>   }
>>>

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

Re: [PATCH] drm/i915/psr: enable psr1 on psr2 panels

[Rodrigo Vivi]

On Wed, Apr 11, 2018 at 03:01:24PM +0530, vathsala nagaraju wrote:
> 
> + puthik
> On Saturday 07 April 2018 12:36 AM, Vivi, Rodrigo wrote:
> > On Fri, Apr 06, 2018 at 12:10:24PM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > 
> > > On Sat, 2018-04-07 at 00:12 +0530, vathsala nagaraju wrote:
> > > > From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > 
> > > > Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
> > > > useful in cases where psr2 fails and user wants to enable
> > > > psr1 feature for power saving until a fix
> > > > is provided for psr2.
> > The parameters shouldn't be used by users to select a configuration.
> > They are marked as unsafe. We should only enable the feature when
> > we are comfortable it doesn't cause trouble.
> 
> The idea was to give user the option to switch to psr1 ,if they want to.

This is exactly the problem that I'm trying to avoid ;)

But I do understand your perspective since I had the same, at least
until when Jani showed me an old but gold note from ajax:
https://www.redhat.com/archives/rhl-devel-list/2008-January/msg00861.html

If we did not validated we shouldn't be allowing the choice.

Although I'm still in favor of the parameters here because it makes
our lives easier when developing and debugging. But user should never
be using those.

Specially on a feature full of corner cases like PSR. So, while
we don't really fix all the known bad PSR bugs we should keep it disabled.


> 
> > 
> > > 
> > > We should perhaps make enable_psr=1 enable just PSR1. I am not
> > > comfortable that we enable PSR2 at all, there are no tests in IGT for
> > > selective update, seems like nobody really knows exactly how well it
> > > works.
> with enable_psr , we are deciding whether to use psr1/psr2.
> we can reuse enable_psr.
> 

While we are not confident PSR2 ever worked we should not be using that
at all. So if you want any change like this I would suggest to avoid PSR2
at all for everybody while we don't fix PSR2.

But when PSR and PSR2 gets enabled we keep the parameter to allow debug
and triage... i.e enable_psr=0 disables PSR, enable_psr=1 enables what ever
PSR is the default for that panel/platform.

Thanks,
Rodrigo.

> > Agreed. Probably good for now to avoid PSR2 in all situations and only
> > allow PSR2 when we are properly testing it.
> > 
> > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/i915_params.c | 5 +++++
> > > >   drivers/gpu/drm/i915/i915_params.h | 1 +
> > > >   drivers/gpu/drm/i915/intel_psr.c   | 2 ++
> > > >   3 files changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > > > index 08108ce..5b6f5af 100644
> > > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > > @@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
> > > >      "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
> > > >      "Default: -1 (use per-chip default)");
> > > > 
> > > > +i915_param_named_unsafe(force_psr1, int, 0600,
> > > > +   "Enable PSR1 on PSR2 Panel "
> > > > +   "(0=disabled, 1=enabled) "
> > > > +   "Default: -1 (use per-chip default)");
> > > > +
> > > >   i915_param_named_unsafe(alpha_support, bool, 0400,
> > > >      "Enable alpha quality driver support for latest hardware. "
> > > >      "See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
> > > > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > > > index c963603..1f5dd1c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_params.h
> > > > +++ b/drivers/gpu/drm/i915/i915_params.h
> > > > @@ -44,6 +44,7 @@
> > > >      param(int, enable_fbc, -1) \
> > > >      param(int, enable_ppgtt, -1) \
> > > >      param(int, enable_psr, -1) \
> > > > +   param(int, force_psr1, -1) \
> > > >      param(int, disable_power_well, -1) \
> > > >      param(int, enable_ips, 1) \
> > > >      param(int, invert_brightness, 0) \
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 2d53f73..415e377 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > 
> > > >      crtc_state->has_psr = true;
> > > >      crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> > > > +   if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
> > > > +           crtc_state->has_psr2 = false;
> > > >      DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
> > > >   }
> > > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/psr: enable psr1 on psr2 panels

[Pandiyan, Dhinakaran]

On Wed, 2018-04-11 at 07:40 -0700, Rodrigo Vivi wrote:
> On Wed, Apr 11, 2018 at 03:01:24PM +0530, vathsala nagaraju wrote:
> > 
> > + puthik
> > On Saturday 07 April 2018 12:36 AM, Vivi, Rodrigo wrote:
> > > On Fri, Apr 06, 2018 at 12:10:24PM -0700, Dhinakaran Pandiyan wrote:
> > > > 
> > > > 
> > > > On Sat, 2018-04-07 at 00:12 +0530, vathsala nagaraju wrote:
> > > > > From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > > 
> > > > > Adds force_psr1 mod parameter to enable psr1 on psr2 panels.
> > > > > useful in cases where psr2 fails and user wants to enable
> > > > > psr1 feature for power saving until a fix
> > > > > is provided for psr2.
> > > The parameters shouldn't be used by users to select a configuration.
> > > They are marked as unsafe. We should only enable the feature when
> > > we are comfortable it doesn't cause trouble.
> > 
> > The idea was to give user the option to switch to psr1 ,if they want to.
> 
> This is exactly the problem that I'm trying to avoid ;)
> 
> But I do understand your perspective since I had the same, at least
> until when Jani showed me an old but gold note from ajax:
> https://www.redhat.com/archives/rhl-devel-list/2008-January/msg00861.html
> 
> If we did not validated we shouldn't be allowing the choice.
> 
> Although I'm still in favor of the parameters here because it makes
> our lives easier when developing and debugging. But user should never
> be using those.
> 
> Specially on a feature full of corner cases like PSR. So, while
> we don't really fix all the known bad PSR bugs we should keep it disabled.
> 
> 
> > 
> > > 
> > > > 
> > > > We should perhaps make enable_psr=1 enable just PSR1. I am not
> > > > comfortable that we enable PSR2 at all, there are no tests in IGT for
> > > > selective update, seems like nobody really knows exactly how well it
> > > > works.
> > with enable_psr , we are deciding whether to use psr1/psr2.
> > we can reuse enable_psr.
> > 
> 
> While we are not confident PSR2 ever worked we should not be using that
> at all. So if you want any change like this I would suggest to avoid PSR2
> at all for everybody while we don't fix PSR2.
> 
> But when PSR and PSR2 gets enabled we keep the parameter to allow debug
> and triage... i.e enable_psr=0 disables PSR, enable_psr=1 enables what ever
> PSR is the default for that panel/platform.
> 


Vathsala,

Please make i915.enable_psr=1 to enable only PSR1 at this point. Until
we add new IGT's to test for selective update, PSR2 will not receive
regular testing and as such is not worthy of enabling even with a module
parameter.




> Thanks,
> Rodrigo.
> 
> > > Agreed. Probably good for now to avoid PSR2 in all situations and only
> > > allow PSR2 when we are properly testing it.
> > > 
> > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/i915/i915_params.c | 5 +++++
> > > > >   drivers/gpu/drm/i915/i915_params.h | 1 +
> > > > >   drivers/gpu/drm/i915/intel_psr.c   | 2 ++
> > > > >   3 files changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > > > > index 08108ce..5b6f5af 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > > > @@ -95,6 +95,11 @@ struct i915_params i915_modparams __read_mostly = {
> > > > >      "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
> > > > >      "Default: -1 (use per-chip default)");
> > > > > 
> > > > > +i915_param_named_unsafe(force_psr1, int, 0600,
> > > > > +   "Enable PSR1 on PSR2 Panel "
> > > > > +   "(0=disabled, 1=enabled) "
> > > > > +   "Default: -1 (use per-chip default)");
> > > > > +
> > > > >   i915_param_named_unsafe(alpha_support, bool, 0400,
> > > > >      "Enable alpha quality driver support for latest hardware. "
> > > > >      "See also CONFIG_DRM_I915_ALPHA_SUPPORT.");
> > > > > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > > > > index c963603..1f5dd1c 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_params.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_params.h
> > > > > @@ -44,6 +44,7 @@
> > > > >      param(int, enable_fbc, -1) \
> > > > >      param(int, enable_ppgtt, -1) \
> > > > >      param(int, enable_psr, -1) \
> > > > > +   param(int, force_psr1, -1) \
> > > > >      param(int, disable_power_well, -1) \
> > > > >      param(int, enable_ips, 1) \
> > > > >      param(int, invert_brightness, 0) \
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 2d53f73..415e377 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -540,6 +540,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > > 
> > > > >      crtc_state->has_psr = true;
> > > > >      crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> > > > > +   if (i915_modparams.force_psr1 == 1 && crtc_state->has_psr2)
> > > > > +           crtc_state->has_psr2 = false;
> > > > >      DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
> > > > >   }
> > > > > 
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx