[PATCH] drm/bridge: ti-sn65dsi83: Add and use hs_rate and lp_rate

[Maxime Ripard]

Hi,

On Sun, Sep 18, 2022 at 02:56:00PM +0200, Marek Vasut wrote:
> On 8/1/22 15:11, Marek Vasut wrote:
> > Fill in hs_rate and lp_rate to struct mipi_dsi_device for this bridge and
> > adjust DSI input frequency calculations such that they expect the DSI host
> > to configure HS clock according to hs_rate.
> > 
> > This is an optimization for the DSI burst mode case. In case the DSI device
> > supports DSI burst mode, it is recommended to operate the DSI interface at
> > the highest possible HS clock frequency which the DSI device supports. This
> > permits the DSI host to send as short as possible bursts of data on the DSI
> > link and keep the DSI data lanes in LP mode otherwise, which reduces power
> > consumption.
>
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Cc: Jagan Teki <jagan at amarulasolutions.com>
> > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Cc: Linus Walleij <linus.walleij at linaro.org>
> > Cc: Robert Foss <robert.foss at linaro.org>
> > Cc: Sam Ravnborg <sam at ravnborg.org>
> > Cc: dri-devel at lists.freedesktop.org
> > ---
> >   drivers/gpu/drm/bridge/ti-sn65dsi83.c | 25 +++++++++++++------------
> >   1 file changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index 14e7aa77e7584..b161f25c3a2f5 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -286,8 +286,7 @@ static u8 sn65dsi83_get_lvds_range(struct sn65dsi83 *ctx,
> >   	return (mode_clock - 12500) / 25000;
> >   }
> > -static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx,
> > -				  const struct drm_display_mode *mode)
> > +static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx)
> >   {
> >   	/*
> >   	 * The encoding of the CHA_DSI_CLK_RANGE is as follows:
> > @@ -303,20 +302,20 @@ static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx,
> >   	 *  DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
> >   	 * the 2 is there because the bus is DDR.
> >   	 */
> > -	return DIV_ROUND_UP(clamp((unsigned int)mode->clock *
> > -			    mipi_dsi_pixel_format_to_bpp(ctx->dsi->format) /
> > -			    ctx->dsi->lanes / 2, 40000U, 500000U), 5000U);
> > +	return DIV_ROUND_UP(ctx->dsi->hs_rate, 5000000U);
> >   }
> > -static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> > +static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx,
> > +				const struct drm_display_mode *mode)
> >   {
> >   	/* The divider is (DSI_CLK / LVDS_CLK) - 1, which really is: */
> > -	unsigned int dsi_div = mipi_dsi_pixel_format_to_bpp(ctx->dsi->format);
> > +	unsigned int dsi_div;
> > +	int mode_clock = mode->clock;
> > -	dsi_div /= ctx->dsi->lanes;
> > +	if (ctx->lvds_dual_link)
> > +		mode_clock /= 2;
> > -	if (!ctx->lvds_dual_link)
> > -		dsi_div /= 2;
> > +	dsi_div = (ctx->dsi->hs_rate / mode_clock) / 1000;
> >   	return dsi_div - 1;
> >   }
> > @@ -397,9 +396,9 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> >   		     REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx, mode)) |
> >   		     REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
> >   	regmap_write(ctx->regmap, REG_DSI_CLK,
> > -		     REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx, mode)));
> > +		     REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
> >   	regmap_write(ctx->regmap, REG_RC_DSI_CLK,
> > -		     REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
> > +		     REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx, mode)));
> >   	/* Set number of DSI lanes and LVDS link config. */
> >   	regmap_write(ctx->regmap, REG_DSI_LANE,
> > @@ -643,6 +642,8 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
> >   	dsi->lanes = dsi_lanes;
> >   	dsi->format = MIPI_DSI_FMT_RGB888;
> >   	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
> > +	dsi->hs_rate = 500000000;
> > +	dsi->lp_rate = 16000000;

Let's leave aside the comment from Dave that the host might choose a
lower HS rate, we can indeed assume it's true for now.

However.. Is there any guarantee that the host can even reach that
frequency in the first place? IIRC, the maximum rate a DSI host can
reach is implementation specific. So I'm not sure this solution flies.

It's not clear to me from that patch what problem / issue it's supposed
to solve in the first place, but it really looks similar to the
discussion we had some time ago about that bridge that could only
operate at a set of fixed frequencies.

You basically want to propagate the clock constraints along the bridge
chain, and make sure every body is fine with that. The most reasonable
would be to make it part of the bridge state, and possibly add a bunch
of extra functions to query the upstream bridge clock output for a given
state.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220919/7b9d85ca/attachment.sig>