On Mon, Jul 13, 2020 at 04:53:33PM -0700, José Roberto de Souza wrote:
> Hours Of Battery Life is a new GEN12+ power-saving feature that allows
> supported motherboards to use a special voltage swing table for eDP
> panels that uses less power.
>
> So here if supported by HW, OEM will set it in VBT and i915 will try
> to train link with HOBL vswing table if link training fails it fall
> back to the original table.
>
> intel_ddi_dp_preemph_max() was optimized to only check the HOBL flag
> instead of do something like is done in intel_ddi_dp_voltage_max()
> because it is only called after the first entry of the voltage swing
> table was loaded so the HOBL flag is valid at that point.
>
> v3:
> - removed a few parameters of icl_ddi_combo_vswing_program() that
> can be taken from encoder
>
> v4:
> - using the HOBL vswing table until training fails completely (Ville)
>
> v5:
> - not reducing lane or link rate when link training fails with HOBL
> active
> - duplicated the HOBL voltage swing entry to match DP spec requirement
>
> BSpec: 49291
> BSpec: 49399
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Animesh Manna <animesh.manna@xxxxxxxxx>
> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 42 +++++++++++++++++++
> .../drm/i915/display/intel_display_types.h | 2 +
> .../drm/i915/display/intel_dp_link_training.c | 19 ++++++---
> drivers/gpu/drm/i915/i915_reg.h | 2 +
> 4 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 2c484b55bcdf..92ae036440fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -706,6 +706,29 @@ static const struct cnl_ddi_buf_trans tgl_combo_phy_ddi_translations_dp_hbr2[] =
> { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 900 900 0.0 */
> };
>
> +/*
> + * Cloned the HOBL entry to comply with the voltage and pre-emphasis entries
> + * that DisplayPort specification requires
> + */
> +static const struct cnl_ddi_buf_trans tgl_combo_phy_ddi_translations_edp_hbr2_hobl[] = {
> + /* VS pre-emp */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 0 0 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 0 1 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 0 2 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 0 3 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 1 0 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 1 1 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 1 2 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 2 0 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 2 1 */
> + { 0x6, 0x7F, 0x3F, 0x00, 0x00 }, /* 3 0 */
> +};
> +
> +static bool is_hobl_buf_trans(const struct cnl_ddi_buf_trans *table)
> +{
> + return table == tgl_combo_phy_ddi_translations_edp_hbr2_hobl;
> +}
OK, so after further thought I guess the double boolean is the least
annoying apporach atm. Slight concerns about this function becoming a
bit of maintenance nightmare, but if we want to improve it then I
suspect we want to revamp the overall buf trans stuff. Eg. I had
one idea to declare the capabilities in each buf trans table. But
not sure if that's viable. Anyway more long term idea.
> +
> static const struct ddi_buf_trans *
> bdw_get_buf_trans_edp(struct intel_encoder *encoder, int *n_entries)
> {
> @@ -1050,6 +1073,16 @@ static const struct cnl_ddi_buf_trans *
> tgl_get_combo_buf_trans(struct intel_encoder *encoder, int type, int rate,
> int *n_entries)
> {
> + if (type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp.hobl) {
> + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> + if (!intel_dp->hobl_failed && rate <= 540000) {
> + /* Same table applies to TGL, RKL and DG1 */
> + *n_entries = ARRAY_SIZE(tgl_combo_phy_ddi_translations_edp_hbr2_hobl);
> + return tgl_combo_phy_ddi_translations_edp_hbr2_hobl;
> + }
> + }
> +
> if (type == INTEL_OUTPUT_HDMI || type == INTEL_OUTPUT_EDP) {
> return icl_get_combo_buf_trans(encoder, type, rate, n_entries);
> } else if (rate > 270000) {
> @@ -2392,6 +2425,15 @@ static void icl_ddi_combo_vswing_program(struct intel_encoder *encoder,
> level = n_entries - 1;
> }
>
> + if (type == INTEL_OUTPUT_EDP) {
> + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> + val = EDP4K2K_MODE_OVRD_EN | EDP4K2K_MODE_OVRD_OPTIMIZED;
> + intel_dp->hobl_active = is_hobl_buf_trans(ddi_translations);
Slightly irks me that we basically now compute this piece of state in
the middle here. But not sure if there's a better way. Anything else
might require yet another call site for the get_buf_trans() stuff
which is probably not good from maintenance POV given the current
code structure.
Also not a huge fan of doing this special programming for eDP only.
But as long as we need to mutate intel_dp->whatever I guess we need
some checks. Maybe it would be nicer to pass the encoder to
is_hobl_buf_trans() and doing the check in there. Or I guess adding
one more extra layer of abstraction would be one option. But probably
not worth the hassle.
> + intel_de_rmw(dev_priv, ICL_PORT_CL_DW10(phy), val,
> + intel_dp->hobl_active ? val : 0);
> + }
> +
> /* Set PORT_TX_DW5 */
> val = intel_de_read(dev_priv, ICL_PORT_TX_DW5_LN0(phy));
> val &= ~(SCALING_MODE_SEL_MASK | RTERM_SELECT_MASK |
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e8f809161c75..5e6634b55e84 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1375,6 +1375,8 @@ struct intel_dp {
>
> /* Display stream compression testing */
> bool force_dsc_en;
> +
> + u8 hobl_failed : 1, hobl_active : 1;
Pls use actual bools. Also if we're worried about the size we
should probably pack way more stuff to bitfiels. Otherwise IMO
separate bools like everything else should do just fine.
With those as bool this is
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> };
>
> enum lspcon_vendor {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index a23ed7290843..f2c8b56be9ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -410,10 +410,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> intel_connector->base.base.id,
> intel_connector->base.name,
> intel_dp->link_rate, intel_dp->lane_count);
> - if (!intel_dp_get_link_train_fallback_values(intel_dp,
> - intel_dp->link_rate,
> - intel_dp->lane_count))
> - /* Schedule a Hotplug Uevent to userspace to start modeset */
> - schedule_work(&intel_connector->modeset_retry_work);
> - return;
> +
> + if (intel_dp->hobl_active) {
> + drm_dbg_kms(&dp_to_i915(intel_dp)->drm,
> + "Link Training failed with HOBL active, not enabling it from now on");
> + intel_dp->hobl_failed = true;
> + } else if (intel_dp_get_link_train_fallback_values(intel_dp,
> + intel_dp->link_rate,
> + intel_dp->lane_count)) {
> + return;
> + }
> +
> + /* Schedule a Hotplug Uevent to userspace to start modeset */
> + schedule_work(&intel_connector->modeset_retry_work);
> }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 86a23ced051b..ea16931c0fa4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1898,6 +1898,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> #define PWR_DOWN_LN_3_1_0 (0xb << 4)
> #define PWR_DOWN_LN_MASK (0xf << 4)
> #define PWR_DOWN_LN_SHIFT 4
> +#define EDP4K2K_MODE_OVRD_EN (1 << 3)
> +#define EDP4K2K_MODE_OVRD_OPTIMIZED (1 << 2)
>
> #define ICL_PORT_CL_DW12(phy) _MMIO(_ICL_PORT_CL_DW(12, phy))
> #define ICL_LANE_ENABLE_AUX (1 << 0)
> --
> 2.27.0
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
