Re: [PATCH] drm/dp: Correctly mask DP_TRAINING_AUX_RD_INTERVAL values for DP 1.4

Manasi Navare
2018-03-08T00:18:33Z

On Wed, Mar 07, 2018 at 03:44:09PM -0800, matthew.s.atwood@intel.com wrote:
> From: Matt Atwood <matthew.s.atwood@intel.com>
>
> DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheeme from 8
> bits to 7 in DPCD 0x000e. The 8th bit is used to identify extended
> receiver capabilities. For panels that use this new feature wait interval
> would be increased by 512 ms, when spec is max 16 ms. This behavior is
> described in table 2-158 of DP 1.4 spec address 0000eh.
>
> With the introduction of DP 1.4 spec main link clock recovery was
> standardized to 100 us regardless of TRAINING_AUX_RD_INTERVAL value.
>
> To avoid breaking panels that are not spec compiant we now warn on
> invalid values.
>
> V2: commit title/message, masking all 7 bits, warn on out of spec values.
> V3: commit message, make link train clock recovery follow DP 1.4 spec.
>
> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> ---
> drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++----
> include/drm/drm_dp_helper.h | 4 ++++
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index adf79be..671b823 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
> EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis);
>
> void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
> - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0)
> + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK;
> +
> + if (rd_interval > 4)
> + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval);

I thought there were comments about setting this to a max of 4 if its greater
than 4.

> +
> + if(rd_interval == 0 || (dpcd[DP_DPCD_REV] & DP_REV_14))
^ space needed between if and open bracket

> udelay(100);
> else
> - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4);
> + mdelay(rd_interval * 4)
> }
> EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay);
>
> void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
> - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0)
> + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK;
> +
> + if (rd_interval > 4)
> + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval);
> +
> + if (rd_interval == 0)
> udelay(400);
> else
> - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4);
> + mdelay(rd_interval * 4);
> }
> EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index da58a42..5bac397 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -64,6 +64,9 @@
> /* AUX CH addresses */
> /* DPCD */
> #define DP_DPCD_REV 0x000
> +# define DP_REV_12 0x012
> +# define DP_REV_13 0x013
> +# define DP_REV_14 0x014
>

IMHO, if we are creating these #defines for revisions then we should
do it for all values so 0x10, 0x11.

Manasi

> #define DP_MAX_LINK_RATE 0x001
>
> @@ -118,6 +121,7 @@
> # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */
>
> #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */
> +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */
>
> #define DP_ADAPTER_CAP 0x00f /* 1.2 */
> # define DP_FORCE_LOAD_SENSE_CAP (1 << 0)
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx