Re: [Intel-gfx] [PATCH] drm/i915/bios: gracefully disable dual eDP for now

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 05, 2021 at 10:30:04PM +0300, Jani Nikula wrote:
> On Tue, 05 Oct 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Oct 05, 2021 at 08:56:36PM +0300, Jani Nikula wrote:
> >> For the time being, neither the power sequencer nor the backlight code
> >> properly support two eDP panels simultaneously. While the software
> >> states will be independent, the same sets of registers will be used for
> >> both eDP panels, clobbering the hardware state and leading to errors.
> >> 
> >> Gracefully disable dual eDP until proper support has been added.
> >> 
> >> Cc: José Roberto de Souza <jose.souza@intel.com>
> >> Cc: Uma Shankar <uma.shankar@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Swati Sharma <swati2.sharma@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_bios.c | 47 +++++++++++++++++++++++
> >>  1 file changed, 47 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> index f9776ca85de3..b99907c656bb 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> @@ -1930,6 +1930,50 @@ static int _intel_bios_max_tmds_clock(const struct intel_bios_encoder_data *devd
> >>  	}
> >>  }
> >>  
> >> +static enum port get_edp_port(struct drm_i915_private *i915)
> >> +{
> >> +	const struct intel_bios_encoder_data *devdata;
> >> +	enum port port;
> >> +
> >> +	for_each_port(port) {
> >> +		devdata = i915->vbt.ports[port];
> >> +
> >> +		if (devdata && intel_bios_encoder_supports_edp(devdata))
> >> +			return port;
> >> +	}
> >> +
> >> +	return PORT_NONE;
> >> +}
> >> +
> >> +/*
> >> + * FIXME: The power sequencer and backlight code currently do not support more
> >> + * than one set registers, at least not on anything other than VLV/CHV. It will
> >> + * clobber the registers. As a temporary workaround, gracefully prevent more
> >> + * than one eDP from being registered.
> >> + */
> >> +static void sanitize_dual_edp(struct intel_bios_encoder_data *devdata,
> >> +			      enum port port)
> >> +{
> >> +	struct drm_i915_private *i915 = devdata->i915;
> >> +	struct child_device_config *child = &devdata->child;
> >> +	enum port p;
> >> +
> >> +	/* CHV might not clobber PPS registers. */
> >> +	if (IS_CHERRYVIEW(i915))
> >
> > vlv and chv should both behave identically. At least I don't remember
> > any single eDP assumptions in the code for either.
> 
> This bit of code is not run on VLV, only CHV and DDI. It's subtle.

Oh right. Maybe we should just flip the switch for VLV. VLV and
HSW did come out around the same time I think, and I have some
vbt specs which are labeled as VLV/HSW. So if it works for HSW
and CHV then I think there's a fair chance VLV will be fine.

> 
> > Hmm. Quick glance suggest bxt/glk should handle this correctly
> > as well? But the more recent platforms are certainly borked.
> > Well, that's assuming the vbt related bits are correct for bxt/glk.
> 
> VLV/CHV figure out the PPS in a complicated manner, and use pipe
> specific backlight. They might work.

Yeah, the assignment only depends on the current pipe driving the port.
Which is a horrible design, but kinda works after you deal with all
the madness (the pps kick stuff).

> 
> BXT/GLK look at VBT for the pps/backlight index, but that's just the
> *one* number. All the structures are set up nicely, but then they use
> the same set of registers for all panels.

Ah, right. So the low level mechanism is there, just not utilized.

> The recent failure mode was a really weird looking VDD warn, and it just
> turned out to be two intel_pps instances using the same registers and
> getting royally confused about the sw/hw states.
> 
> We'd need to figure out the per-panel pps/backlight to use from VBT, for
> each panel, and then set that up.

I guess one concern is which of the listed eDP ports we're going to
nuke, and does the port that is left have some chance of working.
Hard to know I guess. But at least the bug report should be a clear
"my display isn't lighting up!" type of thing.

Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel