| Message ID | 20191217164702.v2.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Improve support for AUO B116XAK01 + other DP | expand |
On Tue, Dec 17, 2019 at 4:48 PM Douglas Anderson <dianders@chromium.org> wrote: > > Based on work by Bjorn Andersson <bjorn.andersson@linaro.org>, > Jeffrey Hugo <jeffrey.l.hugo@gmail.com>, and > Rob Clark <robdclark@chromium.org>. > > 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@chromium.org> > --- > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Dec 17, 2019 at 8:01 PM Rob Clark <robdclark@gmail.com> wrote: > > On Tue, Dec 17, 2019 at 4:48 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > Based on work by Bjorn Andersson <bjorn.andersson@linaro.org>, > > Jeffrey Hugo <jeffrey.l.hugo@gmail.com>, and > > Rob Clark <robdclark@chromium.org>. > > > > 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@chromium.org> > > --- > > > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On Tue, Dec 17, 2019 at 8:03 PM Rob Clark <robdclark@gmail.com> wrote: > > > > + 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 :-)) I think v3 should look much better to you. I also added a note to the commit log indicating that the DP 1.4 patch was only tested via hackery... https://lore.kernel.org/r/20191218143416.v3.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid -Doug
Hi Douglas,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.5-rc2 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Douglas-Anderson/drm-bridge-ti-sn65dsi86-Improve-support-for-AUO-B116XAK01-other-DP/20191221-083448
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7e0165b2f1a912a06e381e91f0f4e495f4ac3736
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function 'ti_sn_bridge_enable':
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:543:18: warning: 'rate_valid' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (rate_valid[i])
~~~~~~~~~~^~~
vim +/rate_valid +543 drivers/gpu/drm/bridge/ti-sn65dsi86.c
477
478 static void ti_sn_bridge_read_valid_rates(struct ti_sn_bridge *pdata,
479 bool rate_valid[])
480 {
481 u8 dpcd_val;
482 int rate_times_200khz;
483 int ret;
484 int i;
485
486 ret = drm_dp_dpcd_readb(&pdata->aux, DP_EDP_DPCD_REV, &dpcd_val);
487 if (ret != 1) {
488 DRM_DEV_ERROR(pdata->dev,
489 "Can't read eDP rev (%d), assuming 1.1\n", ret);
490 dpcd_val = DP_EDP_11;
491 }
492
493 if (dpcd_val >= DP_EDP_14) {
494 /* eDP 1.4 devices must provide a custom table */
495 __le16 sink_rates[DP_MAX_SUPPORTED_RATES];
496
497 ret = drm_dp_dpcd_read(&pdata->aux, DP_SUPPORTED_LINK_RATES,
498 sink_rates, sizeof(sink_rates));
499
500 if (ret != sizeof(sink_rates)) {
501 DRM_DEV_ERROR(pdata->dev,
502 "Can't read supported rate table (%d)\n", ret);
503
504 /* By zeroing we'll fall back to DP_MAX_LINK_RATE. */
505 memset(sink_rates, 0, sizeof(sink_rates));
506 }
507
508 for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
509 rate_times_200khz = le16_to_cpu(sink_rates[i]);
510
511 if (!rate_times_200khz)
512 break;
513
514 switch (rate_times_200khz) {
515 case 27000:
516 rate_valid[7] = 1;
517 break;
518 case 21600:
519 rate_valid[6] = 1;
520 break;
521 case 16200:
522 rate_valid[5] = 1;
523 break;
524 case 13500:
525 rate_valid[4] = 1;
526 break;
527 case 12150:
528 rate_valid[3] = 1;
529 break;
530 case 10800:
531 rate_valid[2] = 1;
532 break;
533 case 8100:
534 rate_valid[1] = 1;
535 break;
536 default:
537 /* unsupported */
538 break;
539 }
540 }
541
542 for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); i++) {
> 543 if (rate_valid[i])
544 return;
545 }
546 DRM_DEV_ERROR(pdata->dev,
547 "No matching eDP rates in table; falling back\n");
548 }
549
550 /* On older versions best we can do is use DP_MAX_LINK_RATE */
551 ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &dpcd_val);
552 if (ret != 1) {
553 DRM_DEV_ERROR(pdata->dev,
554 "Can't read max rate (%d); assuming 5.4 GHz\n",
555 ret);
556 dpcd_val = DP_LINK_BW_5_4;
557 }
558
559 switch (dpcd_val) {
560 default:
561 DRM_DEV_ERROR(pdata->dev,
562 "Unexpected max rate (%#x); assuming 5.4 GHz\n",
563 (int)dpcd_val);
564 /* fall through */
565 case DP_LINK_BW_5_4:
566 rate_valid[7] = 1;
567 /* fall through */
568 case DP_LINK_BW_2_7:
569 rate_valid[4] = 1;
570 /* fall through */
571 case DP_LINK_BW_1_62:
572 rate_valid[1] = 1;
573 break;
574 }
575 }
576
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
Dear Robot, On Sat, Dec 21, 2019 at 5:57 AM kbuild test robot <lkp@intel.com> wrote: > > Hi Douglas, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on linus/master] > [also build test WARNING on v5.5-rc2 next-20191220] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Douglas-Anderson/drm-bridge-ti-sn65dsi86-Improve-support-for-AUO-B116XAK01-other-DP/20191221-083448 > base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7e0165b2f1a912a06e381e91f0f4e495f4ac3736 > config: sh-allmodconfig (attached as .config) > compiler: sh4-linux-gcc (GCC) 7.5.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.5.0 make.cross ARCH=sh > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > Note: it may well be a FALSE warning. FWIW you are at least aware of it now. > http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings > > All warnings (new ones prefixed by >>): > > drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function 'ti_sn_bridge_enable': > >> drivers/gpu/drm/bridge/ti-sn65dsi86.c:543:18: warning: 'rate_valid' may be used uninitialized in this function [-Wmaybe-uninitialized] > if (rate_valid[i]) > ~~~~~~~~~~^~~ I love your report! Interestingly I had already noticed this problem myself and v3 of the patch fixes the issue. See: https://lore.kernel.org/r/20191218143416.v3.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid If the maintainer of the robot is reading this, something to improve about your robot is that it could have noticed v3 of the patch (which was posted several days before your report) and skipped analyzing v2 of the patch. I'm currently using Change-Ids embedded in my Message-Id to help automation relate one version of my patches to the next. Specifically you compare the Message-Id of v2 and v3 of this patch: 20191217164702.v2.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid 20191218143416.v3.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid Since the last section before the "@changeid" remained constant it could be assumed that this patch replaced the v2. I know there's not too much usage of this technique yet, but if only more tools supported it then maybe more people would use it. -Doug
On 1/7/20 6:43 AM, Doug Anderson wrote: > Dear Robot, > > On Sat, Dec 21, 2019 at 5:57 AM kbuild test robot <lkp@intel.com> wrote: >> Hi Douglas, >> >> I love your patch! Perhaps something to improve: >> >> [auto build test WARNING on linus/master] >> [also build test WARNING on v5.5-rc2 next-20191220] >> [if your patch is applied to the wrong git tree, please drop us a note to help >> improve the system. BTW, we also suggest to use '--base' option to specify the >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982] >> >> url: https://github.com/0day-ci/linux/commits/Douglas-Anderson/drm-bridge-ti-sn65dsi86-Improve-support-for-AUO-B116XAK01-other-DP/20191221-083448 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7e0165b2f1a912a06e381e91f0f4e495f4ac3736 >> config: sh-allmodconfig (attached as .config) >> compiler: sh4-linux-gcc (GCC) 7.5.0 >> reproduce: >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # save the attached .config to linux build tree >> GCC_VERSION=7.5.0 make.cross ARCH=sh >> >> If you fix the issue, kindly add following tag >> Reported-by: kbuild test robot <lkp@intel.com> >> >> Note: it may well be a FALSE warning. FWIW you are at least aware of it now. >> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings >> >> All warnings (new ones prefixed by >>): >> >> drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function 'ti_sn_bridge_enable': >>>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:543:18: warning: 'rate_valid' may be used uninitialized in this function [-Wmaybe-uninitialized] >> if (rate_valid[i]) >> ~~~~~~~~~~^~~ > I love your report! Interestingly I had already noticed this problem > myself and v3 of the patch fixes the issue. See: > > https://lore.kernel.org/r/20191218143416.v3.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid > > > If the maintainer of the robot is reading this, something to improve > about your robot is that it could have noticed v3 of the patch (which > was posted several days before your report) and skipped analyzing v2 > of the patch. I'm currently using Change-Ids embedded in my > Message-Id to help automation relate one version of my patches to the > next. Specifically you compare the Message-Id of v2 and v3 of this > patch: > > 20191217164702.v2.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid > 20191218143416.v3.9.Ib59207b66db377380d13748752d6fce5596462c5@changeid > > Since the last section before the "@changeid" remained constant it > could be assumed that this patch replaced the v2. I know there's not > too much usage of this technique yet, but if only more tools supported > it then maybe more people would use it. Hi Doug, Thanks for your suggestion, the root cause is that the v3 wasn't handled before this report. We'll definitely give serious thought to your suggestion. v2: Douglas-Anderson/drm-bridge-ti-sn65dsi86-Improve-support-for-AUO-B116XAK01-other-DP/20191221-083448 v3: Douglas-Anderson/drm-bridge-ti-sn65dsi86-Improve-support-for-AUO-B116XAK01-other-DP/20191222-062646 Best Regards, Rong Chen > > > -Doug > _______________________________________________ > kbuild-all mailing list -- kbuild-all@lists.01.org > To unsubscribe send an email to kbuild-all-leave@lists.01.org
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: + 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;
Based on work by Bjorn Andersson <bjorn.andersson@linaro.org>, Jeffrey Hugo <jeffrey.l.hugo@gmail.com>, and Rob Clark <robdclark@chromium.org>. 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@chromium.org> --- 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(-)