On Thu, 19 Dec 2013 14:29:39 -0200
Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Date: Wed Jun 12 17:27:24 2013 -0300
> drm/i915: extract intel_edp_init_connector
>
> v2: - Rewrite commit message.
>
> Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> 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 7df5085..9d96447 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3532,14 +3532,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;
> @@ -3547,8 +3547,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);
> @@ -3566,8 +3564,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) {
> @@ -3616,6 +3613,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;
>
> @@ -3699,13 +3697,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);
Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
