diff mbox series

[1/3] drm/i915/dp: Return the right vswing tables

Message ID 20200327213413.166623-1-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/i915/dp: Return the right vswing tables | expand

Commit Message

Souza, Jose March 27, 2020, 9:34 p.m. UTC 
DDI ports have its encoders initialized with INTEL_OUTPUT_DDI type and
later eDP ports that have the type changed to INTEL_OUTPUT_EDP.
But for all other DDI ports it can drive HDMI or DP depending on what
user connects to the ports.

ehl_get_combo_buf_trans() and tgl_get_combo_buf_trans() was checking
for INTEL_OUTPUT_DP that was never true, causing eDP vswing tables
being used.

So here changing the check to INTEL_OUTPUT_DDI, HDMI cases will be
correctly handled as it do not use encoder->type, instead it calls the
functions with INTEL_OUTPUT_HDMI as type parameter and HDMI don't have
retraining.

Fixes: bd3cf6f7ce20 ("drm/i915/dp/tgl+: Update combo phy vswing tables")
Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ville Syrjälä March 30, 2020, 2:50 p.m. UTC | #1 
On Fri, Mar 27, 2020 at 02:34:11PM -0700, José Roberto de Souza wrote:
> DDI ports have its encoders initialized with INTEL_OUTPUT_DDI type and
> later eDP ports that have the type changed to INTEL_OUTPUT_EDP.
> But for all other DDI ports it can drive HDMI or DP depending on what
> user connects to the ports.
> 
> ehl_get_combo_buf_trans() and tgl_get_combo_buf_trans() was checking
> for INTEL_OUTPUT_DP that was never true, causing eDP vswing tables
> being used.
> 
> So here changing the check to INTEL_OUTPUT_DDI, HDMI cases will be
> correctly handled as it do not use encoder->type, instead it calls the
> functions with INTEL_OUTPUT_HDMI as type parameter and HDMI don't have
> retraining.
> 
> Fixes: bd3cf6f7ce20 ("drm/i915/dp/tgl+: Update combo phy vswing tables")
> Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 916a802af788..7af1572d4f1d 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -947,7 +947,7 @@ static const struct cnl_ddi_buf_trans *
>  ehl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
>  			int *n_entries)
>  {
> -	if (type == INTEL_OUTPUT_DP && rate > 270000) {
> +	if (type == INTEL_OUTPUT_DDI && rate > 270000) {

Please no. I'd rather not see "DDI" here. We want to check which mode
we're driving the output in, and "DDI" isn't one of the valid choices.

The fact that we sometimes pass in encoder->type is a bit of shortcut
to make the DP vs. EDP distinction. And so far every function knew to
only compare the value against EDP/HDMI and neve against DP. Looks like
someone broke that (admittedly crappy) convention.

We should probably fix this a bit higher up and make sure we only ever
pass in EDP/DP/HDMI, never DDI.

>  		*n_entries = ARRAY_SIZE(ehl_combo_phy_ddi_translations_hbr2_hbr3);
>  		return ehl_combo_phy_ddi_translations_hbr2_hbr3;
>  	}
> @@ -959,7 +959,7 @@ static const struct cnl_ddi_buf_trans *
>  tgl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
>  			int *n_entries)
>  {
> -	if (type != INTEL_OUTPUT_DP) {
> +	if (type != INTEL_OUTPUT_DDI) {
>  		return icl_get_combo_buf_trans(dev_priv, type, rate, n_entries);
>  	} else if (rate > 270000) {
>  		*n_entries = ARRAY_SIZE(tgl_combo_phy_ddi_translations_dp_hbr2);
> -- 
> 2.26.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose March 30, 2020, 7:24 p.m. UTC | #2 
On Mon, 2020-03-30 at 17:50 +0300, Ville Syrjälä wrote:
> On Fri, Mar 27, 2020 at 02:34:11PM -0700, José Roberto de Souza
> wrote:
> > DDI ports have its encoders initialized with INTEL_OUTPUT_DDI type
> > and
> > later eDP ports that have the type changed to INTEL_OUTPUT_EDP.
> > But for all other DDI ports it can drive HDMI or DP depending on
> > what
> > user connects to the ports.
> > 
> > ehl_get_combo_buf_trans() and tgl_get_combo_buf_trans() was
> > checking
> > for INTEL_OUTPUT_DP that was never true, causing eDP vswing tables
> > being used.
> > 
> > So here changing the check to INTEL_OUTPUT_DDI, HDMI cases will be
> > correctly handled as it do not use encoder->type, instead it calls
> > the
> > functions with INTEL_OUTPUT_HDMI as type parameter and HDMI don't
> > have
> > retraining.
> > 
> > Fixes: bd3cf6f7ce20 ("drm/i915/dp/tgl+: Update combo phy vswing
> > tables")
> > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 916a802af788..7af1572d4f1d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -947,7 +947,7 @@ static const struct cnl_ddi_buf_trans *
> >  ehl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int
> > type, int rate,
> >  			int *n_entries)
> >  {
> > -	if (type == INTEL_OUTPUT_DP && rate > 270000) {
> > +	if (type == INTEL_OUTPUT_DDI && rate > 270000) {
> 
> Please no. I'd rather not see "DDI" here. We want to check which mode
> we're driving the output in, and "DDI" isn't one of the valid
> choices.
> 
> The fact that we sometimes pass in encoder->type is a bit of shortcut
> to make the DP vs. EDP distinction. And so far every function knew to
> only compare the value against EDP/HDMI and neve against DP. Looks
> like
> someone broke that (admittedly crappy) convention.
> 
> We should probably fix this a bit higher up and make sure we only
> ever
> pass in EDP/DP/HDMI, never DDI.

Okay so for now I will just do the bellow:

if (type != INTEL_OUTPUT_EDP && type != INTEL_OUTPUT_HDMI && rate >
270000) {

Good enough for now?
> 
> >  		*n_entries =
> > ARRAY_SIZE(ehl_combo_phy_ddi_translations_hbr2_hbr3);
> >  		return ehl_combo_phy_ddi_translations_hbr2_hbr3;
> >  	}
> > @@ -959,7 +959,7 @@ static const struct cnl_ddi_buf_trans *
> >  tgl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int
> > type, int rate,
> >  			int *n_entries)
> >  {
> > -	if (type != INTEL_OUTPUT_DP) {
> > +	if (type != INTEL_OUTPUT_DDI) {
> >  		return icl_get_combo_buf_trans(dev_priv, type, rate,
> > n_entries);
> >  	} else if (rate > 270000) {
> >  		*n_entries =
> > ARRAY_SIZE(tgl_combo_phy_ddi_translations_dp_hbr2);
> > -- 
> > 2.26.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä March 31, 2020, 2:42 p.m. UTC | #3 
On Mon, Mar 30, 2020 at 07:24:55PM +0000, Souza, Jose wrote:
> On Mon, 2020-03-30 at 17:50 +0300, Ville Syrjälä wrote:
> > On Fri, Mar 27, 2020 at 02:34:11PM -0700, José Roberto de Souza
> > wrote:
> > > DDI ports have its encoders initialized with INTEL_OUTPUT_DDI type
> > > and
> > > later eDP ports that have the type changed to INTEL_OUTPUT_EDP.
> > > But for all other DDI ports it can drive HDMI or DP depending on
> > > what
> > > user connects to the ports.
> > > 
> > > ehl_get_combo_buf_trans() and tgl_get_combo_buf_trans() was
> > > checking
> > > for INTEL_OUTPUT_DP that was never true, causing eDP vswing tables
> > > being used.
> > > 
> > > So here changing the check to INTEL_OUTPUT_DDI, HDMI cases will be
> > > correctly handled as it do not use encoder->type, instead it calls
> > > the
> > > functions with INTEL_OUTPUT_HDMI as type parameter and HDMI don't
> > > have
> > > retraining.
> > > 
> > > Fixes: bd3cf6f7ce20 ("drm/i915/dp/tgl+: Update combo phy vswing
> > > tables")
> > > Cc: Clinton A Taylor <clinton.a.taylor@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 916a802af788..7af1572d4f1d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -947,7 +947,7 @@ static const struct cnl_ddi_buf_trans *
> > >  ehl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int
> > > type, int rate,
> > >  			int *n_entries)
> > >  {
> > > -	if (type == INTEL_OUTPUT_DP && rate > 270000) {
> > > +	if (type == INTEL_OUTPUT_DDI && rate > 270000) {
> > 
> > Please no. I'd rather not see "DDI" here. We want to check which mode
> > we're driving the output in, and "DDI" isn't one of the valid
> > choices.
> > 
> > The fact that we sometimes pass in encoder->type is a bit of shortcut
> > to make the DP vs. EDP distinction. And so far every function knew to
> > only compare the value against EDP/HDMI and neve against DP. Looks
> > like
> > someone broke that (admittedly crappy) convention.
> > 
> > We should probably fix this a bit higher up and make sure we only
> > ever
> > pass in EDP/DP/HDMI, never DDI.
> 
> Okay so for now I will just do the bellow:
> 
> if (type != INTEL_OUTPUT_EDP && type != INTEL_OUTPUT_HDMI && rate >
> 270000) {
> 
> Good enough for now?

Works for me.

> > 
> > >  		*n_entries =
> > > ARRAY_SIZE(ehl_combo_phy_ddi_translations_hbr2_hbr3);
> > >  		return ehl_combo_phy_ddi_translations_hbr2_hbr3;
> > >  	}
> > > @@ -959,7 +959,7 @@ static const struct cnl_ddi_buf_trans *
> > >  tgl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int
> > > type, int rate,
> > >  			int *n_entries)
> > >  {
> > > -	if (type != INTEL_OUTPUT_DP) {
> > > +	if (type != INTEL_OUTPUT_DDI) {
> > >  		return icl_get_combo_buf_trans(dev_priv, type, rate,
> > > n_entries);
> > >  	} else if (rate > 270000) {
> > >  		*n_entries =
> > > ARRAY_SIZE(tgl_combo_phy_ddi_translations_dp_hbr2);
> > > -- 
> > > 2.26.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 916a802af788..7af1572d4f1d 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -947,7 +947,7 @@  static const struct cnl_ddi_buf_trans *
 ehl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
 			int *n_entries)
 {
-	if (type == INTEL_OUTPUT_DP && rate > 270000) {
+	if (type == INTEL_OUTPUT_DDI && rate > 270000) {
 		*n_entries = ARRAY_SIZE(ehl_combo_phy_ddi_translations_hbr2_hbr3);
 		return ehl_combo_phy_ddi_translations_hbr2_hbr3;
 	}
@@ -959,7 +959,7 @@  static const struct cnl_ddi_buf_trans *
 tgl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate,
 			int *n_entries)
 {
-	if (type != INTEL_OUTPUT_DP) {
+	if (type != INTEL_OUTPUT_DDI) {
 		return icl_get_combo_buf_trans(dev_priv, type, rate, n_entries);
 	} else if (rate > 270000) {
 		*n_entries = ARRAY_SIZE(tgl_combo_phy_ddi_translations_dp_hbr2);