Re: [Intel-gfx] [PATCH] Revert "Revert "drm/i915/dp: Correctly advertise HBR3 for GEN11+""
On Wed, Jul 15, 2020 at 04:16:09PM -0700, Matt Atwood wrote:
> On Wed, Jul 15, 2020 at 11:27:15PM +0300, Imre Deak wrote:
> > On Wed, Jul 15, 2020 at 12:15:35PM -0700, Manasi Navare wrote:
> > > On Wed, Jul 15, 2020 at 07:29:31PM +0300, Imre Deak wrote:
> > > > The problem the reverted patch revealed could've been fixed by commit
> > > > 619ad4874585 ("drm/i915/ddi: Don't frob the DP link scramble disabling
> > > > flag")
> > > > in particular because the revealed problem (at least in one case)
> > > > happened
> > > > when switching to the TPS4 training pattern, which needs scrambling.
> > > >
> > > > Let's try applying the HBR3 fix again.
> >
> > The link training failure still happens the same way on fi-icl-u2.
> Previously this only failed on a specific TGL CI system, did you get
> failures on both this go around? ICL passed last time.
Arg, it's fi-tgl-u2, sorry for the mixup. For instance:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18181/fi-tgl-u2/dmesg0.txt
https://patchwork.freedesktop.org/series/79486/ would be another thing
to try.
> >
> > > >
> > > > This reverts commit d3913019602e32ef6fbba8eb0167e83250cdab22.
> > > >
> > > > Cc: Matt Atwood
> > > > Cc: Ville Syrjälä
> > > > Cc: Manasi Navare
> > > > Cc: José Roberto de Souza
> > > > Signed-off-by: Imre Deak
> > >
> > > Makes sense since the problem occured only in CI, not on
> > > the local testing done by Matt on his eDP panel.
> > >
> > > Reviewed-by: Manasi Navare
> > >
> > > Manasi
> > >
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 28 ++---
> > > > 1 file changed, 11 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index d6295eb20b63..a5ab405d3a12 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -137,6 +137,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4};
> > > > *
> > > > * If a CPU or PCH DP output is attached to an eDP panel, this function
> > > > * will return true, and false otherwise.
> > > > + *
> > > > + * This function is not safe to use prior to encoder type being set.
> > > > */
> > > > bool intel_dp_is_edp(struct intel_dp *intel_dp)
> > > > {
> > > > @@ -8157,8 +8159,6 @@ intel_dp_init_connector(struct intel_digital_port
> > > > *dig_port,
> > > > intel_encoder->base.name))
> > > > return false;
> > > >
> > > > - intel_dp_set_source_rates(intel_dp);
> > > > -
> > > > intel_dp->reset_link_params = true;
> > > > intel_dp->pps_pipe = INVALID_PIPE;
> > > > intel_dp->active_pipe = INVALID_PIPE;
> > > > @@ -8174,28 +8174,22 @@ intel_dp_init_connector(struct
> > > > intel_digital_port *dig_port,
> > > > */
> > > > drm_WARN_ON(dev, intel_phy_is_tc(dev_priv, phy));
> > > > type = DRM_MODE_CONNECTOR_eDP;
> > > > + intel_encoder->type = INTEL_OUTPUT_EDP;
> > > > +
> > > > + /* eDP only on port B and/or C on vlv/chv */
> > > > + if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> > > > + IS_CHERRYVIEW(dev_priv)) &&
> > > > + port != PORT_B && port != PORT_C))
> > > > + return false;
> > > > } else {
> > > > type = DRM_MODE_CONNECTOR_DisplayPort;
> > > > }
> > > >
> > > > + intel_dp_set_source_rates(intel_dp);
> > > > +
> > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > >
> > > > - /*
> > > > -* For eDP we always set the encoder type to INTEL_OUTPUT_EDP,
> > > > but
> > > > -* for DP the encoder type can be set by the caller to
> > > > -* INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> > > > -*/
> > > > - if (type == DRM_MODE_CONNECTOR_eDP)
> > > > - intel_encoder->type = INTEL_OUTPUT_EDP;
> > > > -
> > > > - /* eDP only on port B and/or C on vlv/chv */
> > > > - if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> > > > - IS_CHERRYVIEW(dev_priv)) &&
> > > > - intel_dp_is_edp(intel_dp) &&
> > > > - port != PORT_B && port != PORT_C))
> > > > - return false;
> > > > -
> > > > drm_dbg_kms(_priv->drm,
> > > > "Adding %s connector on [ENCODER:%d:%s]\n",
> > > > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> > > > --
> > > > 2.23.1
> > > >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "Revert "drm/i915/dp: Correctly advertise HBR3 for GEN11+""
On Wed, Jul 15, 2020 at 11:27:15PM +0300, Imre Deak wrote:
> On Wed, Jul 15, 2020 at 12:15:35PM -0700, Manasi Navare wrote:
> > On Wed, Jul 15, 2020 at 07:29:31PM +0300, Imre Deak wrote:
> > > The problem the reverted patch revealed could've been fixed by commit
> > > 619ad4874585 ("drm/i915/ddi: Don't frob the DP link scramble disabling
> > > flag")
> > > in particular because the revealed problem (at least in one case) happened
> > > when switching to the TPS4 training pattern, which needs scrambling.
> > >
> > > Let's try applying the HBR3 fix again.
>
> The link training failure still happens the same way on fi-icl-u2.
Previously this only failed on a specific TGL CI system, did you get
failures on both this go around? ICL passed last time.
>
> > >
> > > This reverts commit d3913019602e32ef6fbba8eb0167e83250cdab22.
> > >
> > > Cc: Matt Atwood
> > > Cc: Ville Syrjälä
> > > Cc: Manasi Navare
> > > Cc: José Roberto de Souza
> > > Signed-off-by: Imre Deak
> >
> > Makes sense since the problem occured only in CI, not on
> > the local testing done by Matt on his eDP panel.
> >
> > Reviewed-by: Manasi Navare
> >
> > Manasi
> >
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dp.c | 28 ++---
> > > 1 file changed, 11 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index d6295eb20b63..a5ab405d3a12 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -137,6 +137,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4};
> > > *
> > > * If a CPU or PCH DP output is attached to an eDP panel, this function
> > > * will return true, and false otherwise.
> > > + *
> > > + * This function is not safe to use prior to encoder type being set.
> > > */
> > > bool intel_dp_is_edp(struct intel_dp *intel_dp)
> > > {
> > > @@ -8157,8 +8159,6 @@ intel_dp_init_connector(struct intel_digital_port
> > > *dig_port,
> > >intel_encoder->base.name))
> > > return false;
> > >
> > > - intel_dp_set_source_rates(intel_dp);
> > > -
> > > intel_dp->reset_link_params = true;
> > > intel_dp->pps_pipe = INVALID_PIPE;
> > > intel_dp->active_pipe = INVALID_PIPE;
> > > @@ -8174,28 +8174,22 @@ intel_dp_init_connector(struct intel_digital_port
> > > *dig_port,
> > >*/
> > > drm_WARN_ON(dev, intel_phy_is_tc(dev_priv, phy));
> > > type = DRM_MODE_CONNECTOR_eDP;
> > > + intel_encoder->type = INTEL_OUTPUT_EDP;
> > > +
> > > + /* eDP only on port B and/or C on vlv/chv */
> > > + if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> > > + IS_CHERRYVIEW(dev_priv)) &&
> > > + port != PORT_B && port != PORT_C))
> > > + return false;
> > > } else {
> > > type = DRM_MODE_CONNECTOR_DisplayPort;
> > > }
> > >
> > > + intel_dp_set_source_rates(intel_dp);
> > > +
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > >
> > > - /*
> > > - * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
> > > - * for DP the encoder type can be set by the caller to
> > > - * INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> > > - */
> > > - if (type == DRM_MODE_CONNECTOR_eDP)
> > > - intel_encoder->type = INTEL_OUTPUT_EDP;
> > > -
> > > - /* eDP only on port B and/or C on vlv/chv */
> > > - if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> > > - IS_CHERRYVIEW(dev_priv)) &&
> > > - intel_dp_is_edp(intel_dp) &&
> > > - port != PORT_B && port != PORT_C))
> > > - return false;
> > > -
> > > drm_dbg_kms(_priv->drm,
> > > "Adding %s connector on [ENCODER:%d:%s]\n",
> > > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> > > --
> > > 2.23.1
> > >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "Revert "drm/i915/dp: Correctly advertise HBR3 for GEN11+""
On Wed, Jul 15, 2020 at 12:15:35PM -0700, Manasi Navare wrote:
> On Wed, Jul 15, 2020 at 07:29:31PM +0300, Imre Deak wrote:
> > The problem the reverted patch revealed could've been fixed by commit
> > 619ad4874585 ("drm/i915/ddi: Don't frob the DP link scramble disabling
> > flag")
> > in particular because the revealed problem (at least in one case) happened
> > when switching to the TPS4 training pattern, which needs scrambling.
> >
> > Let's try applying the HBR3 fix again.
The link training failure still happens the same way on fi-icl-u2.
> >
> > This reverts commit d3913019602e32ef6fbba8eb0167e83250cdab22.
> >
> > Cc: Matt Atwood
> > Cc: Ville Syrjälä
> > Cc: Manasi Navare
> > Cc: José Roberto de Souza
> > Signed-off-by: Imre Deak
>
> Makes sense since the problem occured only in CI, not on
> the local testing done by Matt on his eDP panel.
>
> Reviewed-by: Manasi Navare
>
> Manasi
>
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 28 ++---
> > 1 file changed, 11 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index d6295eb20b63..a5ab405d3a12 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -137,6 +137,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4};
> > *
> > * If a CPU or PCH DP output is attached to an eDP panel, this function
> > * will return true, and false otherwise.
> > + *
> > + * This function is not safe to use prior to encoder type being set.
> > */
> > bool intel_dp_is_edp(struct intel_dp *intel_dp)
> > {
> > @@ -8157,8 +8159,6 @@ intel_dp_init_connector(struct intel_digital_port
> > *dig_port,
> > intel_encoder->base.name))
> > return false;
> >
> > - intel_dp_set_source_rates(intel_dp);
> > -
> > intel_dp->reset_link_params = true;
> > intel_dp->pps_pipe = INVALID_PIPE;
> > intel_dp->active_pipe = INVALID_PIPE;
> > @@ -8174,28 +8174,22 @@ intel_dp_init_connector(struct intel_digital_port
> > *dig_port,
> > */
> > drm_WARN_ON(dev, intel_phy_is_tc(dev_priv, phy));
> > type = DRM_MODE_CONNECTOR_eDP;
> > + intel_encoder->type = INTEL_OUTPUT_EDP;
> > +
> > + /* eDP only on port B and/or C on vlv/chv */
> > + if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> > + IS_CHERRYVIEW(dev_priv)) &&
> > + port != PORT_B && port != PORT_C))
> > + return false;
> > } else {
> > type = DRM_MODE_CONNECTOR_DisplayPort;
> > }
> >
> > + intel_dp_set_source_rates(intel_dp);
> > +
> > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> >
> > - /*
> > -* For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
> > -* for DP the encoder type can be set by the caller to
> > -* INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> > -*/
> > - if (type == DRM_MODE_CONNECTOR_eDP)
> > - intel_encoder->type = INTEL_OUTPUT_EDP;
> > -
> > - /* eDP only on port B and/or C on vlv/chv */
> > - if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> > - IS_CHERRYVIEW(dev_priv)) &&
> > - intel_dp_is_edp(intel_dp) &&
> > - port != PORT_B && port != PORT_C))
> > - return false;
> > -
> > drm_dbg_kms(_priv->drm,
> > "Adding %s connector on [ENCODER:%d:%s]\n",
> > type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> > --
> > 2.23.1
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "Revert "drm/i915/dp: Correctly advertise HBR3 for GEN11+""
On Wed, Jul 15, 2020 at 07:29:31PM +0300, Imre Deak wrote:
> The problem the reverted patch revealed could've been fixed by commit
> 619ad4874585 ("drm/i915/ddi: Don't frob the DP link scramble disabling flag")
> in particular because the revealed problem (at least in one case) happened
> when switching to the TPS4 training pattern, which needs scrambling.
>
> Let's try applying the HBR3 fix again.
>
> This reverts commit d3913019602e32ef6fbba8eb0167e83250cdab22.
>
> Cc: Matt Atwood
> Cc: Ville Syrjälä
> Cc: Manasi Navare
> Cc: José Roberto de Souza
> Signed-off-by: Imre Deak
Makes sense since the problem occured only in CI, not on
the local testing done by Matt on his eDP panel.
Reviewed-by: Manasi Navare
Manasi
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 28 ++---
> 1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index d6295eb20b63..a5ab405d3a12 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -137,6 +137,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4};
> *
> * If a CPU or PCH DP output is attached to an eDP panel, this function
> * will return true, and false otherwise.
> + *
> + * This function is not safe to use prior to encoder type being set.
> */
> bool intel_dp_is_edp(struct intel_dp *intel_dp)
> {
> @@ -8157,8 +8159,6 @@ intel_dp_init_connector(struct intel_digital_port
> *dig_port,
>intel_encoder->base.name))
> return false;
>
> - intel_dp_set_source_rates(intel_dp);
> -
> intel_dp->reset_link_params = true;
> intel_dp->pps_pipe = INVALID_PIPE;
> intel_dp->active_pipe = INVALID_PIPE;
> @@ -8174,28 +8174,22 @@ intel_dp_init_connector(struct intel_digital_port
> *dig_port,
>*/
> drm_WARN_ON(dev, intel_phy_is_tc(dev_priv, phy));
> type = DRM_MODE_CONNECTOR_eDP;
> + intel_encoder->type = INTEL_OUTPUT_EDP;
> +
> + /* eDP only on port B and/or C on vlv/chv */
> + if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> + IS_CHERRYVIEW(dev_priv)) &&
> + port != PORT_B && port != PORT_C))
> + return false;
> } else {
> type = DRM_MODE_CONNECTOR_DisplayPort;
> }
>
> + intel_dp_set_source_rates(intel_dp);
> +
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> intel_dp->active_pipe = vlv_active_pipe(intel_dp);
>
> - /*
> - * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
> - * for DP the encoder type can be set by the caller to
> - * INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
> - */
> - if (type == DRM_MODE_CONNECTOR_eDP)
> - intel_encoder->type = INTEL_OUTPUT_EDP;
> -
> - /* eDP only on port B and/or C on vlv/chv */
> - if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
> - IS_CHERRYVIEW(dev_priv)) &&
> - intel_dp_is_edp(intel_dp) &&
> - port != PORT_B && port != PORT_C))
> - return false;
> -
> drm_dbg_kms(_priv->drm,
> "Adding %s connector on [ENCODER:%d:%s]\n",
> type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> --
> 2.23.1
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] Revert "Revert "drm/i915/dp: Correctly advertise HBR3 for GEN11+""
The problem the reverted patch revealed could've been fixed by commit
619ad4874585 ("drm/i915/ddi: Don't frob the DP link scramble disabling flag")
in particular because the revealed problem (at least in one case) happened
when switching to the TPS4 training pattern, which needs scrambling.
Let's try applying the HBR3 fix again.
This reverts commit d3913019602e32ef6fbba8eb0167e83250cdab22.
Cc: Matt Atwood
Cc: Ville Syrjälä
Cc: Manasi Navare
Cc: José Roberto de Souza
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/display/intel_dp.c | 28 ++---
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index d6295eb20b63..a5ab405d3a12 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -137,6 +137,8 @@ static const u8 valid_dsc_slicecount[] = {1, 2, 4};
*
* If a CPU or PCH DP output is attached to an eDP panel, this function
* will return true, and false otherwise.
+ *
+ * This function is not safe to use prior to encoder type being set.
*/
bool intel_dp_is_edp(struct intel_dp *intel_dp)
{
@@ -8157,8 +8159,6 @@ intel_dp_init_connector(struct intel_digital_port
*dig_port,
intel_encoder->base.name))
return false;
- intel_dp_set_source_rates(intel_dp);
-
intel_dp->reset_link_params = true;
intel_dp->pps_pipe = INVALID_PIPE;
intel_dp->active_pipe = INVALID_PIPE;
@@ -8174,28 +8174,22 @@ intel_dp_init_connector(struct intel_digital_port
*dig_port,
*/
drm_WARN_ON(dev, intel_phy_is_tc(dev_priv, phy));
type = DRM_MODE_CONNECTOR_eDP;
+ intel_encoder->type = INTEL_OUTPUT_EDP;
+
+ /* eDP only on port B and/or C on vlv/chv */
+ if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
+ IS_CHERRYVIEW(dev_priv)) &&
+ port != PORT_B && port != PORT_C))
+ return false;
} else {
type = DRM_MODE_CONNECTOR_DisplayPort;
}
+ intel_dp_set_source_rates(intel_dp);
+
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
intel_dp->active_pipe = vlv_active_pipe(intel_dp);
- /*
-* For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
-* for DP the encoder type can be set by the caller to
-* INTEL_OUTPUT_UNKNOWN for DDI, so don't rewrite it.
-*/
- if (type == DRM_MODE_CONNECTOR_eDP)
- intel_encoder->type = INTEL_OUTPUT_EDP;
-
- /* eDP only on port B and/or C on vlv/chv */
- if (drm_WARN_ON(dev, (IS_VALLEYVIEW(dev_priv) ||
- IS_CHERRYVIEW(dev_priv)) &&
- intel_dp_is_edp(intel_dp) &&
- port != PORT_B && port != PORT_C))
- return false;
-
drm_dbg_kms(_priv->drm,
"Adding %s connector on [ENCODER:%d:%s]\n",
type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
--
2.23.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
