Re: [PATCH v2 9/9] drm/bridge: ti-sn65dsi86: Avoid invalid rates

From: Rob Clark
Date: Tue Dec 17 2019 - 23:01:36 EST

On Tue, Dec 17, 2019 at 4:48 PM Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Based on work by Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>,
> Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx>, and
> Rob Clark <robdclark@xxxxxxxxxxxx>.
>
> Let's read the SUPPORTED_LINK_RATES and/or MAX_LINK_RATE (depending on
> the eDP version of the sink) to figure out what eDP rates are
> supported and pick the ideal one.
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - Patch ("Avoid invalid rates") replaces ("Skip non-standard DP rates")
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 118 ++++++++++++++++++++------
> 1 file changed, 93 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index e1b817ccd9c7..da5ddf6be92b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -475,39 +475,103 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata)
> return i;
> }
>
> -static int ti_sn_bridge_get_max_dp_rate_idx(struct ti_sn_bridge *pdata)
> +static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
> + bool rate_valid[])
> {
> - u8 data;
> + u8 dpcd_val;
> + int rate_times_200khz;
> int ret;
> + int i;
>
> - ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &data);
> + ret = drm_dp_dpcd_readb(&pdata->aux, DP_EDP_DPCD_REV, &dpcd_val);
> + if (ret != 1) {
> + DRM_DEV_ERROR(pdata->dev,
> + "Can't read eDP rev (%d), assuming 1.1\n", ret);
> + dpcd_val = DP_EDP_11;
> + }
> +
> + if (dpcd_val >= DP_EDP_14) {
> + /* eDP 1.4 devices must provide a custom table */
> + __le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> +
> + ret = drm_dp_dpcd_read(&pdata->aux, DP_SUPPORTED_LINK_RATES,
> + sink_rates, sizeof(sink_rates));
> +
> + if (ret != sizeof(sink_rates)) {
> + DRM_DEV_ERROR(pdata->dev,
> + "Can't read supported rate table (%d)\n", ret);
> +
> + /* By zeroing we'll fall back to DP_MAX_LINK_RATE. */
> + memset(sink_rates, 0, sizeof(sink_rates));
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> + rate_times_200khz = le16_to_cpu(sink_rates[i]);
> +
> + if (!rate_times_200khz)
> + break;
> +
> + switch (rate_times_200khz) {
> + case 27000:

maybe a bit bike-sheddy, but I kinda prefer to use traditional normal
units, ie. just multiply the returned value by 200 and adjust the
switch case values. At least then they match the values in the lut
(other than khz vs mhz), which makes this whole logic a bit more
obvious... and at that point, maybe just loop over the lut table?

BR,
-R

> + rate_valid[7] = 1;
> + break;
> + case 21600:
> + rate_valid[6] = 1;
> + break;
> + case 16200:
> + rate_valid[5] = 1;
> + break;
> + case 13500:
> + rate_valid[4] = 1;
> + break;
> + case 12150:
> + rate_valid[3] = 1;
> + break;
> + case 10800:
> + rate_valid[2] = 1;
> + break;
> + case 8100:
> + rate_valid[1] = 1;
> + break;
> + default:
> + /* unsupported */
> + break;
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); i++) {
> + if (rate_valid[i])
> + return;
> + }
> + DRM_DEV_ERROR(pdata->dev,
> + "No matching eDP rates in table; falling back\n");
> + }
> +
> + /* On older versions best we can do is use DP_MAX_LINK_RATE */
> + ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &dpcd_val);
> if (ret != 1) {
> DRM_DEV_ERROR(pdata->dev,
> "Can't read max rate (%d); assuming 5.4 GHz\n",
> ret);
> - return ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1;
> + dpcd_val = DP_LINK_BW_5_4;
> }
>
> - /*
> - * Return an index into ti_sn_bridge_dp_rate_lut. Just hardcode
> - * these indicies since it's not like the register spec is ever going
> - * to change and a loop would just be more complicated. Apparently
> - * the DP sink can only return these few rates as supported even
> - * though the bridge allows some rates in between.
> - */
> - switch (data) {
> - case DP_LINK_BW_1_62:
> - return 1;
> - case DP_LINK_BW_2_7:
> - return 4;
> + switch (dpcd_val) {
> + default:
> + DRM_DEV_ERROR(pdata->dev,
> + "Unexpected max rate (%#x); assuming 5.4 GHz\n",
> + (int)dpcd_val);
> + /* fall through */
> case DP_LINK_BW_5_4:
> - return 7;
> + rate_valid[7] = 1;
> + /* fall through */
> + case DP_LINK_BW_2_7:
> + rate_valid[4] = 1;
> + /* fall through */
> + case DP_LINK_BW_1_62:
> + rate_valid[1] = 1;
> + break;
> }
> -
> - DRM_DEV_ERROR(pdata->dev,
> - "Unexpected max data rate (%#x); assuming 5.4 GHz\n",
> - (int)data);
> - return ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1;
> }
>
> static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
> @@ -609,9 +673,9 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx,
> static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> + bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)];
> const char *last_err_str = "No supported DP rate";
> int dp_rate_idx;
> - int max_dp_rate_idx;
> unsigned int val;
> int ret = -EINVAL;
>
> @@ -655,11 +719,15 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
> val);
>
> + ti_sn_bridge_read_valid_rates(pdata, rate_valid);
> +
> /* Train until we run out of rates */
> - max_dp_rate_idx = ti_sn_bridge_get_max_dp_rate_idx(pdata);
> for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> - dp_rate_idx <= max_dp_rate_idx;
> + dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> dp_rate_idx++) {
> + if (!rate_valid[dp_rate_idx])
> + continue;
> +
> ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
> if (!ret)
> break;
> --
> 2.24.1.735.g03f4e72817-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel