* [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 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
* 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
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.