| Message ID | 20191203023110.488972-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/dp: Define each HBR link rate | expand |
On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> wrote: > This is better than keep those values in the code that you always > need to check the DP spec to know what level of HBR it is. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index a976606d21c7..914f0cc4d237 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -49,6 +49,10 @@ > #include "intel_tc.h" > #include "intel_vdsc.h" > > +#define HBR_RATE 270000 > +#define HBR2_RATE 540000 > +#define HBR3_RATE 810000 > + > struct ddi_buf_trans { > u32 trans1; /* balance leg enable, de-emph level */ > u32 trans2; /* vref sel, vswing */ > @@ -888,7 +892,7 @@ icl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate, > if (type == INTEL_OUTPUT_HDMI) { > *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi); > return icl_combo_phy_ddi_translations_hdmi; > - } else if (rate > 540000 && type == INTEL_OUTPUT_EDP) { > + } else if (rate > HBR2_RATE && type == INTEL_OUTPUT_EDP) { I don't want a patch switching some random place to using a macro. Either we stick to numbers or switch all. And if switch all, add the rates to drm core, not locally to intel_ddi.c. (And then wonder what to do with the intermediate rates in intel_dp_set_source_rates()...) Personally, HBR<N> is less useful to me in code, it's the actual rate that helps me. But I'll trust Ville's judgement on this one. BR, Jani. > *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_edp_hbr3); > return icl_combo_phy_ddi_translations_edp_hbr3; > } else if (type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.low_vswing) {
On Tue, Dec 03, 2019 at 11:08:52AM +0200, Jani Nikula wrote: > On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> wrote: > > This is better than keep those values in the code that you always > > need to check the DP spec to know what level of HBR it is. > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index a976606d21c7..914f0cc4d237 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -49,6 +49,10 @@ > > #include "intel_tc.h" > > #include "intel_vdsc.h" > > > > +#define HBR_RATE 270000 > > +#define HBR2_RATE 540000 > > +#define HBR3_RATE 810000 > > + > > struct ddi_buf_trans { > > u32 trans1; /* balance leg enable, de-emph level */ > > u32 trans2; /* vref sel, vswing */ > > @@ -888,7 +892,7 @@ icl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate, > > if (type == INTEL_OUTPUT_HDMI) { > > *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi); > > return icl_combo_phy_ddi_translations_hdmi; > > - } else if (rate > 540000 && type == INTEL_OUTPUT_EDP) { > > + } else if (rate > HBR2_RATE && type == INTEL_OUTPUT_EDP) { > > I don't want a patch switching some random place to using a > macro. Either we stick to numbers or switch all. > > And if switch all, add the rates to drm core, not locally to > intel_ddi.c. (And then wonder what to do with the intermediate rates in > intel_dp_set_source_rates()...) Yeah, we'll still end up with a mix of defines and raw numbers. > > Personally, HBR<N> is less useful to me in code, it's the actual rate > that helps me. > > But I'll trust Ville's judgement on this one. I tend to prefer raw numbers for this sort of stuff. If we didn't have the intermediate rates I might have a different opinion. The only thing I really worry about with raw numbers is the potential for typos. The original problem of bspec talking about hbr2 in the bug trans tables we could probably solve with a comment.
On Mon, Dec 02, 2019 at 06:31:09PM -0800, José Roberto de Souza wrote: > This is better than keep those values in the code that you always > need to check the DP spec to know what level of HBR it is. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> I think there are a bunch of other places where we could use these new macros too, but that can be done in a followup. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> We might want to add RBR (162000) as well for completeness in the future. Matt > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index a976606d21c7..914f0cc4d237 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -49,6 +49,10 @@ > #include "intel_tc.h" > #include "intel_vdsc.h" > > +#define HBR_RATE 270000 > +#define HBR2_RATE 540000 > +#define HBR3_RATE 810000 > + > struct ddi_buf_trans { > u32 trans1; /* balance leg enable, de-emph level */ > u32 trans2; /* vref sel, vswing */ > @@ -888,7 +892,7 @@ icl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate, > if (type == INTEL_OUTPUT_HDMI) { > *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi); > return icl_combo_phy_ddi_translations_hdmi; > - } else if (rate > 540000 && type == INTEL_OUTPUT_EDP) { > + } else if (rate > HBR2_RATE && type == INTEL_OUTPUT_EDP) { > *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_edp_hbr3); > return icl_combo_phy_ddi_translations_edp_hbr3; > } else if (type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.low_vswing) { > -- > 2.24.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2019-12-03 at 15:11 +0200, Ville Syrjälä wrote: > On Tue, Dec 03, 2019 at 11:08:52AM +0200, Jani Nikula wrote: > > On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> > > wrote: > > > This is better than keep those values in the code that you always > > > need to check the DP spec to know what level of HBR it is. > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index a976606d21c7..914f0cc4d237 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -49,6 +49,10 @@ > > > #include "intel_tc.h" > > > #include "intel_vdsc.h" > > > > > > +#define HBR_RATE 270000 > > > +#define HBR2_RATE 540000 > > > +#define HBR3_RATE 810000 > > > + > > > struct ddi_buf_trans { > > > u32 trans1; /* balance leg enable, de-emph level */ > > > u32 trans2; /* vref sel, vswing */ > > > @@ -888,7 +892,7 @@ icl_get_combo_buf_trans(struct > > > drm_i915_private *dev_priv, int type, int rate, > > > if (type == INTEL_OUTPUT_HDMI) { > > > *n_entries = > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi); > > > return icl_combo_phy_ddi_translations_hdmi; > > > - } else if (rate > 540000 && type == INTEL_OUTPUT_EDP) { > > > + } else if (rate > HBR2_RATE && type == INTEL_OUTPUT_EDP) { > > > > I don't want a patch switching some random place to using a > > macro. Either we stick to numbers or switch all. > > > > And if switch all, add the rates to drm core, not locally to > > intel_ddi.c. (And then wonder what to do with the intermediate > > rates in > > intel_dp_set_source_rates()...) > > Yeah, we'll still end up with a mix of defines and raw numbers. > > > Personally, HBR<N> is less useful to me in code, it's the actual > > rate > > that helps me. > > > > But I'll trust Ville's judgement on this one. > > I tend to prefer raw numbers for this sort of stuff. If we didn't > have > the intermediate rates I might have a different opinion. The only > thing > I really worry about with raw numbers is the potential for typos. > > The original problem of bspec talking about hbr2 in the bug trans > tables we could probably solve with a comment. > Okay, dropping this change.
On Tue, Dec 03, 2019 at 03:11:54PM +0200, Ville Syrjälä wrote: > On Tue, Dec 03, 2019 at 11:08:52AM +0200, Jani Nikula wrote: > > On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com> wrote: > > > This is better than keep those values in the code that you always > > > need to check the DP spec to know what level of HBR it is. > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index a976606d21c7..914f0cc4d237 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -49,6 +49,10 @@ > > > #include "intel_tc.h" > > > #include "intel_vdsc.h" > > > > > > +#define HBR_RATE 270000 > > > +#define HBR2_RATE 540000 > > > +#define HBR3_RATE 810000 > > > + > > > struct ddi_buf_trans { > > > u32 trans1; /* balance leg enable, de-emph level */ > > > u32 trans2; /* vref sel, vswing */ > > > @@ -888,7 +892,7 @@ icl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate, > > > if (type == INTEL_OUTPUT_HDMI) { > > > *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi); > > > return icl_combo_phy_ddi_translations_hdmi; > > > - } else if (rate > 540000 && type == INTEL_OUTPUT_EDP) { > > > + } else if (rate > HBR2_RATE && type == INTEL_OUTPUT_EDP) { > > > > I don't want a patch switching some random place to using a > > macro. Either we stick to numbers or switch all. > > > > And if switch all, add the rates to drm core, not locally to > > intel_ddi.c. (And then wonder what to do with the intermediate rates in > > intel_dp_set_source_rates()...) > > Yeah, we'll still end up with a mix of defines and raw numbers. > > > > > Personally, HBR<N> is less useful to me in code, it's the actual rate > > that helps me. > > > > But I'll trust Ville's judgement on this one. > > I tend to prefer raw numbers for this sort of stuff. If we didn't have > the intermediate rates I might have a different opinion. The only thing > I really worry about with raw numbers is the potential for typos. Yes, especially due to the typos and possibilities of missing or adding an extra 0 makes me wonder that it could be a good idea to add all the #defines RBR - HBR3 in drm_dp_helper.h somewhere with proper comments on which of the spec added which rate etc. Regards Manasi > > The original problem of bspec talking about hbr2 in the bug trans > tables we could probably solve with a comment. > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index a976606d21c7..914f0cc4d237 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -49,6 +49,10 @@ #include "intel_tc.h" #include "intel_vdsc.h" +#define HBR_RATE 270000 +#define HBR2_RATE 540000 +#define HBR3_RATE 810000 + struct ddi_buf_trans { u32 trans1; /* balance leg enable, de-emph level */ u32 trans2; /* vref sel, vswing */ @@ -888,7 +892,7 @@ icl_get_combo_buf_trans(struct drm_i915_private *dev_priv, int type, int rate, if (type == INTEL_OUTPUT_HDMI) { *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_hdmi); return icl_combo_phy_ddi_translations_hdmi; - } else if (rate > 540000 && type == INTEL_OUTPUT_EDP) { + } else if (rate > HBR2_RATE && type == INTEL_OUTPUT_EDP) { *n_entries = ARRAY_SIZE(icl_combo_phy_ddi_translations_edp_hbr3); return icl_combo_phy_ddi_translations_edp_hbr3; } else if (type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.low_vswing) {
This is better than keep those values in the code that you always need to check the DP spec to know what level of HBR it is. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)