Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Decouple DP output lanes from DSI input lanes

[Doug Anderson]

Hi,

On Tue, Oct 22, 2019 at 12:01 PM Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx> wrote:
>
> Based on work by Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>
> The bridge can be configured to use 1, 2, or 4 DP lanes.  This
> configuration is independent of the input DSI lanes.  Right now, the
> driver assumes that there is 1:1 mapping of input lanes to output lanes
> which is not correct and does not work for manu devices such as the
> Lenovo Miix 630 and Lenovo Yoga C630 laptops.
>
> The bridge can also be configured to use one of a number of data rates on
> the DP lanes.  Currently any of the supported rates is considered valid,
> however the configured rate must also be supported by the connected panel,
> and not all rates are supported or even valid for any particular panel.
>
> Luckily, we can determine what we need at runtime by reading the DPCD from
> the attached panel.  DPCD will tell us the maximum number of supported
> lanes, and the supported data rates.
>
> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx>
> ---
>
> Bjorn, I think this should address the issue you pointed out concerning
> the data rate glitch I missed in your origional work.  Would you kindly
> give this a test and let me know if it appears to address all of the
> issues you were working around?
>
> v2:
> -Use DPCD instead of DT to address the issue of some panels not
> supporting all the rates
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 97 ++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 3 deletions(-)

Overall I'm suggesting moving over to my series and I've taken the
best stuff from your patch and put it atop my series.  Please yell if
you disagree.  You can find the cover letter for my v2 at:

https://lore.kernel.org/r/20191218004741.102067-1-dianders@xxxxxxxxxxxx

A few misc comments below in any case.


> @@ -444,10 +447,91 @@ static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
>         regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>
>         /* set DP data rate */
> -       dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
> +       dp_rate_mhz = ((bit_rate_mhz / pdata->dp_lanes) * DP_CLK_FUDGE_NUM) /
>                                                         DP_CLK_FUDGE_DEN;

One note is that "bit_rate_mhz" is still calculated using the MIPI
pixel format, which is wrong.  It happens that (at the moment) we have
24 bits per pixel for both cases, though.


> +       /* read the panel capabilities to determine valid supported rates */
> +       val = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LINK_RATE, &dpcd_val);
> +       if (!val) {
> +               DRM_ERROR("Reading max link rate from DPCD failed\n");
> +               return;
> +       }
> +
> +       if (dpcd_val) {

I think your patch is assuming that the only case you want to use the
table is if dpcd_val is 0.  This doesn't appear to be the case.  In
Table 4-24 of the spec it states that you can have a non-zero value
here and still provide a table.  That might be useful if you want to
be backward compatible with an eDP 1.3 source but also provide an
optimized rate for a eDP 1.4 source.

We should be checking the eDP revision first and always using the
table if it's eDP 1.4.


> +               /* cap to the max rate supported by the bridge */
> +               if (dpcd_val > 0x14)
> +                       dpcd_val = 0x14;
> +
> +               switch (dpcd_val) {
> +               case 0x14:

There are constants.  Like DP_LINK_BW_5_4


> @@ -505,7 +589,14 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>                            CHA_DSI_LANES_MASK, val);
>
>         /* DP lane config */
> -       val = DP_NUM_LANES(pdata->dsi->lanes - 1);
> +       ret = drm_dp_dpcd_readb(&pdata->aux, DP_MAX_LANE_COUNT, (u8 *)&val);

This is an iffy cast.  The function is only guaranteed to set the
first byte of val which could be the most or least significant byte.
Other bytes will remain as-is.