[RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state

[Dmitry Baryshkov]

On Sun, 10 Jul 2022 at 22:11, Sam Ravnborg <sam at ravnborg.org> wrote:
>
> Hi Dmitry,
>
> On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> > Rather than reading the pdata->connector directly, fetch the connector
> > using drm_atomic_state. This allows us to make pdata->connector optional
> > (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 01171547f638..df08207d6223 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> >       regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> >  }
> >
> > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> > +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> >  {
> > -     if (pdata->connector->display_info.bpc <= 6)
> > +     if (connector->display_info.bpc <= 6)
> >               return 18;
> >       else
> >               return 24;
> > @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> >       0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> >  };
> >
> > -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> > +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> >  {
> >       unsigned int bit_rate_khz, dp_rate_mhz;
> >       unsigned int i;
> > @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> >               &pdata->bridge.encoder->crtc->state->adjusted_mode;
> >
> >       /* Calculate minimum bit rate based on our pixel clock. */
> > -     bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> > +     bit_rate_khz = mode->clock * bpp;
> >
> >       /* Calculate minimum DP data rate, taking 80% as per DP spec */
> >       dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> > @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> >                                      struct drm_bridge_state *old_bridge_state)
> >  {
> >       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +     struct drm_connector *connector;
> >       const char *last_err_str = "No supported DP rate";
> >       unsigned int valid_rates;
> >       int dp_rate_idx;
> >       unsigned int val;
> >       int ret = -EINVAL;
> >       int max_dp_lanes;
> > +     unsigned int bpp;
> > +
> > +     connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> > +                                                          bridge->encoder);
> > +     if (!connector)
> > +             return;
> It would be prudent with a dev_err() logging here as we do not expect to
> fail.
> I looked into something similar, but with a less elegant solution, and
> could not convince myself that the display driver would create the
> connector before ti_sn_bridge_atomic_enable() was called.

If I understand your concern, the connectors (as does the rest of
CRTC/encoder/etc objects) are not dynamic, so they must be created
before being able to use the DRM device or any part of thereof is
being actually used (enable/disable/modeset).

>
> This is another reason why a dev_err would be nice - so tester could see
> if this fails or not.

Will fix this in v2.

>
> With the dev_err added:
> Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
>
> >
> >       max_dp_lanes = ti_sn_get_max_lanes(pdata);
> >       pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> > @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> >       drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> >                          DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
> >
> > +     bpp = ti_sn_bridge_get_bpp(connector);
> >       /* Set the DP output format (18 bpp or 24 bpp) */
> > -     val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> > +     val = (bpp == 18) ? BPP_18_RGB : 0;
> >       regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
> >
> >       /* DP lane config */
> > @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> >       valid_rates = ti_sn_bridge_read_valid_rates(pdata);
> >
> >       /* Train until we run out of rates */
> > -     for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> > +     for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> >            dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> >            dp_rate_idx++) {
> >               if (!(valid_rates & BIT(dp_rate_idx)))
> > --
> > 2.35.1



-- 
With best wishes
Dmitry