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