* [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
@ 2017-07-19 10:29 Jenny TC
2017-07-19 11:50 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-07-19 18:22 ` [PATCH] " Pandiyan, Dhinakaran
0 siblings, 2 replies; 15+ messages in thread
From: Jenny TC @ 2017-07-19 10:29 UTC (permalink / raw)
To: intel-gfx
Cc: Puthikorn Voravootivat, Daniel Vetter, Jenny TC, David Weinehall
With older panels, AUX pin for backlight doesn't work. On some
panels, this causes backlight issues on S3 resume. Enable the
feature only for eDP1.4 or later.
Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index b25cd88..421f31f 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
* via PWM pin or using AUX is better than using PWM pin.
*
* The heuristic to determine that using AUX pin is better than using PWM pin is
- * that the panel support any of the feature list here.
+ * that the panel is eDP 1.4 or later and support any of the feature list here
* - Regional backlight brightness adjustment
* - Backlight PWM frequency set
* - More than 8 bits resolution of brightness level
@@ -310,6 +310,10 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
return true;
+ /* Enable this for eDP 1.4 panel or later. */
+ if (intel_dp->edp_dpcd[0] < DP_EDP_14)
+ return false;
+
/* Panel supports regional backlight brightness adjustment */
if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
®_val) != 1) {
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-19 10:29 [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later Jenny TC
@ 2017-07-19 11:50 ` Patchwork
2017-07-19 18:22 ` [PATCH] " Pandiyan, Dhinakaran
1 sibling, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-07-19 11:50 UTC (permalink / raw)
To: Jenny TC; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use AUX for backlight only if eDP 1.4 or later
URL : https://patchwork.freedesktop.org/series/27566/
State : success
== Summary ==
Series 27566v1 drm/i915: Use AUX for backlight only if eDP 1.4 or later
https://patchwork.freedesktop.org/api/1.0/series/27566/revisions/1/mbox/
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
dmesg-warn -> PASS (fi-pnv-d510) fdo#101597
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-byt-j1900) fdo#101705
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:441s
fi-bdw-gvtdvm total:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:429s
fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:358s
fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:546s
fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:514s
fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:496s
fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:487s
fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:600s
fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:437s
fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:415s
fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:418s
fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:501s
fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:471s
fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:469s
fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:577s
fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:578s
fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:558s
fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:461s
fi-skl-6700hq total:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:584s
fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:471s
fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:474s
fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:434s
fi-skl-x1585l total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:471s
fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:540s
fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:403s
541e75cb2ac9f85388e9e6f228d98f266506eebd drm-tip: 2017y-07m-19d-10h-10m-36s UTC integration manifest
b10d4f6 drm/i915: Use AUX for backlight only if eDP 1.4 or later
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5232/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-19 10:29 [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later Jenny TC
2017-07-19 11:50 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-07-19 18:22 ` Pandiyan, Dhinakaran
2017-07-20 9:33 ` Jani Nikula
1 sibling, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-07-19 18:22 UTC (permalink / raw)
To: Tc, Jenny; +Cc: puthik, Vetter, Daniel, intel-gfx, Weinehall, David
On Wed, 2017-07-19 at 15:59 +0530, Jenny TC wrote:
> With older panels, AUX pin for backlight doesn't work. On some
> panels, this causes backlight issues on S3 resume.
What is it that we are missing in the resume path?
> Enable the
> feature only for eDP1.4 or later.
I can't find the eDP 1.4 requirement in the spec. Regional brightness
control was added in eDP 1.4, but we don't enable that. My concern is we
might be missing a real fix by ignoring < eDP 1.4 panels.
>
> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
1. Please use the "Fixes" tag to refer to the commit that introduced the
code you are fixing.
2. The "Suggested-by" tag is more common to give credits to the person
who suggested the fix.
3. Please use the "Bugzilla" tag to point to the bug that David
reported.
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index b25cd88..421f31f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
> * via PWM pin or using AUX is better than using PWM pin.
> *
> * The heuristic to determine that using AUX pin is better than using PWM pin is
> - * that the panel support any of the feature list here.
> + * that the panel is eDP 1.4 or later and support any of the feature list here
> * - Regional backlight brightness adjustment
> * - Backlight PWM frequency set
> * - More than 8 bits resolution of brightness level
> @@ -310,6 +310,10 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> return true;
>
> + /* Enable this for eDP 1.4 panel or later. */
> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
> + return false;
> +
> /* Panel supports regional backlight brightness adjustment */
> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
> ®_val) != 1) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-19 18:22 ` [PATCH] " Pandiyan, Dhinakaran
@ 2017-07-20 9:33 ` Jani Nikula
2017-07-20 10:06 ` Tc, Jenny
2017-07-20 15:35 ` David Weinehall
0 siblings, 2 replies; 15+ messages in thread
From: Jani Nikula @ 2017-07-20 9:33 UTC (permalink / raw)
To: Pandiyan, Dhinakaran, Tc, Jenny
Cc: puthik, Vetter, Daniel, intel-gfx, Weinehall, David
On Wed, 19 Jul 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> On Wed, 2017-07-19 at 15:59 +0530, Jenny TC wrote:
>> With older panels, AUX pin for backlight doesn't work.
What evidence do you have to back up that claim?
>> On some panels, this causes backlight issues on S3 resume.
>
> What is it that we are missing in the resume path?
>
>> Enable the
>> feature only for eDP1.4 or later.
>
> I can't find the eDP 1.4 requirement in the spec. Regional brightness
> control was added in eDP 1.4, but we don't enable that. My concern is we
> might be missing a real fix by ignoring < eDP 1.4 panels.
Agreed.
This has been going on too long now, with no real effort to get at the
roots of the problem. The right approach is to revert the regressing
commits now, and start over. Reverts posted [1].
BR,
Jani.
[1] https://patchwork.freedesktop.org/series/27623/
>
>
>>
>> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
>
> 1. Please use the "Fixes" tag to refer to the commit that introduced the
> code you are fixing.
> 2. The "Suggested-by" tag is more common to give credits to the person
> who suggested the fix.
> 3. Please use the "Bugzilla" tag to point to the bug that David
> reported.
>
>
>> Signed-off-by: Jenny TC <jenny.tc@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> index b25cd88..421f31f 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>> * via PWM pin or using AUX is better than using PWM pin.
>> *
>> * The heuristic to determine that using AUX pin is better than using PWM pin is
>> - * that the panel support any of the feature list here.
>> + * that the panel is eDP 1.4 or later and support any of the feature list here
>> * - Regional backlight brightness adjustment
>> * - Backlight PWM frequency set
>> * - More than 8 bits resolution of brightness level
>> @@ -310,6 +310,10 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
>> return true;
>>
>> + /* Enable this for eDP 1.4 panel or later. */
>> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
>> + return false;
>> +
>> /* Panel supports regional backlight brightness adjustment */
>> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
>> ®_val) != 1) {
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-20 9:33 ` Jani Nikula
@ 2017-07-20 10:06 ` Tc, Jenny
2017-07-20 20:27 ` Pandiyan, Dhinakaran
2017-07-20 15:35 ` David Weinehall
1 sibling, 1 reply; 15+ messages in thread
From: Tc, Jenny @ 2017-07-20 10:06 UTC (permalink / raw)
To: Jani Nikula, Pandiyan, Dhinakaran
Cc: puthik, Vetter, Daniel, intel-gfx, Weinehall, David
> >> With older panels, AUX pin for backlight doesn't work.
>
> What evidence do you have to back up that claim?
Debugging further it's found that the panel I am having doesn't support AUX Backlight.
cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
...
0701: bb ff 00 00
....
With below change its working for my panel. But doesn't address issue reported in
https://bugs.freedesktop.org/show_bug.cgi?id=101619 which seems to have a wrong DPCD data.
Since we don't have a proper fix for all panels, I agree for revert.
intel_dp_aux_display_control_heuristic(struct intel_connector *connector)
struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
uint8_t reg_val;
+ /* Panel dosn't support enabling AUX Backlight control */
+ if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)) {
+ return false;
+ }
/* Panel doesn't support adjusting backlight brightness via PWN pin */
if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
return true;
> >> On some panels, this causes backlight issues on S3 resume.
> >
> > What is it that we are missing in the resume path?
> >
> >> Enable the
> >> feature only for eDP1.4 or later.
> >
> > I can't find the eDP 1.4 requirement in the spec. Regional brightness
> > control was added in eDP 1.4, but we don't enable that. My concern is
> > we might be missing a real fix by ignoring < eDP 1.4 panels.
>
> Agreed.
>
> This has been going on too long now, with no real effort to get at the roots of
> the problem. The right approach is to revert the regressing commits now,
> and start over. Reverts posted [1].
>
> BR,
> Jani.
>
> [1] https://patchwork.freedesktop.org/series/27623/
>
> >
> >
> >>
> >> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
> >
> > 1. Please use the "Fixes" tag to refer to the commit that introduced
> > the code you are fixing.
> > 2. The "Suggested-by" tag is more common to give credits to the person
> > who suggested the fix.
> > 3. Please use the "Bugzilla" tag to point to the bug that David
> > reported.
> >
> >
> >> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> index b25cd88..421f31f 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct
> intel_connector *connector,
> >> * via PWM pin or using AUX is better than using PWM pin.
> >> *
> >> * The heuristic to determine that using AUX pin is better than
> >> using PWM pin is
> >> - * that the panel support any of the feature list here.
> >> + * that the panel is eDP 1.4 or later and support any of the feature
> >> + list here
> >> * - Regional backlight brightness adjustment
> >> * - Backlight PWM frequency set
> >> * - More than 8 bits resolution of brightness level @@ -310,6
> >> +310,10 @@ static int intel_dp_aux_setup_backlight(struct
> intel_connector *connector,
> >> if (!(intel_dp->edp_dpcd[2] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> >> return true;
> >>
> >> + /* Enable this for eDP 1.4 panel or later. */
> >> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
> >> + return false;
> >> +
> >> /* Panel supports regional backlight brightness adjustment */
> >> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
> >> ®_val) != 1) {
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-20 9:33 ` Jani Nikula
2017-07-20 10:06 ` Tc, Jenny
@ 2017-07-20 15:35 ` David Weinehall
2017-07-20 18:20 ` Pandiyan, Dhinakaran
1 sibling, 1 reply; 15+ messages in thread
From: David Weinehall @ 2017-07-20 15:35 UTC (permalink / raw)
To: Jani Nikula, Pandiyan, Dhinakaran, Tc, Jenny
Cc: puthik, Vetter, Daniel, intel-gfx
On 2017-07-20 12:33, Jani Nikula wrote:
> On Wed, 19 Jul 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
>> On Wed, 2017-07-19 at 15:59 +0530, Jenny TC wrote:
>>> With older panels, AUX pin for backlight doesn't work.
> What evidence do you have to back up that claim?
Chapter 10.1 does, at least according to my reading, state that AUX CH
backlight control
requires eDP v1.4.
>>> On some panels, this causes backlight issues on S3 resume.
>> What is it that we are missing in the resume path?
I suspect this isn't so much a resume issue, as a matter of some devices
by sheer luck (read by using the backlight set already on bootup)
keeping the right backlight level until after a suspend/resume cycle.
>>
>>> Enable the
>>> feature only for eDP1.4 or later.
>> I can't find the eDP 1.4 requirement in the spec. Regional brightness
>> control was added in eDP 1.4, but we don't enable that. My concern is we
>> might be missing a real fix by ignoring < eDP 1.4 panels.
> Agreed.
>
> This has been going on too long now, with no real effort to get at the
> roots of the problem. The right approach is to revert the regressing
> commits now, and start over. Reverts posted [1].
>
> BR,
> Jani.
>
> [1] https://patchwork.freedesktop.org/series/27623/
>
>>
>>> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
>> 1. Please use the "Fixes" tag to refer to the commit that introduced the
>> code you are fixing.
>> 2. The "Suggested-by" tag is more common to give credits to the person
>> who suggested the fix.
>> 3. Please use the "Bugzilla" tag to point to the bug that David
>> reported.
>>
>>
>>> Signed-off-by: Jenny TC <jenny.tc@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>>> index b25cd88..421f31f 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>>> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>>> * via PWM pin or using AUX is better than using PWM pin.
>>> *
>>> * The heuristic to determine that using AUX pin is better than using PWM pin is
>>> - * that the panel support any of the feature list here.
>>> + * that the panel is eDP 1.4 or later and support any of the feature list here
>>> * - Regional backlight brightness adjustment
>>> * - Backlight PWM frequency set
>>> * - More than 8 bits resolution of brightness level
>>> @@ -310,6 +310,10 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>>> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
>>> return true;
>>>
>>> + /* Enable this for eDP 1.4 panel or later. */
>>> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
>>> + return false;
>>> +
>>> /* Panel supports regional backlight brightness adjustment */
>>> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
>>> ®_val) != 1) {
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-20 15:35 ` David Weinehall
@ 2017-07-20 18:20 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-07-20 18:20 UTC (permalink / raw)
To: Weinehall, David; +Cc: puthik, Vetter, Daniel, intel-gfx, Tc, Jenny
On Thu, 2017-07-20 at 18:35 +0300, David Weinehall wrote:
> On 2017-07-20 12:33, Jani Nikula wrote:
> > On Wed, 19 Jul 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> >> On Wed, 2017-07-19 at 15:59 +0530, Jenny TC wrote:
> >>> With older panels, AUX pin for backlight doesn't work.
> > What evidence do you have to back up that claim?
> Chapter 10.1 does, at least according to my reading, state that AUX CH
> backlight control
> requires eDP v1.4.
>
Yeah, I read that too and it is very misleading. There are several
references to AUX CH backlight control being added in eDP v1.2 if you
read further.
"Table 10-1 summarizes the display brightness control modes available
with eDP v1.2 (and higher)."
"Displays that report eDP v1.1 (or lower; indicated by a value of 00h)
have 00h values in DPCD Addresses 00701h through 007FFh a because
enhanced display control capability is not supported. For eDP v1.2 (and
higher), the Source device reads the eDP capability registers starting
at DPCD Address 00701h. Through the use of read/write registers located
within the range of DPCD Addresses 00720h through 007FFh, the Source
device is able to control certain display aspects based on the
capabilities reported h in DPCD Addresses 00701h through 0071Fh."
> >>> On some panels, this causes backlight issues on S3 resume.
> >> What is it that we are missing in the resume path?
> I suspect this isn't so much a resume issue, as a matter of some devices
> by sheer luck (read by using the backlight set already on bootup)
> keeping the right backlight level until after a suspend/resume cycle.
Makes sense.
> >>
> >>> Enable the
> >>> feature only for eDP1.4 or later.
> >> I can't find the eDP 1.4 requirement in the spec. Regional brightness
> >> control was added in eDP 1.4, but we don't enable that. My concern is we
> >> might be missing a real fix by ignoring < eDP 1.4 panels.
> > Agreed.
> >
> > This has been going on too long now, with no real effort to get at the
> > roots of the problem. The right approach is to revert the regressing
> > commits now, and start over. Reverts posted [1].
> >
> > BR,
> > Jani.
> >
> > [1] https://patchwork.freedesktop.org/series/27623/
> >
> >>
> >>> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
> >> 1. Please use the "Fixes" tag to refer to the commit that introduced the
> >> code you are fixing.
> >> 2. The "Suggested-by" tag is more common to give credits to the person
> >> who suggested the fix.
> >> 3. Please use the "Bugzilla" tag to point to the bug that David
> >> reported.
> >>
> >>
> >>> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
> >>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >>> index b25cd88..421f31f 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >>> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
> >>> * via PWM pin or using AUX is better than using PWM pin.
> >>> *
> >>> * The heuristic to determine that using AUX pin is better than using PWM pin is
> >>> - * that the panel support any of the feature list here.
> >>> + * that the panel is eDP 1.4 or later and support any of the feature list here
> >>> * - Regional backlight brightness adjustment
> >>> * - Backlight PWM frequency set
> >>> * - More than 8 bits resolution of brightness level
> >>> @@ -310,6 +310,10 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
> >>> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> >>> return true;
> >>>
> >>> + /* Enable this for eDP 1.4 panel or later. */
> >>> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
> >>> + return false;
> >>> +
> >>> /* Panel supports regional backlight brightness adjustment */
> >>> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
> >>> ®_val) != 1) {
>
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki
> Business Identity Code: 0357606 - 4
> Domiciled in Helsinki
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-20 10:06 ` Tc, Jenny
@ 2017-07-20 20:27 ` Pandiyan, Dhinakaran
2017-07-24 23:15 ` Puthikorn Voravootivat
0 siblings, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-07-20 20:27 UTC (permalink / raw)
To: Tc, Jenny; +Cc: puthik, Vetter, Daniel, intel-gfx, Weinehall, David
On Thu, 2017-07-20 at 10:06 +0000, Tc, Jenny wrote:
> > >> With older panels, AUX pin for backlight doesn't work.
> >
> > What evidence do you have to back up that claim?
>
> Debugging further it's found that the panel I am having doesn't support AUX Backlight.
>
> cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
> ...
> 0701: bb ff 00 00
> ....
>
> With below change its working for my panel. But doesn't address issue reported in
> https://bugs.freedesktop.org/show_bug.cgi?id=101619 which seems to have a wrong DPCD data.
> Since we don't have a proper fix for all panels, I agree for revert.
>
> intel_dp_aux_display_control_heuristic(struct intel_connector *connector)
> struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> uint8_t reg_val;
>
> + /* Panel dosn't support enabling AUX Backlight control */
> + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)) {
> + return false;
> + }
AUX backlight brightness control should work even without AUX enable
capability.
> /* Panel doesn't support adjusting backlight brightness via PWN pin */
> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> return true;
>
> > >> On some panels, this causes backlight issues on S3 resume.
> > >
> > > What is it that we are missing in the resume path?
> > >
> > >> Enable the
> > >> feature only for eDP1.4 or later.
> > >
> > > I can't find the eDP 1.4 requirement in the spec. Regional brightness
> > > control was added in eDP 1.4, but we don't enable that. My concern is
> > > we might be missing a real fix by ignoring < eDP 1.4 panels.
> >
> > Agreed.
> >
> > This has been going on too long now, with no real effort to get at the roots of
> > the problem. The right approach is to revert the regressing commits now,
> > and start over. Reverts posted [1].
> >
> > BR,
> > Jani.
> >
> > [1] https://patchwork.freedesktop.org/series/27623/
> >
> > >
> > >
> > >>
> > >> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
> > >
> > > 1. Please use the "Fixes" tag to refer to the commit that introduced
> > > the code you are fixing.
> > > 2. The "Suggested-by" tag is more common to give credits to the person
> > > who suggested the fix.
> > > 3. Please use the "Bugzilla" tag to point to the bug that David
> > > reported.
> > >
> > >
> > >> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> > >> ---
> > >> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
> > >> 1 file changed, 5 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > >> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > >> index b25cd88..421f31f 100644
> > >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > >> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct
> > intel_connector *connector,
> > >> * via PWM pin or using AUX is better than using PWM pin.
> > >> *
> > >> * The heuristic to determine that using AUX pin is better than
> > >> using PWM pin is
> > >> - * that the panel support any of the feature list here.
> > >> + * that the panel is eDP 1.4 or later and support any of the feature
> > >> + list here
> > >> * - Regional backlight brightness adjustment
> > >> * - Backlight PWM frequency set
> > >> * - More than 8 bits resolution of brightness level @@ -310,6
> > >> +310,10 @@ static int intel_dp_aux_setup_backlight(struct
> > intel_connector *connector,
> > >> if (!(intel_dp->edp_dpcd[2] &
> > DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> > >> return true;
> > >>
> > >> + /* Enable this for eDP 1.4 panel or later. */
> > >> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
> > >> + return false;
> > >> +
> > >> /* Panel supports regional backlight brightness adjustment */
> > >> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
> > >> ®_val) != 1) {
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-20 20:27 ` Pandiyan, Dhinakaran
@ 2017-07-24 23:15 ` Puthikorn Voravootivat
2017-07-25 12:41 ` David Weinehall
2017-07-31 10:55 ` Jani Nikula
0 siblings, 2 replies; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-07-24 23:15 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: intel-gfx, Weinehall, David, puthik, Vetter, Daniel, Tc, Jenny
I saw a DP 1.3 panel that advertise AUX backlight brightness control
but not working properly. So it should work but not in real world.
I think that is good reason enough to add this as a heuristic.
On Thu, Jul 20, 2017 at 1:27 PM, Pandiyan, Dhinakaran
<dhinakaran.pandiyan@intel.com> wrote:
>
>
>
> On Thu, 2017-07-20 at 10:06 +0000, Tc, Jenny wrote:
>> > >> With older panels, AUX pin for backlight doesn't work.
>> >
>> > What evidence do you have to back up that claim?
>>
>> Debugging further it's found that the panel I am having doesn't support AUX Backlight.
>>
>> cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
>> ...
>> 0701: bb ff 00 00
>> ....
>>
>> With below change its working for my panel. But doesn't address issue reported in
>> https://bugs.freedesktop.org/show_bug.cgi?id=101619 which seems to have a wrong DPCD data.
>> Since we don't have a proper fix for all panels, I agree for revert.
>>
>> intel_dp_aux_display_control_heuristic(struct intel_connector *connector)
>> struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>> uint8_t reg_val;
>>
>> + /* Panel dosn't support enabling AUX Backlight control */
>> + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP)) {
>> + return false;
>> + }
>
> AUX backlight brightness control should work even without AUX enable
> capability.
>
>
>> /* Panel doesn't support adjusting backlight brightness via PWN pin */
>> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
>> return true;
>>
>> > >> On some panels, this causes backlight issues on S3 resume.
>> > >
>> > > What is it that we are missing in the resume path?
>> > >
>> > >> Enable the
>> > >> feature only for eDP1.4 or later.
>> > >
>> > > I can't find the eDP 1.4 requirement in the spec. Regional brightness
>> > > control was added in eDP 1.4, but we don't enable that. My concern is
>> > > we might be missing a real fix by ignoring < eDP 1.4 panels.
>> >
>> > Agreed.
>> >
>> > This has been going on too long now, with no real effort to get at the roots of
>> > the problem. The right approach is to revert the regressing commits now,
>> > and start over. Reverts posted [1].
>> >
>> > BR,
>> > Jani.
>> >
>> > [1] https://patchwork.freedesktop.org/series/27623/
>> >
>> > >
>> > >
>> > >>
>> > >> Fix-suggested-by: Puthikorn Voravootivat <puthik@chromium.org>
>> > >
>> > > 1. Please use the "Fixes" tag to refer to the commit that introduced
>> > > the code you are fixing.
>> > > 2. The "Suggested-by" tag is more common to give credits to the person
>> > > who suggested the fix.
>> > > 3. Please use the "Bugzilla" tag to point to the bug that David
>> > > reported.
>> > >
>> > >
>> > >> Signed-off-by: Jenny TC <jenny.tc@intel.com>
>> > >> ---
>> > >> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +++++-
>> > >> 1 file changed, 5 insertions(+), 1 deletion(-)
>> > >>
>> > >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> > >> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> > >> index b25cd88..421f31f 100644
>> > >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> > >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> > >> @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct
>> > intel_connector *connector,
>> > >> * via PWM pin or using AUX is better than using PWM pin.
>> > >> *
>> > >> * The heuristic to determine that using AUX pin is better than
>> > >> using PWM pin is
>> > >> - * that the panel support any of the feature list here.
>> > >> + * that the panel is eDP 1.4 or later and support any of the feature
>> > >> + list here
>> > >> * - Regional backlight brightness adjustment
>> > >> * - Backlight PWM frequency set
>> > >> * - More than 8 bits resolution of brightness level @@ -310,6
>> > >> +310,10 @@ static int intel_dp_aux_setup_backlight(struct
>> > intel_connector *connector,
>> > >> if (!(intel_dp->edp_dpcd[2] &
>> > DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
>> > >> return true;
>> > >>
>> > >> + /* Enable this for eDP 1.4 panel or later. */
>> > >> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
>> > >> + return false;
>> > >> +
>> > >> /* Panel supports regional backlight brightness adjustment */
>> > >> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
>> > >> ®_val) != 1) {
>> >
>> > --
>> > Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-24 23:15 ` Puthikorn Voravootivat
@ 2017-07-25 12:41 ` David Weinehall
2017-07-25 13:11 ` David Weinehall
2017-07-31 10:55 ` Jani Nikula
1 sibling, 1 reply; 15+ messages in thread
From: David Weinehall @ 2017-07-25 12:41 UTC (permalink / raw)
To: Puthikorn Voravootivat, Pandiyan, Dhinakaran
Cc: Vetter, Daniel, intel-gfx, Tc, Jenny
On 2017-07-25 02:15, Puthikorn Voravootivat wrote:
> I saw a DP 1.3 panel that advertise AUX backlight brightness control
> but not working properly. So it should work but not in real world.
> I think that is good reason enough to add this as a heuristic.
>
>
Either key it on eDP 1.4 and hope that it's a reasonable expectation,
or employ a whitelist (potentially lots of effort adding all displays that
supports AUX backlight, but safe from regressions) or a blacklist
(potentially fewer displays need to be added, but risks introducing
regressions for end users).
Kind regards, David
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-25 12:41 ` David Weinehall
@ 2017-07-25 13:11 ` David Weinehall
0 siblings, 0 replies; 15+ messages in thread
From: David Weinehall @ 2017-07-25 13:11 UTC (permalink / raw)
To: David Weinehall
Cc: Puthikorn Voravootivat, Vetter, Daniel, intel-gfx, Pandiyan,
Dhinakaran, Tc, Jenny
On Tue, Jul 25, 2017 at 03:41:46PM +0300, David Weinehall wrote:
> On 2017-07-25 02:15, Puthikorn Voravootivat wrote:
> > I saw a DP 1.3 panel that advertise AUX backlight brightness control
> > but not working properly. So it should work but not in real world.
> > I think that is good reason enough to add this as a heuristic.
> >
> >
> Either key it on eDP 1.4 and hope that it's a reasonable expectation,
> or employ a whitelist (potentially lots of effort adding all displays that
> supports AUX backlight, but safe from regressions) or a blacklist
> (potentially fewer displays need to be added, but risks introducing
> regressions for end users).
Sorry for the blurb in the previous mail, I replied from my official
Intel address by mistake :S
Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-24 23:15 ` Puthikorn Voravootivat
2017-07-25 12:41 ` David Weinehall
@ 2017-07-31 10:55 ` Jani Nikula
2017-07-31 22:41 ` Puthikorn Voravootivat
1 sibling, 1 reply; 15+ messages in thread
From: Jani Nikula @ 2017-07-31 10:55 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: puthik, Vetter, Daniel, intel-gfx, Tc, Jenny, Weinehall, David
On Mon, 24 Jul 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> I saw a DP 1.3 panel that advertise AUX backlight brightness control
> but not working properly. So it should work but not in real world.
> I think that is good reason enough to add this as a heuristic.
Are you suggesting the one panel you mention is representative of the
real world?
Granted, the original aux backlight implementation supported a very
narrow selection of panels. I believe this is the very reason you are
working on this patch series.
But now you're suggesting another arbitrary narrow selection of panels
based on limited evidence.
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-31 10:55 ` Jani Nikula
@ 2017-07-31 22:41 ` Puthikorn Voravootivat
2017-08-01 23:15 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-07-31 22:41 UTC (permalink / raw)
To: Jani Nikula
Cc: intel-gfx, Pandiyan, Dhinakaran, Weinehall, David,
Puthikorn Voravootivat, Vetter, Daniel, Tc, Jenny
> But now you're suggesting another arbitrary narrow selection of panels
> based on limited evidence.
I understand your point that the panel I observe is not the
representative of the real world.
My point is that we don't know that the panel will work or not unless
we test all panel in the world.
And blacklist would be too much work to maintain and whitelist would
make this code too limited.
As standard adoption should be better over time, I suggest that the
newer panel should have
better implement of the standard than older panel. And I suggest that
eDP 1.4 should be a good
heuristic for the "newer panel" based on these 2 reasons
1. Even though it is a limited evident, David and I independently saw
unrelated eDP 1.3 panel that
implement this feature incorrectly.
2. eDP 1.4 is the first version that support AUX backlight enablement.
TCON vendor probably also
make sure the AUX backlight brightness ajustment works when testing
that feature.
Is this make sense?
Thanks.
On Mon, Jul 31, 2017 at 3:55 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 24 Jul 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
>> I saw a DP 1.3 panel that advertise AUX backlight brightness control
>> but not working properly. So it should work but not in real world.
>> I think that is good reason enough to add this as a heuristic.
>
> Are you suggesting the one panel you mention is representative of the
> real world?
>
> Granted, the original aux backlight implementation supported a very
> narrow selection of panels. I believe this is the very reason you are
> working on this patch series.
>
> But now you're suggesting another arbitrary narrow selection of panels
> based on limited evidence.
>
>
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-07-31 22:41 ` Puthikorn Voravootivat
@ 2017-08-01 23:15 ` Pandiyan, Dhinakaran
2017-08-02 13:53 ` David Weinehall
0 siblings, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-08-01 23:15 UTC (permalink / raw)
To: puthik; +Cc: Vetter, Daniel, intel-gfx, Tc, Jenny, Weinehall, David
On Mon, 2017-07-31 at 15:41 -0700, Puthikorn Voravootivat wrote:
> > But now you're suggesting another arbitrary narrow selection of panels
> > based on limited evidence.
>
> I understand your point that the panel I observe is not the
> representative of the real world.
>
> My point is that we don't know that the panel will work or not unless
> we test all panel in the world.
> And blacklist would be too much work to maintain and whitelist would
> make this code too limited.
> As standard adoption should be better over time, I suggest that the
> newer panel should have
> better implement of the standard than older panel. And I suggest that
> eDP 1.4 should be a good
> heuristic for the "newer panel" based on these 2 reasons
>
> 1. Even though it is a limited evident, David and I independently saw
> unrelated eDP 1.3 panel that
> implement this feature incorrectly.
> 2. eDP 1.4 is the first version that support AUX backlight enablement.
> TCON vendor probably also
> make sure the AUX backlight brightness ajustment works when testing
> that feature.
>
> Is this make sense?
>
> Thanks.
>
I tried to investigate this a little bit and found a device that
reproduces the issue. The backlight does not come back up after a
suspend-resume cycle because the PWM controller does not get enabled at
resume. However, things just work at boot because the BIOS happens to
enable PCH PWM at boot and the panel lights up via the BL_PWM_PIN. Like
you said, this could be because some eDP 1.3 panels have a broken
implementation and eDP 1.4 panels are better. Or, with the BL_PWM_PIN
wired to the board, it simply overrides the DPCD settings. I decided to
not disconnect the PWM pin and test this theory since this was a
development laptop. In summary, I am not really sure blacklisting all
eDP 1.3 panels is the best idea. Also, I don't know how many eDP 1.4
panels this has been tested to correctly work on.
Anyway, since we have four panels that do not work, we could check if
these are the same model/make etc. and blacklist them if there's a
common thread.
-DK
> On Mon, Jul 31, 2017 at 3:55 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
> > On Mon, 24 Jul 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> >> I saw a DP 1.3 panel that advertise AUX backlight brightness control
> >> but not working properly. So it should work but not in real world.
> >> I think that is good reason enough to add this as a heuristic.
> >
> > Are you suggesting the one panel you mention is representative of the
> > real world?
> >
> > Granted, the original aux backlight implementation supported a very
> > narrow selection of panels. I believe this is the very reason you are
> > working on this patch series.
> >
> > But now you're suggesting another arbitrary narrow selection of panels
> > based on limited evidence.
> >
> >
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
2017-08-01 23:15 ` Pandiyan, Dhinakaran
@ 2017-08-02 13:53 ` David Weinehall
0 siblings, 0 replies; 15+ messages in thread
From: David Weinehall @ 2017-08-02 13:53 UTC (permalink / raw)
To: Pandiyan, Dhinakaran, puthik; +Cc: Vetter, Daniel, intel-gfx, Tc, Jenny
On 2017-08-02 02:15, Pandiyan, Dhinakaran wrote:
>
>
> On Mon, 2017-07-31 at 15:41 -0700, Puthikorn Voravootivat wrote:
>>> But now you're suggesting another arbitrary narrow selection of panels
>>> based on limited evidence.
>> I understand your point that the panel I observe is not the
>> representative of the real world.
>>
>> My point is that we don't know that the panel will work or not unless
>> we test all panel in the world.
>> And blacklist would be too much work to maintain and whitelist would
>> make this code too limited.
>> As standard adoption should be better over time, I suggest that the
>> newer panel should have
>> better implement of the standard than older panel. And I suggest that
>> eDP 1.4 should be a good
>> heuristic for the "newer panel" based on these 2 reasons
>>
>> 1. Even though it is a limited evident, David and I independently saw
>> unrelated eDP 1.3 panel that
>> implement this feature incorrectly.
>> 2. eDP 1.4 is the first version that support AUX backlight enablement.
>> TCON vendor probably also
>> make sure the AUX backlight brightness ajustment works when testing
>> that feature.
>>
>> Is this make sense?
>>
>> Thanks.
>>
> I tried to investigate this a little bit and found a device that
> reproduces the issue. The backlight does not come back up after a
> suspend-resume cycle because the PWM controller does not get enabled at
> resume. However, things just work at boot because the BIOS happens to
> enable PCH PWM at boot and the panel lights up via the BL_PWM_PIN. Like
> you said, this could be because some eDP 1.3 panels have a broken
> implementation and eDP 1.4 panels are better. Or, with the BL_PWM_PIN
> wired to the board, it simply overrides the DPCD settings. I decided to
> not disconnect the PWM pin and test this theory since this was a
> development laptop. In summary, I am not really sure blacklisting all
> eDP 1.3 panels is the best idea. Also, I don't know how many eDP 1.4
> panels this has been tested to correctly work on.
>
> Anyway, since we have four panels that do not work, we could check if
> these are the same model/make etc. and blacklist them if there's a
> common thread.
>
The panel in my system is made by LG, model LP140QH1-SPF1.
Kind regards, David
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-08-02 13:53 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-19 10:29 [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later Jenny TC
2017-07-19 11:50 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-07-19 18:22 ` [PATCH] " Pandiyan, Dhinakaran
2017-07-20 9:33 ` Jani Nikula
2017-07-20 10:06 ` Tc, Jenny
2017-07-20 20:27 ` Pandiyan, Dhinakaran
2017-07-24 23:15 ` Puthikorn Voravootivat
2017-07-25 12:41 ` David Weinehall
2017-07-25 13:11 ` David Weinehall
2017-07-31 10:55 ` Jani Nikula
2017-07-31 22:41 ` Puthikorn Voravootivat
2017-08-01 23:15 ` Pandiyan, Dhinakaran
2017-08-02 13:53 ` David Weinehall
2017-07-20 15:35 ` David Weinehall
2017-07-20 18:20 ` Pandiyan, Dhinakaran
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.