| Message ID | 20190617234810.25874-1-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/ehl: Allow combo PHY A to drive a third external display | expand |
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@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > 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? > /* > * 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? Patch is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + 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
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@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > 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." Matt > > Patch is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > + 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
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@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > 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@linux.intel.com> > > > > > + 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
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); + /* * 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) + 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 */
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@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- 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(+)