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

From: Rob Clark
Date: Tue Dec 17 2019 - 23:03:42 EST

On Tue, Dec 17, 2019 at 8:01 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> 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?

(hit SEND too soon)

and other than that, lgtm but haven't had a chance to test it yet
(although I guess none of us have an eDP 1.4+ screen so maybe that is
moot :-))

BR,
-R


> 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