| Message ID | 20180522002558.29262-21-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Paulo Zanoni >Sent: Monday, May 21, 2018 5:26 PM >To: intel-gfx@lists.freedesktop.org >Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com> >Subject: [Intel-gfx] [PATCH 20/24] drm/i915/icl: implement the tc/legacy HPD >{dis, }connect flow for DP > >Implement the DFLEXDPPMS/DFLEXDPCSSS dance for DisplayPort. These >functions need to be called during HPD assert/deassert, but due to how our driver >works it's much simpler if we always call them when >icl_digital_port_connected() is called, which means we won't call them exactly >once per HPD event. This should also cover the connected boot case, whatever >the BIOS does. > >We're still missing the HDMI case, which should be implemented in the next >patch. > >Also notice that, today, the BSpec pages for the DFLEXDPPMS and DFLEXDPCSSS >registers are wrong, so you should only trust the flows described by the "Gen11 >TypeC Programming" page in our spec. > >Cc: Animesh Manna <animesh.manna@intel.com> >Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >--- > drivers/gpu/drm/i915/i915_reg.h | 6 +++++ drivers/gpu/drm/i915/intel_dp.c | >57 ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 62 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >index 24308d4435f5..42cbace4c61e 100644 >--- a/drivers/gpu/drm/i915/i915_reg.h >+++ b/drivers/gpu/drm/i915/i915_reg.h >@@ -10027,4 +10027,10 @@ enum skl_power_gate { > _ICL_PHY_MISC_B) > #define ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN (1 << 23) > >+#define PORT_TX_DFLEXDPPMS > _MMIO(0x163890) >+#define DP_PHY_MODE_STATUS_COMPLETED(tc_port) (1 << >(tc_port)) >+ >+#define PORT_TX_DFLEXDPCSSS > _MMIO(0x163894) >+#define DP_PHY_MODE_STATUS_NOT_SAFE(tc_port) (1 << (tc_port)) >+ > #endif /* _I915_REG_H_ */ >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >index f3d5b9eed625..f25f871e7c22 100644 >--- a/drivers/gpu/drm/i915/intel_dp.c >+++ b/drivers/gpu/drm/i915/intel_dp.c >@@ -4762,6 +4762,56 @@ static void icl_update_tc_port_type(struct >drm_i915_private *dev_priv, > type_str); > } > >+static bool icl_tc_phy_mode_status_connect(struct drm_i915_private *dev_priv, >+ struct intel_digital_port *dig_port) { >+ enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); >+ u32 val; >+ >+ if (dig_port->tc_type != TC_PORT_LEGACY && >+ dig_port->tc_type != TC_PORT_TYPEC) >+ return true; Shouldn’t this return false? >+ val = I915_READ(PORT_TX_DFLEXDPPMS); >+ if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { >+ DRM_ERROR("DP PHY for TC port %d not ready\n", tc_port); >+ return false; >+ } >+ >+ /* >+ * This function may be called many times in a row without an HPD event >+ * in between, so try to avoid the write when we can. >+ */ >+ val = I915_READ(PORT_TX_DFLEXDPCSSS); >+ if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) { >+ val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); >+ I915_WRITE(PORT_TX_DFLEXDPCSSS, val); >+ } >+ >+ return true; >+} >+ >+static void icl_tc_phy_mode_status_disconnect(struct drm_i915_private >*dev_priv, >+ struct intel_digital_port *dig_port) { >+ enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); >+ u32 val; >+ >+ if (dig_port->tc_type != TC_PORT_LEGACY && >+ dig_port->tc_type != TC_PORT_TYPEC) >+ return; >+ >+ /* >+ * This function may be called many times in a row without an HPD event >+ * in between, so try to avoid the write when we can. >+ */ So, in the sequences to enable, it does tell that enabling suitable aux power domains is optional. But in the disable sequence, disable AUX_PWR is mentioned as a non-optional step. In which case it has to be before we set DFLEXDPCSSS register to 0. Is it being addressed in another patch? The rest of the patch looks good. Anusha >+ val = I915_READ(PORT_TX_DFLEXDPCSSS); >+ if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) { >+ val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); >+ I915_WRITE(PORT_TX_DFLEXDPCSSS, val); >+ } >+} >+ > static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > struct intel_digital_port *intel_dig_port) { @@ >-4782,12 +4832,17 @@ static bool icl_tc_port_connected(struct >drm_i915_private *dev_priv, > if (cpu_isr & tbt_bit) > is_tbt = true; > >- if (!is_legacy && !is_typec && !is_tbt) >+ if (!is_legacy && !is_typec && !is_tbt) { >+ icl_tc_phy_mode_status_disconnect(dev_priv, intel_dig_port); > return false; >+ } > > icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec, > is_tbt); > >+ if (!icl_tc_phy_mode_status_connect(dev_priv, intel_dig_port)) >+ return false; >+ > return true; > } > >-- >2.14.3 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2018-06-21 às 15:04 -0700, Srivatsa, Anusha escreveu: > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > > Behalf Of > > Paulo Zanoni > > Sent: Monday, May 21, 2018 5:26 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com> > > Subject: [Intel-gfx] [PATCH 20/24] drm/i915/icl: implement the > > tc/legacy HPD > > {dis, }connect flow for DP > > > > Implement the DFLEXDPPMS/DFLEXDPCSSS dance for DisplayPort. These > > functions need to be called during HPD assert/deassert, but due to > > how our driver > > works it's much simpler if we always call them when > > icl_digital_port_connected() is called, which means we won't call > > them exactly > > once per HPD event. This should also cover the connected boot case, > > whatever > > the BIOS does. > > > > We're still missing the HDMI case, which should be implemented in > > the next > > patch. > > > > Also notice that, today, the BSpec pages for the DFLEXDPPMS and > > DFLEXDPCSSS > > registers are wrong, so you should only trust the flows described > > by the "Gen11 > > TypeC Programming" page in our spec. > > > > Cc: Animesh Manna <animesh.manna@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 6 > > +++++ drivers/gpu/drm/i915/intel_dp.c | > > 57 ++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 62 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 24308d4435f5..42cbace4c61e 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -10027,4 +10027,10 @@ enum skl_power_gate { > > _ICL_PHY_MISC_B) > > #define ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN (1 << 23) > > > > +#define PORT_TX_DFLEXDPPMS > > _MMIO(0x163890) > > +#define DP_PHY_MODE_STATUS_COMPLETED(tc_port) (1 > > << > > (tc_port)) > > + > > +#define PORT_TX_DFLEXDPCSSS > > _MMIO(0x163894) > > +#define DP_PHY_MODE_STATUS_NOT_SAFE(tc_port) (1 > > << (tc_port)) > > + > > #endif /* _I915_REG_H_ */ > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index f3d5b9eed625..f25f871e7c22 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4762,6 +4762,56 @@ static void icl_update_tc_port_type(struct > > drm_i915_private *dev_priv, > > type_str); > > } > > > > +static bool icl_tc_phy_mode_status_connect(struct drm_i915_private > > *dev_priv, > > + struct > > intel_digital_port *dig_port) { > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, > > dig_port->base.port); > > + u32 val; > > + > > + if (dig_port->tc_type != TC_PORT_LEGACY && > > + dig_port->tc_type != TC_PORT_TYPEC) > > + return true; > > Shouldn’t this return false? Types that don't need to run this sequence can be considered connected, so we return true. > > > + val = I915_READ(PORT_TX_DFLEXDPPMS); > > + if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { > > + DRM_ERROR("DP PHY for TC port %d not ready\n", > > tc_port); > > + return false; > > + } > > + > > + /* > > + * This function may be called many times in a row without > > an HPD event > > + * in between, so try to avoid the write when we can. > > + */ > > + val = I915_READ(PORT_TX_DFLEXDPCSSS); > > + if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) { > > + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > > + } > > + > > + return true; > > +} > > + > > +static void icl_tc_phy_mode_status_disconnect(struct > > drm_i915_private > > *dev_priv, > > + struct > > intel_digital_port *dig_port) { > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, > > dig_port->base.port); > > + u32 val; > > + > > + if (dig_port->tc_type != TC_PORT_LEGACY && > > + dig_port->tc_type != TC_PORT_TYPEC) > > + return; > > + > > + /* > > + * This function may be called many times in a row without > > an HPD event > > + * in between, so try to avoid the write when we can. > > + */ > > So, in the sequences to enable, it does tell that enabling suitable > aux power domains is optional. But in the disable sequence, disable > AUX_PWR is mentioned as a non-optional step. > In which case it has to be before we set DFLEXDPCSSS register to 0. > > Is it being addressed in another patch? It should be disabled here because we're not using the port anymore at this point. > > The rest of the patch looks good. > > Anusha > > > + val = I915_READ(PORT_TX_DFLEXDPCSSS); > > + if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) { > > + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > > + } > > +} > > + > > static bool icl_tc_port_connected(struct drm_i915_private > > *dev_priv, > > struct intel_digital_port > > *intel_dig_port) { @@ > > -4782,12 +4832,17 @@ static bool icl_tc_port_connected(struct > > drm_i915_private *dev_priv, > > if (cpu_isr & tbt_bit) > > is_tbt = true; > > > > - if (!is_legacy && !is_typec && !is_tbt) > > + if (!is_legacy && !is_typec && !is_tbt) { > > + icl_tc_phy_mode_status_disconnect(dev_priv, > > intel_dig_port); > > return false; > > + } > > > > icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, > > is_typec, > > is_tbt); > > > > + if (!icl_tc_phy_mode_status_connect(dev_priv, > > intel_dig_port)) > > + return false; > > + > > return true; > > } > > > > -- > > 2.14.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 24308d4435f5..42cbace4c61e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -10027,4 +10027,10 @@ enum skl_power_gate { _ICL_PHY_MISC_B) #define ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN (1 << 23) +#define PORT_TX_DFLEXDPPMS _MMIO(0x163890) +#define DP_PHY_MODE_STATUS_COMPLETED(tc_port) (1 << (tc_port)) + +#define PORT_TX_DFLEXDPCSSS _MMIO(0x163894) +#define DP_PHY_MODE_STATUS_NOT_SAFE(tc_port) (1 << (tc_port)) + #endif /* _I915_REG_H_ */ diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f3d5b9eed625..f25f871e7c22 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4762,6 +4762,56 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, type_str); } +static bool icl_tc_phy_mode_status_connect(struct drm_i915_private *dev_priv, + struct intel_digital_port *dig_port) +{ + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + u32 val; + + if (dig_port->tc_type != TC_PORT_LEGACY && + dig_port->tc_type != TC_PORT_TYPEC) + return true; + + val = I915_READ(PORT_TX_DFLEXDPPMS); + if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { + DRM_ERROR("DP PHY for TC port %d not ready\n", tc_port); + return false; + } + + /* + * This function may be called many times in a row without an HPD event + * in between, so try to avoid the write when we can. + */ + val = I915_READ(PORT_TX_DFLEXDPCSSS); + if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) { + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); + } + + return true; +} + +static void icl_tc_phy_mode_status_disconnect(struct drm_i915_private *dev_priv, + struct intel_digital_port *dig_port) +{ + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + u32 val; + + if (dig_port->tc_type != TC_PORT_LEGACY && + dig_port->tc_type != TC_PORT_TYPEC) + return; + + /* + * This function may be called many times in a row without an HPD event + * in between, so try to avoid the write when we can. + */ + val = I915_READ(PORT_TX_DFLEXDPCSSS); + if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) { + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); + } +} + static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, struct intel_digital_port *intel_dig_port) { @@ -4782,12 +4832,17 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, if (cpu_isr & tbt_bit) is_tbt = true; - if (!is_legacy && !is_typec && !is_tbt) + if (!is_legacy && !is_typec && !is_tbt) { + icl_tc_phy_mode_status_disconnect(dev_priv, intel_dig_port); return false; + } icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec, is_tbt); + if (!icl_tc_phy_mode_status_connect(dev_priv, intel_dig_port)) + return false; + return true; }
Implement the DFLEXDPPMS/DFLEXDPCSSS dance for DisplayPort. These functions need to be called during HPD assert/deassert, but due to how our driver works it's much simpler if we always call them when icl_digital_port_connected() is called, which means we won't call them exactly once per HPD event. This should also cover the connected boot case, whatever the BIOS does. We're still missing the HDMI case, which should be implemented in the next patch. Also notice that, today, the BSpec pages for the DFLEXDPPMS and DFLEXDPCSSS registers are wrong, so you should only trust the flows described by the "Gen11 TypeC Programming" page in our spec. Cc: Animesh Manna <animesh.manna@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 6 +++++ drivers/gpu/drm/i915/intel_dp.c | 57 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-)