| Message ID | 1386358364-1539-6-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 6 Dec 2013 17:32:44 -0200 Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Our driver has two different ways of waiting for panel power > sequencing delays. One of these ways is through > ironlake_wait_panel_status, which implicitly uses the values written > to our registers. The other way is through the functions that call > intel_wait_until_after, and on this case we do direct msleep() calls > on the intel_dp->xxx_delay variables. > > Function intel_dp_init_panel_power_sequencer is responsible for > initializing the _delay variables and deciding which values we need to > write to the registers, but it does not write these values to the > registers. Only at intel_dp_init_panel_power_sequencer_registers we > actually do this write. > > Then problem is that when we call intel_dp_i2c_init, we will get some > I2C calls, which will trigger a VDD enable, which will make use of the > panel power sequencing registers and the _delay variables, so we need > to have both ready by this time. Today, when this happens, the _delay > variables are zero (because they were not computed) and the panel > power sequence registers contain whatever values were written by the > BIOS (which are usually correct). > > What this patch does is to make sure that function > intel_dp_init_panel_power_sequencer is called earlier, so by the time > we call intel_dp_i2c_init, the _delay variables will already be > initialized. The actual registers won't contain their final values, > but at least they will contain the values set by the BIOS. > > The good side is that we were reading the values, but were not using > them for anything (because we were just skipping the msleep(0) calls), > so this "fix" shouldn't fix any real existing bugs. I was only able to > identify the problem because I added some debug code to check how much > time time we were saving with my previous patch. > > Regression introduced by: > commit ed92f0b239ac971edc509169ae3d6955fbe0a188 > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > Date: Wed Jun 12 17:27:24 2013 -0300 > drm/i915: extract intel_edp_init_connector > > v2: - Rewrite commit message. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 79f7ec2..9cc5819 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3540,14 +3540,14 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, > } > > static bool intel_edp_init_connector(struct intel_dp *intel_dp, > - struct intel_connector *intel_connector) > + struct intel_connector *intel_connector, > + struct edp_power_seq *power_seq) > { > struct drm_connector *connector = &intel_connector->base; > 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 = dev->dev_private; > struct drm_display_mode *fixed_mode = NULL; > - struct edp_power_seq power_seq = { 0 }; > bool has_dpcd; > struct drm_display_mode *scan; > struct edid *edid; > @@ -3555,8 +3555,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > if (!is_edp(intel_dp)) > return true; > > - intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); > - > /* Cache DPCD and EDID for edp. */ > ironlake_edp_panel_vdd_on(intel_dp); > has_dpcd = intel_dp_get_dpcd(intel_dp); > @@ -3574,8 +3572,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > } > > /* We now know it's not a ghost, init power sequence regs. */ > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, > - &power_seq); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq); > > edid = drm_get_edid(connector, &intel_dp->adapter); > if (edid) { > @@ -3624,6 +3621,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > struct drm_device *dev = intel_encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > enum port port = intel_dig_port->port; > + struct edp_power_seq power_seq = { 0 }; > const char *name = NULL; > int type, error; > > @@ -3707,13 +3705,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > BUG(); > } > > + if (is_edp(intel_dp)) > + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); > + > error = intel_dp_i2c_init(intel_dp, intel_connector, name); > WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n", > error, port_name(port)); > > intel_dp->psr_setup_done = false; > > - if (!intel_edp_init_connector(intel_dp, intel_connector)) { > + if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) { > i2c_del_adapter(&intel_dp->adapter); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); Yeah, seems reasonable. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 79f7ec2..9cc5819 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3540,14 +3540,14 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, } static bool intel_edp_init_connector(struct intel_dp *intel_dp, - struct intel_connector *intel_connector) + struct intel_connector *intel_connector, + struct edp_power_seq *power_seq) { struct drm_connector *connector = &intel_connector->base; 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 = dev->dev_private; struct drm_display_mode *fixed_mode = NULL; - struct edp_power_seq power_seq = { 0 }; bool has_dpcd; struct drm_display_mode *scan; struct edid *edid; @@ -3555,8 +3555,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, if (!is_edp(intel_dp)) return true; - intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); - /* Cache DPCD and EDID for edp. */ ironlake_edp_panel_vdd_on(intel_dp); has_dpcd = intel_dp_get_dpcd(intel_dp); @@ -3574,8 +3572,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, } /* We now know it's not a ghost, init power sequence regs. */ - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, - &power_seq); + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq); edid = drm_get_edid(connector, &intel_dp->adapter); if (edid) { @@ -3624,6 +3621,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, struct drm_device *dev = intel_encoder->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; enum port port = intel_dig_port->port; + struct edp_power_seq power_seq = { 0 }; const char *name = NULL; int type, error; @@ -3707,13 +3705,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, BUG(); } + if (is_edp(intel_dp)) + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); + error = intel_dp_i2c_init(intel_dp, intel_connector, name); WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n", error, port_name(port)); intel_dp->psr_setup_done = false; - if (!intel_edp_init_connector(intel_dp, intel_connector)) { + if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) { i2c_del_adapter(&intel_dp->adapter); if (is_edp(intel_dp)) { cancel_delayed_work_sync(&intel_dp->panel_vdd_work);