On Tue, Jun 06, 2017 at 12:24:30PM +0300, Jani Nikula wrote:
> On Tue, 06 Jun 2017, Mustamin B Mustaffa <mustamin.b.mustaffa@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
