[Intel-gfx] [PATCH 19/19] drm/i915: init the DP panel power seq regs earlier

[Paulo Zanoni]

2013/12/5 Jani Nikula <jani.nikula at linux.intel.com>:
> On Thu, 21 Nov 2013, Paulo Zanoni <przanoni at gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> When we call intel_dp_i2c_init we already get some I2C calls, which
>> will trigger a VDD enable, which will make use of the panel power
>> sequencing registers, so we need to have them ready by this time.
>
> But this patch won't make the registers ready either! The only thing
> this one does is initialize the delays in intel_dp. (And unlock the
> registers, but that's done in the vdd enable function too.)

Facepalm. You're right. I was mainly looking at the msleep() calls,
which are fixed by this patch. Besides this, we also need the
registers to be correct, but I guess this should be a separate patch.

>
> And you actually can't trivially initialize the registers either,
> because we will only later figure out whether this is a ghost eDP, and
> such initialization might botch up LVDS.

I wonder if we shouldn't at least try to do the right thing on HSW+,
since there's no LVDS there. But then there's the "eDP on port D" case
which we can't forget.


>
> See
> commit f30d26e468322b50d5e376bec40658683aff8ece
> Author: Jani Nikula <jani.nikula at intel.com>
> Date:   Wed Jan 16 10:53:40 2013 +0200
>
>     drm/i915/eDP: do not write power sequence registers for ghost eDP
>
> I'm sorry I don't readily have any suggestions.
>
> If you are only interested in fixing the delays for msleep, then please
> fix the commit message!

Yeah, I'll fix the commit message for now, but I'll also think about
what to do with the registers.

Thanks,
Paulo

>
> BR,
> Jani.
>
>
>
>>
>> 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 at intel.com>
>>     Date:   Wed Jun 12 17:27:24 2013 -0300
>>         drm/i915: extract intel_edp_init_connector
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 3a1ca80..23927a0 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3565,14 +3565,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;
>> @@ -3580,8 +3580,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);
>> @@ -3599,8 +3597,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) {
>> @@ -3649,6 +3646,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;
>>
>> @@ -3748,13 +3746,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);
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Paulo Zanoni