Re: [PATCH] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

Imre Deak
2016-06-20T13:56:14Z

On ma, 2016-06-20 at 13:39 +0300, Ville Syrjälä wrote:
> On Fri, Jun 17, 2016 at 06:48:45PM +0300, Imre Deak wrote:
> > Atm on ILK we attempt to detect if eDP is present even if LVDS was
> > already detected and an encoder for it was registered. This involves
> > trying to read out the eDP EDID, which in turn needs the same power
> > sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> > may or may not interfere with the LVDS panel, but it's probably safer to
> > prevent this.  Registering both an LVDS and an eDP connector would also
> > present a similar problem accessing the shared PPS at any point later in
> > an unexpected way.
> >
> > This was caught by CI with the PPS sanity checks in place and the
> > initial eDP EDID readout waiting for the panel power cycle timeout
> > without the PPS registers being initialized. To solve this latter
> > problem move PPS register init earlier before the first use of the
> > PPS.
> >
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index be08351..fe543d7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5316,8 +5316,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >   if (!is_edp(intel_dp))
> >   return true;
> >
> > + /*
> > +  * On ILK we may get here with LVDS already registered. Since the
>
> ILK,SNB,IVB, if you want to accurately list the platforms.

Oops. I made the wrong assumption that IBX/CPT=ILK. Will fix this.

>
> > +  * driver uses the only internal power sequencer available for both
> > +  * eDP and LVDS bail out early in this case to prevent interfering
> > +  * with an already powered-on LVDS power sequencer.
> > +  */
> > + for_each_intel_encoder(dev, intel_encoder) {
> > + if (intel_encoder->type == INTEL_OUTPUT_LVDS) {
> > + DRM_INFO("LVDS was detected, not registering eDP\n");
> > + return false;
> > + }
> > + }
>
> We could abort much earlier, in the encoder init.

Yes, with an is_edp() check. Although preserving eDP detect for a non-
active LVDS output as discussed with Chris, intel_edp_init_connector()
would have to know about such an LVDS output to save/restore the PPS
state. Do you still want me then to move this earlier passing a flag to
intel_edp_init_connector()?

> > +
> >   pps_lock(intel_dp);
> >   intel_edp_panel_vdd_sanitize(intel_dp);
> > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> >   pps_unlock(intel_dp);
>
> For VLV/CHV we do this stuff in the encoder init already. Maybe we
> should move all that stuff here as well?

Ok, I did this:
https://github.com/ideak/linux/commit/72a224980515a87d6102c0f5c63306d35b337efe

AUX should be also inited after PPS setup, so I also moved that after PPS setup.
Ideally we had separate AUX init/register phases, then we could init AUX upfront
for both DP and eDP and register it after eDP init. B/c of the AUX/I2C
register/init is also conflated atm, this needs more work, I think Chris has
already something to separate those.

> >
> >   /* Cache DPCD and EDID for edp. */
> > @@ -5334,11 +5348,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >   return false;
> >   }
> >
> > - /* We now know it's not a ghost, init power sequence regs. */
> > - pps_lock(intel_dp);
> > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> > - pps_unlock(intel_dp);
> > -
> >   mutex_lock(&dev->mode_config.mutex);
> >   edid = drm_get_edid(connector, &intel_dp->aux.ddc);
> >   if (edid) {
> > --
> > 2.5.0