* [PATCH] drm/i915/bxt: Enable VBT based BL control for DP @ 2017-06-06 7:09 Mustamin B Mustaffa 2017-06-06 7:28 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-06-06 9:24 ` [PATCH] " Jani Nikula 0 siblings, 2 replies; 8+ messages in thread From: Mustamin B Mustaffa @ 2017-06-06 7:09 UTC (permalink / raw) To: intel-gfx; +Cc: Mustamin B Mustaffa Currently, BXT_PP is hardcoded with value '0'. It practically disabled eDP backlight on MRB (BXT) platform. This patch will tell which BXT_PP registers (there are two set of PP_CONTROL in the spec) to be used as defined in VBT (Video Bios Timing table) and this will enabled eDP backlight controller on MRB (BXT) platform. Change-Id: I42242d8def30d09298b629e3cd4828016189c3fa Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-46254 --- drivers/gpu/drm/i915/intel_dp.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d1670b8..124f58b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) /* We should never land here with regular DP ports */ WARN_ON(!is_edp(intel_dp)); - /* - * TODO: BXT has 2 PPS instances. The correct port->PPS instance - * mapping needs to be retrieved from VBT, for now just hard-code to - * use instance #0 always. - */ if (!intel_dp->pps_reset) - return 0; + return dev_priv->vbt.backlight.controller; intel_dp->pps_reset = false; -- 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] 8+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/bxt: Enable VBT based BL control for DP 2017-06-06 7:09 [PATCH] drm/i915/bxt: Enable VBT based BL control for DP Mustamin B Mustaffa @ 2017-06-06 7:28 ` Patchwork 2017-06-06 9:24 ` [PATCH] " Jani Nikula 1 sibling, 0 replies; 8+ messages in thread From: Patchwork @ 2017-06-06 7:28 UTC (permalink / raw) To: Mustamin B Mustaffa; +Cc: intel-gfx == Series Details == Series: drm/i915/bxt: Enable VBT based BL control for DP URL : https://patchwork.freedesktop.org/series/25323/ State : success == Summary == Series 25323v1 drm/i915/bxt: Enable VBT based BL control for DP https://patchwork.freedesktop.org/api/1.0/series/25323/revisions/1/mbox/ Test kms_busy: Subgroup basic-flip-default-a: pass -> DMESG-WARN (fi-skl-6700hq) fdo#101144 fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:442s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:437s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:577s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:511s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:490s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:486s fi-glk-2a total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:588s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:432s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:416s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:490s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:463s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:463s fi-kbl-7560u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:568s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:460s fi-skl-6700hq total:278 pass:228 dwarn:2 dfail:0 fail:26 skip:22 time:406s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:465s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:506s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:438s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:531s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:398s 30d3326ecb407cad6c03ef6a6d3805c70ba9f0a9 drm-tip: 2017y-06m-05d-15h-21m-50s UTC integration manifest 2be13ca drm/i915/bxt: Enable VBT based BL control for DP == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4884/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP 2017-06-06 7:09 [PATCH] drm/i915/bxt: Enable VBT based BL control for DP Mustamin B Mustaffa 2017-06-06 7:28 ` ✓ Fi.CI.BAT: success for " Patchwork @ 2017-06-06 9:24 ` Jani Nikula 2017-06-06 12:34 ` Imre Deak 1 sibling, 1 reply; 8+ messages in thread From: Jani Nikula @ 2017-06-06 9:24 UTC (permalink / raw) To: intel-gfx; +Cc: Mustamin B Mustaffa On Tue, 06 Jun 2017, Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> wrote: > Currently, BXT_PP is hardcoded with value '0'. > It practically disabled eDP backlight on MRB (BXT) platform. > > This patch will tell which BXT_PP registers (there are two set of PP_CONTROL in the spec) > to be used as defined in VBT (Video Bios Timing table) and this will enabled eDP > backlight controller on MRB (BXT) platform. Didn't look at the spec. Is there a 1:1 mapping/routing with the two PPS and backlight PWM? Does this hold for Geminilake too, as your change impacts that as well? > Change-Id: I42242d8def30d09298b629e3cd4828016189c3fa Drop this. > Signed-off-by: Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> > Tracked-On: https://jira01.devtools.intel.com/browse/OAM-46254 Drop this. Even I can't access this. > --- > drivers/gpu/drm/i915/intel_dp.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index d1670b8..124f58b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) > /* We should never land here with regular DP ports */ > WARN_ON(!is_edp(intel_dp)); > > - /* > - * TODO: BXT has 2 PPS instances. The correct port->PPS instance > - * mapping needs to be retrieved from VBT, for now just hard-code to > - * use instance #0 always. > - */ > if (!intel_dp->pps_reset) > - return 0; > + return dev_priv->vbt.backlight.controller; So the existing code around here looks a bit convoluted, not least because now pretty much all PPS access first does - intel_pps_get_registers(), which calls - bxt_power_sequencer_idx(), which calls - intel_dp_init_panel_power_sequencer_registers(), which calls - intel_pps_get_registers()... With your change, for controller == 1 and pps_reset == true, the first time the registers are needed, we'll initialize the correct controller 1 registers, but controller 0 registers are returned. From there on, we'll keep returning controller 1 registers until pps_reset is set to true again. Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS lost state after suspend breaking eDP link training") and 8e8232d51878 ("drm/i915: Deduplicate PPS register retrieval") which I think create the loop. BR, Jani. > > intel_dp->pps_reset = false; -- 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] 8+ messages in thread
* Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP 2017-06-06 9:24 ` [PATCH] " Jani Nikula @ 2017-06-06 12:34 ` Imre Deak 2017-06-06 14:58 ` Bloomfield, Jon 0 siblings, 1 reply; 8+ messages in thread From: Imre Deak @ 2017-06-06 12:34 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx, Mustamin B Mustaffa On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote: > On Tue, 06 Jun 2017, Mustamin B Mustaffa <mustamin.b.mustaffa@intel.com> wrote: > > [...] > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index d1670b8..124f58b 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) > > /* We should never land here with regular DP ports */ > > WARN_ON(!is_edp(intel_dp)); > > > > - /* > > - * TODO: BXT has 2 PPS instances. The correct port->PPS instance > > - * mapping needs to be retrieved from VBT, for now just hard-code to > > - * use instance #0 always. > > - */ > > if (!intel_dp->pps_reset) > > - return 0; > > + return dev_priv->vbt.backlight.controller; > > So the existing code around here looks a bit convoluted, not least > because now pretty much all PPS access first does > > - intel_pps_get_registers(), which calls > - bxt_power_sequencer_idx(), which calls > - intel_dp_init_panel_power_sequencer_registers(), which calls > - intel_pps_get_registers()... > > With your change, for controller == 1 and pps_reset == true, the first > time the registers are needed, we'll initialize the correct controller 1 > registers, but controller 0 registers are returned. From there on, we'll > keep returning controller 1 registers until pps_reset is set to true > again. > > Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS lost > state after suspend breaking eDP link training") and 8e8232d51878 > ("drm/i915: Deduplicate PPS register retrieval") which I think create > the loop. A loop would be prevented by the pps->reset check, but agree the code isn't nice, I guess I overlooked this when I wrote it. To make things clearer we could factor out a helper from intel_dp_init_panel_power_sequencer_registers() that takes pps_registers and call this helper from bxt_power_sequencer_idx(). So how about something like the following: diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 58dca87..e312f63 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -424,6 +424,14 @@ static void intel_dp_unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes) dst[i] = src >> ((3-i) * 8); } +struct pps_registers { + i915_reg_t pp_ctrl; + i915_reg_t pp_stat; + i915_reg_t pp_on; + i915_reg_t pp_off; + i915_reg_t pp_div; +}; + static void intel_dp_init_panel_power_sequencer(struct drm_device *dev, struct intel_dp *intel_dp); @@ -431,6 +439,16 @@ static void intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, struct intel_dp *intel_dp, bool force_disable_vdd); + +static void intel_pps_get_instance_registers(struct drm_i915_private *dev_priv, + int idx, + struct pps_registers *regs); + +static void +intel_dp_init_panel_power_sequencer_instance(struct drm_i915_private *dev_priv, + struct intel_dp *intel_dp, + bool force_disable_vdd, + const struct pps_registers *regs); static void intel_dp_pps_init(struct drm_device *dev, struct intel_dp *intel_dp); @@ -627,6 +645,8 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = intel_dig_port->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); + struct pps_registers regs; + int idx; lockdep_assert_held(&dev_priv->pps_mutex); @@ -638,8 +658,10 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) * mapping needs to be retrieved from VBT, for now just hard-code to * use instance #0 always. */ + idx = 0; + if (!intel_dp->pps_reset) - return 0; + return idx; intel_dp->pps_reset = false; @@ -647,9 +669,11 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) * Only the HW needs to be reprogrammed, the SW state is fixed and * has been setup during connector init. */ - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, false); + intel_pps_get_instance_registers(dev_priv, idx, ®s); + intel_dp_init_panel_power_sequencer_instance(dev_priv, intel_dp, false, + ®s); - return 0; + return idx; } typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv, @@ -773,27 +797,12 @@ void intel_power_sequencer_reset(struct drm_i915_private *dev_priv) } } -struct pps_registers { - i915_reg_t pp_ctrl; - i915_reg_t pp_stat; - i915_reg_t pp_on; - i915_reg_t pp_off; - i915_reg_t pp_div; -}; - -static void intel_pps_get_registers(struct drm_i915_private *dev_priv, - struct intel_dp *intel_dp, - struct pps_registers *regs) +static void intel_pps_get_instance_registers(struct drm_i915_private *dev_priv, + int pps_idx, + struct pps_registers *regs) { - int pps_idx = 0; - memset(regs, 0, sizeof(*regs)); - if (IS_GEN9_LP(dev_priv)) - pps_idx = bxt_power_sequencer_idx(intel_dp); - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) - pps_idx = vlv_power_sequencer_pipe(intel_dp); - regs->pp_ctrl = PP_CONTROL(pps_idx); regs->pp_stat = PP_STATUS(pps_idx); regs->pp_on = PP_ON_DELAYS(pps_idx); @@ -802,6 +811,20 @@ static void intel_pps_get_registers(struct drm_i915_private *dev_priv, regs->pp_div = PP_DIVISOR(pps_idx); } +static void intel_pps_get_registers(struct drm_i915_private *dev_priv, + struct intel_dp *intel_dp, + struct pps_registers *regs) +{ + int pps_idx = 0; + + if (IS_GEN9_LP(dev_priv)) + pps_idx = bxt_power_sequencer_idx(intel_dp); + else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) + pps_idx = vlv_power_sequencer_pipe(intel_dp); + + intel_pps_get_instance_registers(dev_priv, pps_idx, regs); +} + static i915_reg_t _pp_ctrl_reg(struct intel_dp *intel_dp) { @@ -5228,21 +5251,18 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev, } static void -intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, - struct intel_dp *intel_dp, - bool force_disable_vdd) +intel_dp_init_panel_power_sequencer_instance(struct drm_i915_private *dev_priv, + struct intel_dp *intel_dp, + bool force_disable_vdd, + const struct pps_registers *regs) { - struct drm_i915_private *dev_priv = to_i915(dev); u32 pp_on, pp_off, pp_div, port_sel = 0; int div = dev_priv->rawclk_freq / 1000; - struct pps_registers regs; enum port port = dp_to_dig_port(intel_dp)->port; const struct edp_power_seq *seq = &intel_dp->pps_delays; lockdep_assert_held(&dev_priv->pps_mutex); - intel_pps_get_registers(dev_priv, intel_dp, ®s); - /* * On some VLV machines the BIOS can leave the VDD * enabled even on power seqeuencers which aren't @@ -5265,7 +5285,7 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, pp &= ~EDP_FORCE_VDD; - I915_WRITE(regs.pp_ctrl, pp); + I915_WRITE(regs->pp_ctrl, pp); } pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) | @@ -5275,7 +5295,7 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, /* Compute the divisor for the pp clock, simply match the Bspec * formula. */ if (IS_GEN9_LP(dev_priv)) { - pp_div = I915_READ(regs.pp_ctrl); + pp_div = I915_READ(regs->pp_ctrl); pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK; pp_div |= (DIV_ROUND_UP((seq->t11_t12 + 1), 1000) << BXT_POWER_CYCLE_DELAY_SHIFT); @@ -5298,19 +5318,32 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, pp_on |= port_sel; - I915_WRITE(regs.pp_on, pp_on); - I915_WRITE(regs.pp_off, pp_off); + I915_WRITE(regs->pp_on, pp_on); + I915_WRITE(regs->pp_off, pp_off); if (IS_GEN9_LP(dev_priv)) - I915_WRITE(regs.pp_ctrl, pp_div); + I915_WRITE(regs->pp_ctrl, pp_div); else - I915_WRITE(regs.pp_div, pp_div); + I915_WRITE(regs->pp_div, pp_div); DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n", - I915_READ(regs.pp_on), - I915_READ(regs.pp_off), + I915_READ(regs->pp_on), + I915_READ(regs->pp_off), IS_GEN9_LP(dev_priv) ? - (I915_READ(regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK) : - I915_READ(regs.pp_div)); + (I915_READ(regs->pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK) : + I915_READ(regs->pp_div)); +} + +static void +intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, + struct intel_dp *intel_dp, + bool force_disable_vdd) +{ + struct drm_i915_private *dev_priv = to_i915(dev); + struct pps_registers regs; + + intel_pps_get_registers(dev_priv, intel_dp, ®s); + intel_dp_init_panel_power_sequencer_instance(dev_priv, intel_dp, + force_disable_vdd, ®s); } static void intel_dp_pps_init(struct drm_device *dev, --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP 2017-06-06 12:34 ` Imre Deak @ 2017-06-06 14:58 ` Bloomfield, Jon 2017-06-06 15:26 ` Imre Deak 0 siblings, 1 reply; 8+ messages in thread From: Bloomfield, Jon @ 2017-06-06 14:58 UTC (permalink / raw) To: Deak, Imre, Jani Nikula; +Cc: intel-gfx, Mustaffa, Mustamin B > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf > Of Imre Deak > Sent: Tuesday, June 6, 2017 5:34 AM > To: Jani Nikula <jani.nikula@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org; Mustaffa, Mustamin B > <mustamin.b.mustaffa@intel.com> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control > for DP > > On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote: > > On Tue, 06 Jun 2017, Mustamin B Mustaffa > <mustamin.b.mustaffa@intel.com> wrote: > > > [...] > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > > > index d1670b8..124f58b 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp > *intel_dp) > > > /* We should never land here with regular DP ports */ > > > WARN_ON(!is_edp(intel_dp)); > > > > > > - /* > > > - * TODO: BXT has 2 PPS instances. The correct port->PPS instance > > > - * mapping needs to be retrieved from VBT, for now just hard-code > to > > > - * use instance #0 always. > > > - */ > > > if (!intel_dp->pps_reset) > > > - return 0; > > > + return dev_priv->vbt.backlight.controller; > > > > So the existing code around here looks a bit convoluted, not least > > because now pretty much all PPS access first does > > > > - intel_pps_get_registers(), which calls > > - bxt_power_sequencer_idx(), which calls > > - intel_dp_init_panel_power_sequencer_registers(), which calls > > - intel_pps_get_registers()... > > > > With your change, for controller == 1 and pps_reset == true, the first > > time the registers are needed, we'll initialize the correct controller 1 > > registers, but controller 0 registers are returned. From there on, we'll > > keep returning controller 1 registers until pps_reset is set to true > > again. > > > > Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS lost > > state after suspend breaking eDP link training") and 8e8232d51878 > > ("drm/i915: Deduplicate PPS register retrieval") which I think create > > the loop. > > A loop would be prevented by the pps->reset check, but agree the code > isn't nice, I guess I overlooked this when I wrote it. To make things > clearer we could factor out a helper from > intel_dp_init_panel_power_sequencer_registers() that takes pps_registers > and call this helper from bxt_power_sequencer_idx(). > > So how about something like the following: Just checking what the intention is here because your proposed change ommits the VBT fix... Are you going to post these changes as a new baseline for Mustaffa's patch ? Or are you asking Mustaffa to fold these changes into his patch ? Hoping it's the former :-) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP 2017-06-06 14:58 ` Bloomfield, Jon @ 2017-06-06 15:26 ` Imre Deak 2017-06-07 8:16 ` Jani Nikula 0 siblings, 1 reply; 8+ messages in thread From: Imre Deak @ 2017-06-06 15:26 UTC (permalink / raw) To: Bloomfield, Jon; +Cc: intel-gfx, Mustaffa, Mustamin B On Tue, Jun 06, 2017 at 05:58:43PM +0300, Bloomfield, Jon wrote: > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf > > Of Imre Deak > > Sent: Tuesday, June 6, 2017 5:34 AM > > To: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Mustaffa, Mustamin B > > <mustamin.b.mustaffa@intel.com> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control > > for DP > > > > On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote: > > > On Tue, 06 Jun 2017, Mustamin B Mustaffa > > <mustamin.b.mustaffa@intel.com> wrote: > > > > [...] > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index d1670b8..124f58b 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp > > *intel_dp) > > > > /* We should never land here with regular DP ports */ > > > > WARN_ON(!is_edp(intel_dp)); > > > > > > > > - /* > > > > - * TODO: BXT has 2 PPS instances. The correct port->PPS instance > > > > - * mapping needs to be retrieved from VBT, for now just hard-code > > to > > > > - * use instance #0 always. > > > > - */ > > > > if (!intel_dp->pps_reset) > > > > - return 0; > > > > + return dev_priv->vbt.backlight.controller; > > > > > > So the existing code around here looks a bit convoluted, not least > > > because now pretty much all PPS access first does > > > > > > - intel_pps_get_registers(), which calls > > > - bxt_power_sequencer_idx(), which calls > > > - intel_dp_init_panel_power_sequencer_registers(), which calls > > > - intel_pps_get_registers()... > > > > > > With your change, for controller == 1 and pps_reset == true, the first > > > time the registers are needed, we'll initialize the correct controller 1 > > > registers, but controller 0 registers are returned. From there on, we'll > > > keep returning controller 1 registers until pps_reset is set to true > > > again. > > > > > > Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS lost > > > state after suspend breaking eDP link training") and 8e8232d51878 > > > ("drm/i915: Deduplicate PPS register retrieval") which I think create > > > the loop. > > > > A loop would be prevented by the pps->reset check, but agree the code > > isn't nice, I guess I overlooked this when I wrote it. To make things > > clearer we could factor out a helper from > > intel_dp_init_panel_power_sequencer_registers() that takes pps_registers > > and call this helper from bxt_power_sequencer_idx(). > > > > So how about something like the following: > > Just checking what the intention is here because your proposed change > ommits the VBT fix... Are you going to post these changes as a new > baseline for Mustaffa's patch ? Or are you asking Mustaffa to fold > these changes into his patch ? Hoping it's the former :-) The change is just to make the code clearer, unrelated to the VBT fix, so it should be a separate patch. I don't mind doing this as a follow-up to Mustaffa's patchset. What his patch here would need is just to return the correct index from bxt_power_sequencer_idx() in all cases. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP 2017-06-06 15:26 ` Imre Deak @ 2017-06-07 8:16 ` Jani Nikula 2017-06-07 14:12 ` Bloomfield, Jon 0 siblings, 1 reply; 8+ messages in thread From: Jani Nikula @ 2017-06-07 8:16 UTC (permalink / raw) To: imre.deak, Bloomfield, Jon; +Cc: intel-gfx, Mustaffa, Mustamin B On Tue, 06 Jun 2017, Imre Deak <imre.deak@intel.com> wrote: > On Tue, Jun 06, 2017 at 05:58:43PM +0300, Bloomfield, Jon wrote: >> > -----Original Message----- >> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf >> > Of Imre Deak >> > Sent: Tuesday, June 6, 2017 5:34 AM >> > To: Jani Nikula <jani.nikula@linux.intel.com> >> > Cc: intel-gfx@lists.freedesktop.org; Mustaffa, Mustamin B >> > <mustamin.b.mustaffa@intel.com> >> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control >> > for DP >> > >> > On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote: >> > > On Tue, 06 Jun 2017, Mustamin B Mustaffa >> > <mustamin.b.mustaffa@intel.com> wrote: >> > > > [...] >> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c >> > b/drivers/gpu/drm/i915/intel_dp.c >> > > > index d1670b8..124f58b 100644 >> > > > --- a/drivers/gpu/drm/i915/intel_dp.c >> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > > > @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp >> > *intel_dp) >> > > > /* We should never land here with regular DP ports */ >> > > > WARN_ON(!is_edp(intel_dp)); >> > > > >> > > > - /* >> > > > - * TODO: BXT has 2 PPS instances. The correct port->PPS instance >> > > > - * mapping needs to be retrieved from VBT, for now just hard-code >> > to >> > > > - * use instance #0 always. >> > > > - */ >> > > > if (!intel_dp->pps_reset) >> > > > - return 0; >> > > > + return dev_priv->vbt.backlight.controller; >> > > >> > > So the existing code around here looks a bit convoluted, not least >> > > because now pretty much all PPS access first does >> > > >> > > - intel_pps_get_registers(), which calls >> > > - bxt_power_sequencer_idx(), which calls >> > > - intel_dp_init_panel_power_sequencer_registers(), which calls >> > > - intel_pps_get_registers()... >> > > >> > > With your change, for controller == 1 and pps_reset == true, the first >> > > time the registers are needed, we'll initialize the correct controller 1 >> > > registers, but controller 0 registers are returned. From there on, we'll >> > > keep returning controller 1 registers until pps_reset is set to true >> > > again. >> > > >> > > Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS lost >> > > state after suspend breaking eDP link training") and 8e8232d51878 >> > > ("drm/i915: Deduplicate PPS register retrieval") which I think create >> > > the loop. >> > >> > A loop would be prevented by the pps->reset check, but agree the code >> > isn't nice, I guess I overlooked this when I wrote it. To make things >> > clearer we could factor out a helper from >> > intel_dp_init_panel_power_sequencer_registers() that takes pps_registers >> > and call this helper from bxt_power_sequencer_idx(). >> > >> > So how about something like the following: >> >> Just checking what the intention is here because your proposed change >> ommits the VBT fix... Are you going to post these changes as a new >> baseline for Mustaffa's patch ? Or are you asking Mustaffa to fold >> these changes into his patch ? Hoping it's the former :-) > > The change is just to make the code clearer, unrelated to the VBT fix, > so it should be a separate patch. I don't mind doing this as a follow-up > to Mustaffa's patchset. What his patch here would need is just to return > the correct index from bxt_power_sequencer_idx() in all cases. I think we might need to backport Mustaffa's patch to stable so we need to do that first as a standalone change. After it has been fixed according to Imre's and my feedback. Oh, and I'd still like someone(tm) to check if the PPS-PWM mapping is fixed 1:1, or can we cross connect them? I just involved Imre here because the existing code is, I think, unnecessarily hard to follow. 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] 8+ messages in thread
* Re: [PATCH] drm/i915/bxt: Enable VBT based BL control for DP 2017-06-07 8:16 ` Jani Nikula @ 2017-06-07 14:12 ` Bloomfield, Jon 0 siblings, 0 replies; 8+ messages in thread From: Bloomfield, Jon @ 2017-06-07 14:12 UTC (permalink / raw) To: Jani Nikula, Deak, Imre; +Cc: intel-gfx, Mustaffa, Mustamin B > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Wednesday, June 7, 2017 1:16 AM > To: Deak, Imre <imre.deak@intel.com>; Bloomfield, Jon > <jon.bloomfield@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Mustaffa, Mustamin B > <mustamin.b.mustaffa@intel.com> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL control > for DP > > On Tue, 06 Jun 2017, Imre Deak <imre.deak@intel.com> wrote: > > On Tue, Jun 06, 2017 at 05:58:43PM +0300, Bloomfield, Jon wrote: > >> > -----Original Message----- > >> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > Behalf > >> > Of Imre Deak > >> > Sent: Tuesday, June 6, 2017 5:34 AM > >> > To: Jani Nikula <jani.nikula@linux.intel.com> > >> > Cc: intel-gfx@lists.freedesktop.org; Mustaffa, Mustamin B > >> > <mustamin.b.mustaffa@intel.com> > >> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/bxt: Enable VBT based BL > control > >> > for DP > >> > > >> > On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote: > >> > > On Tue, 06 Jun 2017, Mustamin B Mustaffa > >> > <mustamin.b.mustaffa@intel.com> wrote: > >> > > > [...] > >> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > >> > b/drivers/gpu/drm/i915/intel_dp.c > >> > > > index d1670b8..124f58b 100644 > >> > > > --- a/drivers/gpu/drm/i915/intel_dp.c > >> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > >> > > > @@ -591,13 +591,8 @@ bxt_power_sequencer_idx(struct intel_dp > >> > *intel_dp) > >> > > > /* We should never land here with regular DP ports */ > >> > > > WARN_ON(!is_edp(intel_dp)); > >> > > > > >> > > > - /* > >> > > > - * TODO: BXT has 2 PPS instances. The correct port->PPS > instance > >> > > > - * mapping needs to be retrieved from VBT, for now just > hard-code > >> > to > >> > > > - * use instance #0 always. > >> > > > - */ > >> > > > if (!intel_dp->pps_reset) > >> > > > - return 0; > >> > > > + return dev_priv->vbt.backlight.controller; > >> > > > >> > > So the existing code around here looks a bit convoluted, not least > >> > > because now pretty much all PPS access first does > >> > > > >> > > - intel_pps_get_registers(), which calls > >> > > - bxt_power_sequencer_idx(), which calls > >> > > - intel_dp_init_panel_power_sequencer_registers(), which calls > >> > > - intel_pps_get_registers()... > >> > > > >> > > With your change, for controller == 1 and pps_reset == true, the first > >> > > time the registers are needed, we'll initialize the correct controller 1 > >> > > registers, but controller 0 registers are returned. From there on, we'll > >> > > keep returning controller 1 registers until pps_reset is set to true > >> > > again. > >> > > > >> > > Cc: Imre as author of commits 78597996370c ("drm/i915/bxt: Fix PPS > lost > >> > > state after suspend breaking eDP link training") and 8e8232d51878 > >> > > ("drm/i915: Deduplicate PPS register retrieval") which I think create > >> > > the loop. > >> > > >> > A loop would be prevented by the pps->reset check, but agree the code > >> > isn't nice, I guess I overlooked this when I wrote it. To make things > >> > clearer we could factor out a helper from > >> > intel_dp_init_panel_power_sequencer_registers() that takes > pps_registers > >> > and call this helper from bxt_power_sequencer_idx(). > >> > > >> > So how about something like the following: > >> > >> Just checking what the intention is here because your proposed change > >> ommits the VBT fix... Are you going to post these changes as a new > >> baseline for Mustaffa's patch ? Or are you asking Mustaffa to fold > >> these changes into his patch ? Hoping it's the former :-) > > > > The change is just to make the code clearer, unrelated to the VBT fix, > > so it should be a separate patch. I don't mind doing this as a follow-up > > to Mustaffa's patchset. What his patch here would need is just to return > > the correct index from bxt_power_sequencer_idx() in all cases. > > I think we might need to backport Mustaffa's patch to stable so we need > to do that first as a standalone change. After it has been fixed > according to Imre's and my feedback. Oh, and I'd still like someone(tm) > to check if the PPS-PWM mapping is fixed 1:1, or can we cross connect > them? Yes, I was having doubts about this yesterday too. I can find nothing the BSpec to indicate any relationship at all between PWM and PPS. Mustamin is from IOTG and this for a specific product they're working on. The correct fix is probably to extend VBT to include a separate PPS select field and then key off that. As this is a new product, there should be no issues with updating the VBT (I hope), but how does that sit with you guys ? Hardcoding is certainly plain wrong, even if all current released products do need 0. > > I just involved Imre here because the existing code is, I think, > unnecessarily hard to follow. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-07 14:12 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-06 7:09 [PATCH] drm/i915/bxt: Enable VBT based BL control for DP Mustamin B Mustaffa 2017-06-06 7:28 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-06-06 9:24 ` [PATCH] " Jani Nikula 2017-06-06 12:34 ` Imre Deak 2017-06-06 14:58 ` Bloomfield, Jon 2017-06-06 15:26 ` Imre Deak 2017-06-07 8:16 ` Jani Nikula 2017-06-07 14:12 ` Bloomfield, Jon
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.