[PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Jenny TC]

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,
 			      &reg_val) != 1) {
-- 
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: Use AUX for backlight only if eDP 1.4 or later

[Patchwork]

== 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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Pandiyan, Dhinakaran]

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,
>  			      &reg_val) != 1) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Jani Nikula]

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,
>>  			      &reg_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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Tc, Jenny]

> >> 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,
> >>  			      &reg_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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Pandiyan, Dhinakaran]

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,
> > >>  			      &reg_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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Puthikorn Voravootivat]

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,
>> > >>                                &reg_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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[David Weinehall]

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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[David Weinehall]

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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Jani Nikula]

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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Puthikorn Voravootivat]

> 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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Pandiyan, Dhinakaran]

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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[David Weinehall]

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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[David Weinehall]

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,
>>>   			      &reg_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

Re: [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later

[Pandiyan, Dhinakaran]

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,
> >>>   			      &reg_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