| Message ID | 20191213154448.9.I1791f91dd22894da04f86699a7507d101d4385bc@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Improve support for AUO B116XAK01 + other low res DP | expand |
On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote: > The bridge chip supports these DP rates according to TI's spec: > * 1.62 Gbps (RBR) > * 2.16 Gbps > * 2.43 Gbps > * 2.7 Gbps (HBR) > * 3.24 Gbps > * 4.32 Gbps > * 5.4 Gbps (HBR2) > > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec. > If other rates work then I believe it's because the sink has allowed > bending the spec a little bit. I think you need to look at the eDP spec. And filter this stuff correctly (there's more fields there for these somewhat irky edp timings). Simply not using them works, but it's defeating the point of having these intermediate clocks for edp panels. -Daniel > > I hoped that we could tell which rates would work and which rates > didn't work based on whether link training passed or not. > Unfortunately this wasn't so good on at least one panel hooked up to > the bridge (AUO B116XAK01). On that panel with 24 bpp configured: > * 1.62: too small for 69500 kHz at 24 bpp > * 2.16: link training failed > * 2.43: link training passed, but garbage on screen > * 2.7: joy and happiness > > Let's bypass all non-standard rates, which makes this panel happy > working. I'll still keep the code organized in such a way where it > _could_ try the other rates, though, on the assumption that eventually > someone will find a way to make use of them. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index cc8bef172f69..cb774ee536cd 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -454,6 +454,15 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { > 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 > }; > > +/** > + * A table indicating which of the rates in ti_sn_bridge_dp_rate_lut > + * is as per the DP spec (AKA a standard) as opposed to an intermediate > + * rate. > + */ > +static const bool ti_sn_bridge_dp_rate_standard[] = { > + false, true, false, false, true, false, false, true > +}; > + > static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata) > { > unsigned int bit_rate_khz, dp_rate_mhz; > @@ -660,6 +669,18 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) > for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); > dp_rate_idx <= max_dp_rate_idx; > dp_rate_idx++) { > + /* > + * To be on the safe side, we'll skip all non-standard > + * rates and move up to the next standard one. This is > + * because some panels will pass link training with a non- > + * standard rate but just show garbage. If the non-standard > + * rates are useful we should figure out how to enable them > + * through querying the panel, having a per-panel whitelist, > + * or adding a DT property. > + */ > + if (!ti_sn_bridge_dp_rate_standard[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 >
Hi, On Fri, Dec 13, 2019 at 4:07 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote: > > The bridge chip supports these DP rates according to TI's spec: > > * 1.62 Gbps (RBR) > > * 2.16 Gbps > > * 2.43 Gbps > > * 2.7 Gbps (HBR) > > * 3.24 Gbps > > * 4.32 Gbps > > * 5.4 Gbps (HBR2) > > > > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec. > > If other rates work then I believe it's because the sink has allowed > > bending the spec a little bit. > > I think you need to look at the eDP spec. And filter this stuff correctly > (there's more fields there for these somewhat irky edp timings). Simply > not using them works, but it's defeating the point of having these > intermediate clocks for edp panels. Ah, I see my problem. I had earlier only found the eDP 1.3 spec which doesn't mention these rates. The eDP 1.4 spec does, however. ...and the change log for 1.4 specifically mentions that it added 4 new link rates and also adds the "SUPPORTED_LINK_RATES" register. I can try to spin a v2 but for now I'll hold off for additional feedback. I'll also note that I'd be totally OK if just the first 8 patches in this series landed for now and someone could eventually figure out how to make this work. With just the first 8 patches I think we will still be in an improved state compared to where we were before (and it fixes the panel I care about) and someone could later write the code to skip unsupported rates... -Doug
On Fri, Dec 13, 2019 at 5:49 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Dec 13, 2019 at 4:07 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote: > > > The bridge chip supports these DP rates according to TI's spec: > > > * 1.62 Gbps (RBR) > > > * 2.16 Gbps > > > * 2.43 Gbps > > > * 2.7 Gbps (HBR) > > > * 3.24 Gbps > > > * 4.32 Gbps > > > * 5.4 Gbps (HBR2) > > > > > > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec. > > > If other rates work then I believe it's because the sink has allowed > > > bending the spec a little bit. > > > > I think you need to look at the eDP spec. And filter this stuff correctly > > (there's more fields there for these somewhat irky edp timings). Simply > > not using them works, but it's defeating the point of having these > > intermediate clocks for edp panels. > > Ah, I see my problem. I had earlier only found the eDP 1.3 spec which > doesn't mention these rates. The eDP 1.4 spec does, however. ...and > the change log for 1.4 specifically mentions that it added 4 new link > rates and also adds the "SUPPORTED_LINK_RATES" register. Yeah, you need the eDP spec. I previously posted https://patchwork.kernel.org/patch/11205201/ and was hoping Bjorn would find time to test it. Maybe it would fit well with your series? I'm coming back from tracel, and hope to review everything you have, but this caught my eye. > > I can try to spin a v2 but for now I'll hold off for additional feedback. > > I'll also note that I'd be totally OK if just the first 8 patches in > this series landed for now and someone could eventually figure out how > to make this work. With just the first 8 patches I think we will > still be in an improved state compared to where we were before (and it > fixes the panel I care about) and someone could later write the code > to skip unsupported rates... > > > -Doug
Hi, On Sun, Dec 15, 2019 at 5:19 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > On Fri, Dec 13, 2019 at 5:49 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Fri, Dec 13, 2019 at 4:07 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Fri, Dec 13, 2019 at 03:45:30PM -0800, Douglas Anderson wrote: > > > > The bridge chip supports these DP rates according to TI's spec: > > > > * 1.62 Gbps (RBR) > > > > * 2.16 Gbps > > > > * 2.43 Gbps > > > > * 2.7 Gbps (HBR) > > > > * 3.24 Gbps > > > > * 4.32 Gbps > > > > * 5.4 Gbps (HBR2) > > > > > > > > As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec. > > > > If other rates work then I believe it's because the sink has allowed > > > > bending the spec a little bit. > > > > > > I think you need to look at the eDP spec. And filter this stuff correctly > > > (there's more fields there for these somewhat irky edp timings). Simply > > > not using them works, but it's defeating the point of having these > > > intermediate clocks for edp panels. > > > > Ah, I see my problem. I had earlier only found the eDP 1.3 spec which > > doesn't mention these rates. The eDP 1.4 spec does, however. ...and > > the change log for 1.4 specifically mentions that it added 4 new link > > rates and also adds the "SUPPORTED_LINK_RATES" register. > > Yeah, you need the eDP spec. I previously posted > https://patchwork.kernel.org/patch/11205201/ and was hoping Bjorn > would find time to test it. Maybe it would fit well with your series? > I'm coming back from tracel, and hope to review everything you have, > but this caught my eye. Ah, interesting. It looks like Rob has already posted a Fixup on my patch: https://lore.kernel.org/r/20191215200632.1019065-1-robdclark@gmail.com ...that should also read the supported rates. I need to go and review / test his new patch (I lost access to the hardware but should get it back tomorrow or the next day), but would you be OK with going that route? I think my series is a superset of yours. Specifically it has these extra features atop yours: * If link training fails and the panel supports faster rates, it will try a faster rate in case it works. * It adds support for using 6bpp when that's all that's needed, reducing bandwidth to the panel (and link rate) * It breaks things into smaller functions (assuming you agree this is a good thing). -Doug
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index cc8bef172f69..cb774ee536cd 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -454,6 +454,15 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 }; +/** + * A table indicating which of the rates in ti_sn_bridge_dp_rate_lut + * is as per the DP spec (AKA a standard) as opposed to an intermediate + * rate. + */ +static const bool ti_sn_bridge_dp_rate_standard[] = { + false, true, false, false, true, false, false, true +}; + static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn_bridge *pdata) { unsigned int bit_rate_khz, dp_rate_mhz; @@ -660,6 +669,18 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); dp_rate_idx <= max_dp_rate_idx; dp_rate_idx++) { + /* + * To be on the safe side, we'll skip all non-standard + * rates and move up to the next standard one. This is + * because some panels will pass link training with a non- + * standard rate but just show garbage. If the non-standard + * rates are useful we should figure out how to enable them + * through querying the panel, having a per-panel whitelist, + * or adding a DT property. + */ + if (!ti_sn_bridge_dp_rate_standard[dp_rate_idx]) + continue; + ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str); if (!ret) break;
The bridge chip supports these DP rates according to TI's spec: * 1.62 Gbps (RBR) * 2.16 Gbps * 2.43 Gbps * 2.7 Gbps (HBR) * 3.24 Gbps * 4.32 Gbps * 5.4 Gbps (HBR2) As far as I can tell, only RBR, HBR, and HBR2 are part of the DP spec. If other rates work then I believe it's because the sink has allowed bending the spec a little bit. I hoped that we could tell which rates would work and which rates didn't work based on whether link training passed or not. Unfortunately this wasn't so good on at least one panel hooked up to the bridge (AUO B116XAK01). On that panel with 24 bpp configured: * 1.62: too small for 69500 kHz at 24 bpp * 2.16: link training failed * 2.43: link training passed, but garbage on screen * 2.7: joy and happiness Let's bypass all non-standard rates, which makes this panel happy working. I'll still keep the code organized in such a way where it _could_ try the other rates, though, on the assumption that eventually someone will find a way to make use of them. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)