diff mbox

drm/i915/edp: Only use alternate fixed mode when requested

Message ID 1523488406-21245-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Taylor, Clinton A April 11, 2018, 11:13 p.m. UTC 
From: Clint Taylor <clinton.a.taylor@intel.com>

In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP
if available."), the patch was always selecting the alternate refresh rate
even though user space was asking for the higher rate. This patch adds a
check for vrefresh rate as well as the rest of the mode geometry.

V2: use clock instead of vrefresh for compare.

Fixes: dc911f5bd8aac ("Allow alternate fixed mode for eDP if available.")
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Chris Wilson April 11, 2018, 11:11 p.m. UTC | #1 
Quoting clinton.a.taylor@intel.com (2018-04-12 00:13:26)
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP
> if available."), the patch was always selecting the alternate refresh rate
> even though user space was asking for the higher rate. This patch adds a
> check for vrefresh rate as well as the rest of the mode geometry.
> 
> V2: use clock instead of vrefresh for compare.
> 
> Fixes: dc911f5bd8aac ("Allow alternate fixed mode for eDP if available.")
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>

Still leaves the other discussion point in the other thread unresolved.
The fields are supplied by the user and can be arbitrary, so if they
request a clock for a 30Hz mode, instead of using the 40Hz alternative,
we use the 60Hz normal mode (by way of example). Is equality always the
best choice here?
-Chris
Taylor, Clinton A April 12, 2018, 9:31 p.m. UTC | #2 
On 04/11/2018 04:11 PM, Chris Wilson wrote:
> Quoting clinton.a.taylor@intel.com (2018-04-12 00:13:26)
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP
>> if available."), the patch was always selecting the alternate refresh rate
>> even though user space was asking for the higher rate. This patch adds a
>> check for vrefresh rate as well as the rest of the mode geometry.
>>
>> V2: use clock instead of vrefresh for compare.
>>
>> Fixes: dc911f5bd8aac ("Allow alternate fixed mode for eDP if available.")
>> Cc: David Weinehall <david.weinehall@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> Still leaves the other discussion point in the other thread unresolved.
> The fields are supplied by the user and can be arbitrary, so if they
> request a clock for a 30Hz mode, instead of using the 40Hz alternative,
> we use the 60Hz normal mode (by way of example). Is equality always the
> best choice here?
This feature is for testing PSR panels that don't support single frame 
setup times in their preferred timing. The down-clocked mode is the 
timing that the panel specifically states is supported. If a customer 
specifies a custom mode either it should be rejected or the eDP fixed 
mode (preferred) should be used. If we want to allow the users to set a 
custom timing to their eDP panels then we should get rid of the fixed 
mode feature for eDP panels.

-Clint

> -Chris
Rodrigo Vivi April 14, 2018, 5:57 p.m. UTC | #3 
> On Apr 12, 2018, at 2:21 PM, Taylor, Clinton A <clinton.a.taylor@intel.com> wrote:
> 
> 
> 
>> On 04/11/2018 04:11 PM, Chris Wilson wrote:
>> Quoting clinton.a.taylor@intel.com (2018-04-12 00:13:26)
>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>> 
>>> In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP
>>> if available."), the patch was always selecting the alternate refresh rate
>>> even though user space was asking for the higher rate. This patch adds a
>>> check for vrefresh rate as well as the rest of the mode geometry.
>>> 
>>> V2: use clock instead of vrefresh for compare.
>>> 
>>> Fixes: dc911f5bd8aac ("Allow alternate fixed mode for eDP if available.")
>>> Cc: David Weinehall <david.weinehall@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> Still leaves the other discussion point in the other thread unresolved.
>> The fields are supplied by the user and can be arbitrary, so if they
>> request a clock for a 30Hz mode, instead of using the 40Hz alternative,
>> we use the 60Hz normal mode (by way of example). Is equality always the
>> best choice here?
> This feature is for testing PSR panels that don't support single frame setup times in their preferred timing. The down-clocked mode is the timing that the panel specifically states is supported. If a customer specifies a custom mode either it should be rejected or the eDP fixed mode (preferred) should be used. If we want to allow the users to set a custom timing to their eDP panels then we should get rid of the fixed mode feature for eDP panels.

But that was the goal of dc911f5bd8aac because
most of the panels we had here by that time with PSR had that short vblank periods for higher mode...
So the idea was to remove the fixed mode allowing the alternate one with lower rate and consequently higher vblank period.

But the idea was never to respect the arbitrary user request. All panels we were targeting here had both modes listed as supported... one with 60Hz and one with 48 Hz. Both same resolution.

