On Tue, Jun 18, 2019 at 10:30:06AM -0700, Matt Roper wrote:
> On Tue, Jun 18, 2019 at 07:08:55PM +0300, Ville Syrjälä wrote:
> > On Mon, Jun 17, 2019 at 04:48:10PM -0700, Matt Roper wrote:
> > > EHL has a mux on combo PHY A that allows it to be driven either by an
> > > internal display (DDI-A or DSI DPHY) or by an external display (DDI-D).
> > > This is a motherboard design decision that can not be changed on the
> > > fly. Unfortunately there are no strap registers that allow us to detect
> > > the board configuration directly, so let's use the VBT to try to figure
> > > it out and program the mux accordingly.
> > >
> > > v2:
> > > - Confirmed that VBT's dvo port refers to the DDI and not the PHY.
> > > Thus we can check more explicitly for (ddi_d && !(ddi_a || dsi)). If
> > > a bad VBT contradicts itself, let internal display win. (Ville)
> > >
> > > Cc: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_bios.c | 3 ++
> > > .../gpu/drm/i915/display/intel_combo_phy.c | 36 +++++++++++++++++++
> > > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > > 3 files changed, 40 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > > index c4710889cb32..0c9808132d67 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > > @@ -1668,6 +1668,9 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
> > > if (!child->device_type)
> > > continue;
> > >
> > > + DRM_DEBUG_KMS("Found VBT child device with type 0x%x\n",
> > > + child->device_type);
> > > +
> >
> > Was this hunk intentional?
> >
>
> Yeah, I figured that having a little more detail on the child device
> details will probably be useful for debugging any cases where the VBT
> seems to contradict itself. I'll add a note about the debug message to
> the commit message to make it clear it's intentional.
>
>
> > > /*
> > > * Copy as much as we know (sizeof) and is available
> > > * (child_dev_size) of the child device. Accessing the data must
> > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > index 841708da5a56..c18079a09a2e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > @@ -260,6 +260,32 @@ void intel_combo_phy_power_up_lanes(struct drm_i915_private *dev_priv,
> > > I915_WRITE(ICL_PORT_CL_DW10(port), val);
> > > }
> > >
> > > +static u32 ehl_combo_phy_a_mux(struct drm_i915_private *i915, u32 val)
> > > +{
> > > + bool ddi_a_present = i915->vbt.ddi_port_info[PORT_A].child != NULL;
> > > + bool ddi_d_present = i915->vbt.ddi_port_info[PORT_D].child != NULL;
> > > + bool dsi_present = intel_bios_is_dsi_present(i915, NULL);
> > > +
> > > + /*
> > > + * VBT's 'dvo port' field for child devices references the DDI, not
> > > + * the PHY. So if combo PHY A is wired up to drive an external
> > > + * display, we should see a child device present on PORT_D and
> > > + * nothing on PORT_A and no DSI.
> > > + */
> > > + if (ddi_d_present && !ddi_a_present && !dsi_present)
> > > + return val | ICL_PHY_MISC_MUX_DDID;
> > > +
> > > + /*
> > > + * If we encounter a VBT that claims to have an external display on
> > > + * DDI-D _and_ an internal display on DDI-A/DSI leave an error message
> > > + * in the log and let the internal display win.
> > > + */
> > > + if (ddi_d_present)
> > > + DRM_ERROR("VBT claims to have both internal and external displays on PHY A. Configuring for internal.\n");
> > > +
> > > + return val & ~ICL_PHY_MISC_MUX_DDID;
> > > +}
> > > +
> > > static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > > {
> > > enum port port;
> > > @@ -273,7 +299,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > > continue;
> > > }
> > >
> > > + /*
> > > + * EHL's combo PHY A can be hooked up to either an external
> > > + * display (via DDI-D) or an internal display (via DDI-A or
> > > + * the DSI DPHY). This is a motherboard design decision that
> > > + * can't be changed on the fly, so initialize the PHY's mux
> > > + * based on whether our VBT indicates the presence of any
> > > + * "internal" child devices.
> > > + */
> > > val = I915_READ(ICL_PHY_MISC(port));
> > > + if (!IS_ICELAKE(dev_priv) && port == PORT_A)
> >
> > Maybe IS_EHL instead?
>
> That's how I was originally going to write it, but I switched it to !icl
> to follow the general convention of "assume future platforms inherit all
> behavior of current platform."
This feels like a one off cost saving measure to me. So not entirely
sure it makes sense to assume future platforms would follow this
apporach.
>
>
> Matt
>
> >
> > Patch is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > > + val = ehl_combo_phy_a_mux(dev_priv, val);
> > > val &= ~ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
> > > I915_WRITE(ICL_PHY_MISC(port), val);
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1ae36b9efc85..68afafafd312 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -11138,6 +11138,7 @@ enum skl_power_gate {
> > > #define _ICL_PHY_MISC_B 0x64C04
> > > #define ICL_PHY_MISC(port) _MMIO_PORT(port, _ICL_PHY_MISC_A, \
> > > _ICL_PHY_MISC_B)
> > > +#define ICL_PHY_MISC_MUX_DDID (1 << 28)
> > > #define ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN (1 << 23)
> > >
> > > /* Icelake Display Stream Compression Registers */
> > > --
> > > 2.17.2
> >
> > --
> > Ville Syrjälä
> > Intel
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