> 
> -Clint
> 
>> -Chris
>
Jani Nikula April 30, 2018, 7:49 a.m. UTC | #4 
On Sat, 14 Apr 2018, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
>> On Apr 12, 2018, at 2:21 PM, Taylor, Clinton A <clinton.a.taylor@intel.com> wrote:
>> 
>> 
>> 
>>> On 04/11/2018 04:11 PM, Chris Wilson wrote:
>>> Quoting clinton.a.taylor@intel.com (2018-04-12 00:13:26)
>>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>> 
>>>> In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP
>>>> if available."), the patch was always selecting the alternate refresh rate
>>>> even though user space was asking for the higher rate. This patch adds a
>>>> check for vrefresh rate as well as the rest of the mode geometry.
>>>> 
>>>> V2: use clock instead of vrefresh for compare.
>>>> 
>>>> Fixes: dc911f5bd8aac ("Allow alternate fixed mode for eDP if available.")
>>>> Cc: David Weinehall <david.weinehall@linux.intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>> Still leaves the other discussion point in the other thread unresolved.
>>> The fields are supplied by the user and can be arbitrary, so if they
>>> request a clock for a 30Hz mode, instead of using the 40Hz alternative,
>>> we use the 60Hz normal mode (by way of example). Is equality always the
>>> best choice here?
>> This feature is for testing PSR panels that don't support single frame setup times in their preferred timing. The down-clocked mode is the timing that the panel specifically states is supported. If a customer specifies a custom mode either it should be rejected or the eDP fixed mode (preferred) should be used. If we want to allow the users to set a custom timing to their eDP panels then we should get rid of the fixed mode feature for eDP panels.
>
> But that was the goal of dc911f5bd8aac because
> most of the panels we had here by that time with PSR had that short vblank periods for higher mode...
> So the idea was to remove the fixed mode allowing the alternate one with lower rate and consequently higher vblank period.
>
> But the idea was never to respect the arbitrary user request. All panels we were targeting here had both modes listed as supported... one with 60Hz and one with 48 Hz. Both same resolution.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469

Either we finally get a fix in with Cc: stable, or we revert
dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP if
available.")

BR,
Jani.



>
>> 
>> -Clint
>> 
>>> -Chris
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Taylor, Clinton A April 30, 2018, 9:54 p.m. UTC | #5 
On 04/30/2018 12:49 AM, Jani Nikula wrote:
> On Sat, 14 Apr 2018, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
>>> On Apr 12, 2018, at 2:21 PM, Taylor, Clinton A <clinton.a.taylor@intel.com> wrote:
>>>
>>>
>>>
>>>> On 04/11/2018 04:11 PM, Chris Wilson wrote:
>>>> Quoting clinton.a.taylor@intel.com (2018-04-12 00:13:26)
>>>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>>>
>>>>> In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP
>>>>> if available."), the patch was always selecting the alternate refresh rate
>>>>> even though user space was asking for the higher rate. This patch adds a
>>>>> check for vrefresh rate as well as the rest of the mode geometry.
>>>>>
>>>>> V2: use clock instead of vrefresh for compare.
>>>>>
>>>>> Fixes: dc911f5bd8aac ("Allow alternate fixed mode for eDP if available.")
>>>>> Cc: David Weinehall <david.weinehall@linux.intel.com>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>>> Still leaves the other discussion point in the other thread unresolved.
>>>> The fields are supplied by the user and can be arbitrary, so if they
>>>> request a clock for a 30Hz mode, instead of using the 40Hz alternative,
>>>> we use the 60Hz normal mode (by way of example). Is equality always the
>>>> best choice here?
>>> This feature is for testing PSR panels that don't support single frame setup times in their preferred timing. The down-clocked mode is the timing that the panel specifically states is supported. If a customer specifies a custom mode either it should be rejected or the eDP fixed mode (preferred) should be used. If we want to allow the users to set a custom timing to their eDP panels then we should get rid of the fixed mode feature for eDP panels.
>> But that was the goal of dc911f5bd8aac because
>> most of the panels we had here by that time with PSR had that short vblank periods for higher mode...
>> So the idea was to remove the fixed mode allowing the alternate one with lower rate and consequently higher vblank period.
>>
>> But the idea was never to respect the arbitrary user request. All panels we were targeting here had both modes listed as supported... one with 60Hz and one with 48 Hz. Both same resolution.
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
>
> Either we finally get a fix in with Cc: stable, or we revert
> dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP if
> available.")

I have V3 of the patch in flight now. Though I have no problem with the 
revert as well.

-Clint


>
> BR,
> Jani.
>
>
>
>>> -Clint
>>>
>>>> -Chris
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4..6a2e256 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1681,7 +1681,8 @@  static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
 			m1->vdisplay == m2->vdisplay &&
 			m1->vsync_start == m2->vsync_start &&
 			m1->vsync_end == m2->vsync_end &&
-			m1->vtotal == m2->vtotal);
+			m1->vtotal == m2->vtotal &&
+			m1->clock == m2->clock);
 	return bres;
 }