diff mbox series

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

Message ID 20220801131113.182487-1-marex@denx.de (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi83: Add and use hs_rate and lp_rate | expand

Commit Message

Marek Vasut Aug. 1, 2022, 1:11 p.m. UTC
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@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Dave Stevenson Aug. 1, 2022, 2:55 p.m. UTC | #1
Hi Marek

On Mon, 1 Aug 2022 at 14:12, Marek Vasut <marex@denx.de> 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.

I think this falls into another of the areas that is lacking in the DSI support.
hs_rate is defined as the *maximum* lane frequency in high speed
mode[1]. As documented there is no obligation on the DSI host to
choose this specific rate, just some frequency below it and above or
equal to that required by the pixel clock. At a system level, the link
frequency is often prescribed for EMC purposes.

The issue is then that the SN65DSI83 is configured to use the DSI
clock lane as the source for the PLL generating the LVDS clock,
therefore with no route for the DSI peripheral to query the link
frequency chosen by the host, you're stuck.

SN65DSI83 also supports non-burst mode (as currently), so how would
this be configured now?
Does MIPI_DSI_MODE_VIDEO_BURST [2] oblige the DSI host to run in burst
mode, or say that burst mode is supported by the peripheral? What if
your DSI host doesn't support burst mode and it is optional on your
peripheral?
I raised these questions and others at [3], but largely got no real answers.


The patch does exactly what you describe and has value, but without
definition in the frameworks of exactly how burst mode must be
implemented by the DSI host, I would say that it's going to cause
issues.
I am aware of users of your driver with the Raspberry Pi, but your
expectation isn't currently valid on the Pi as there is no definition
of the correct thing for the host to do.

Cheers.
  Dave

[1] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_mipi_dsi.h#L176
[2] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_mipi_dsi.h#L119
[3] start of thread at
https://lists.freedesktop.org/archives/dri-devel/2021-July/313576.html
and specifically hs_rate/burst at
https://lists.freedesktop.org/archives/dri-devel/2021-October/326732.html

> 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@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: dri-devel@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;
>
>         ret = devm_mipi_dsi_attach(dev, dsi);
>         if (ret < 0) {
> --
> 2.35.1
>
Marek Vasut Aug. 1, 2022, 4:04 p.m. UTC | #2
On 8/1/22 16:55, Dave Stevenson wrote:
> Hi Marek

Hi,

> On Mon, 1 Aug 2022 at 14:12, Marek Vasut <marex@denx.de> 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.
> 
> I think this falls into another of the areas that is lacking in the DSI support.
> hs_rate is defined as the *maximum* lane frequency in high speed
> mode[1]. As documented there is no obligation on the DSI host to
> choose this specific rate, just some frequency below it and above or
> equal to that required by the pixel clock. At a system level, the link
> frequency is often prescribed for EMC purposes.

The reduced upper limit could likely be defined by a DT property, but 
that's not hard to add.

> The issue is then that the SN65DSI83 is configured to use the DSI
> clock lane as the source for the PLL generating the LVDS clock,
> therefore with no route for the DSI peripheral to query the link
> frequency chosen by the host, you're stuck.

At least making the host pick the highest supported frequency means we 
have a well defined link frequency which either is accepted by both ends 
or not at all, instead of the current guesswork on both ends, bridge and 
host.

Regarding reduction of the maximum hs_rate, see above, probably use a DT 
property. Regarding check for hs_rate below minimum possible rate, 
likely something the DSI host should do based on desired mode .

If you are looking for some frequency negotiation starter, look at:
[RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock 
from DSI bridge

> SN65DSI83 also supports non-burst mode (as currently), so how would
> this be configured now?
> Does MIPI_DSI_MODE_VIDEO_BURST [2] oblige the DSI host to run in burst
> mode, or say that burst mode is supported by the peripheral?

My understanding is that it is the former -- if the flag is set, the DSI 
host must operate the device in burst mode.

> What if
> your DSI host doesn't support burst mode and it is optional on your
> peripheral?

The limit still applies. In the sync pulses mode, you can still run the 
link at high frequency, it's just that the DSI data lanes won't go into 
LP mode between bursts of data, they would stuff the link with NOPs 
instead, as far as I can tell.

> I raised these questions and others at [3], but largely got no real answers.
> 
> 
> The patch does exactly what you describe and has value, but without
> definition in the frameworks of exactly how burst mode must be
> implemented by the DSI host, I would say that it's going to cause
> issues.
> I am aware of users of your driver with the Raspberry Pi, but your
> expectation isn't currently valid on the Pi as there is no definition
> of the correct thing for the host to do.

This patch actually helped me deal with stability issues on MX8M . Of 
course, the DSIM driver has to handle the case where bridge provides 
hs_rate and configure its PLL accordingly. That's a two-liner patch.
Adam Ford Aug. 1, 2022, 4:38 p.m. UTC | #3
On Mon, Aug 1, 2022 at 11:05 AM Marek Vasut <marex@denx.de> wrote:
>
> On 8/1/22 16:55, Dave Stevenson wrote:
> > Hi Marek
>
> Hi,
>
> > On Mon, 1 Aug 2022 at 14:12, Marek Vasut <marex@denx.de> 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.
> >
> > I think this falls into another of the areas that is lacking in the DSI support.
> > hs_rate is defined as the *maximum* lane frequency in high speed
> > mode[1]. As documented there is no obligation on the DSI host to
> > choose this specific rate, just some frequency below it and above or
> > equal to that required by the pixel clock. At a system level, the link
> > frequency is often prescribed for EMC purposes.
>
> The reduced upper limit could likely be defined by a DT property, but
> that's not hard to add.
>
> > The issue is then that the SN65DSI83 is configured to use the DSI
> > clock lane as the source for the PLL generating the LVDS clock,
> > therefore with no route for the DSI peripheral to query the link
> > frequency chosen by the host, you're stuck.
>
> At least making the host pick the highest supported frequency means we
> have a well defined link frequency which either is accepted by both ends
> or not at all, instead of the current guesswork on both ends, bridge and
> host.
>
> Regarding reduction of the maximum hs_rate, see above, probably use a DT
> property. Regarding check for hs_rate below minimum possible rate,
> likely something the DSI host should do based on desired mode .
>
> If you are looking for some frequency negotiation starter, look at:
> [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock
> from DSI bridge
>
> > SN65DSI83 also supports non-burst mode (as currently), so how would
> > this be configured now?
> > Does MIPI_DSI_MODE_VIDEO_BURST [2] oblige the DSI host to run in burst
> > mode, or say that burst mode is supported by the peripheral?
>
> My understanding is that it is the former -- if the flag is set, the DSI
> host must operate the device in burst mode.
>
> > What if
> > your DSI host doesn't support burst mode and it is optional on your
> > peripheral?
>
> The limit still applies. In the sync pulses mode, you can still run the
> link at high frequency, it's just that the DSI data lanes won't go into
> LP mode between bursts of data, they would stuff the link with NOPs
> instead, as far as I can tell.
>
> > I raised these questions and others at [3], but largely got no real answers.
> >
> >
> > The patch does exactly what you describe and has value, but without
> > definition in the frameworks of exactly how burst mode must be
> > implemented by the DSI host, I would say that it's going to cause
> > issues.
> > I am aware of users of your driver with the Raspberry Pi, but your
> > expectation isn't currently valid on the Pi as there is no definition
> > of the correct thing for the host to do.
>
> This patch actually helped me deal with stability issues on MX8M . Of

I have a board with the SN65DSI83 and I have been testing Jagan's DSIM
patch series and I noticed the SN65DSI83  throws a PLL timeout error.
What kind of stability issues were you seeing?   I'll try to pull in
this patch, and I'm happy to test and reply with 'Tested-by' if it
works.

adam

> course, the DSIM driver has to handle the case where bridge provides
> hs_rate and configure its PLL accordingly. That's a two-liner patch.
Marek Vasut Aug. 1, 2022, 6 p.m. UTC | #4
On 8/1/22 18:38, Adam Ford wrote:
> On Mon, Aug 1, 2022 at 11:05 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 8/1/22 16:55, Dave Stevenson wrote:
>>> Hi Marek
>>
>> Hi,
>>
>>> On Mon, 1 Aug 2022 at 14:12, Marek Vasut <marex@denx.de> 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.
>>>
>>> I think this falls into another of the areas that is lacking in the DSI support.
>>> hs_rate is defined as the *maximum* lane frequency in high speed
>>> mode[1]. As documented there is no obligation on the DSI host to
>>> choose this specific rate, just some frequency below it and above or
>>> equal to that required by the pixel clock. At a system level, the link
>>> frequency is often prescribed for EMC purposes.
>>
>> The reduced upper limit could likely be defined by a DT property, but
>> that's not hard to add.
>>
>>> The issue is then that the SN65DSI83 is configured to use the DSI
>>> clock lane as the source for the PLL generating the LVDS clock,
>>> therefore with no route for the DSI peripheral to query the link
>>> frequency chosen by the host, you're stuck.
>>
>> At least making the host pick the highest supported frequency means we
>> have a well defined link frequency which either is accepted by both ends
>> or not at all, instead of the current guesswork on both ends, bridge and
>> host.
>>
>> Regarding reduction of the maximum hs_rate, see above, probably use a DT
>> property. Regarding check for hs_rate below minimum possible rate,
>> likely something the DSI host should do based on desired mode .
>>
>> If you are looking for some frequency negotiation starter, look at:
>> [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock
>> from DSI bridge
>>
>>> SN65DSI83 also supports non-burst mode (as currently), so how would
>>> this be configured now?
>>> Does MIPI_DSI_MODE_VIDEO_BURST [2] oblige the DSI host to run in burst
>>> mode, or say that burst mode is supported by the peripheral?
>>
>> My understanding is that it is the former -- if the flag is set, the DSI
>> host must operate the device in burst mode.
>>
>>> What if
>>> your DSI host doesn't support burst mode and it is optional on your
>>> peripheral?
>>
>> The limit still applies. In the sync pulses mode, you can still run the
>> link at high frequency, it's just that the DSI data lanes won't go into
>> LP mode between bursts of data, they would stuff the link with NOPs
>> instead, as far as I can tell.
>>
>>> I raised these questions and others at [3], but largely got no real answers.
>>>
>>>
>>> The patch does exactly what you describe and has value, but without
>>> definition in the frameworks of exactly how burst mode must be
>>> implemented by the DSI host, I would say that it's going to cause
>>> issues.
>>> I am aware of users of your driver with the Raspberry Pi, but your
>>> expectation isn't currently valid on the Pi as there is no definition
>>> of the correct thing for the host to do.
>>
>> This patch actually helped me deal with stability issues on MX8M . Of
> 
> I have a board with the SN65DSI83 and I have been testing Jagan's DSIM
> patch series and I noticed the SN65DSI83  throws a PLL timeout error.
> What kind of stability issues were you seeing?   I'll try to pull in
> this patch, and I'm happy to test and reply with 'Tested-by' if it
> works.

You need to make sure the DSIM generates these 500 MHz HS clock (PLL at 
1 GHz, P/M/S 3/125/0 I think). I still use the older DSIM driver with 
this kind of patch:

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index ddadce208d38b..11f6aaf741466 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1593,6 +1593,19 @@ static int samsung_dsim_host_attach(struct 
mipi_dsi_host *host,
         dsi->format = device->format;
         dsi->mode_flags = device->mode_flags;

+       /*
+        * In case the nearest bridge specifies DSI HS clock rate and 
supports
+        * DSI burst mode, run the DSI link at highest supported DSI HS 
clock
+        * frequency to achieve the shortest transfer bursts, longest 
time in
+        * LP mode between bursts, and thus most power efficient transfer.
+        *
+        * Note that the DSIM PLL runs at 2x HS clock rate.
+        */
+       if (device->hs_rate && (device->mode_flags & 
MIPI_DSI_MODE_VIDEO_BURST))
+               dsi->burst_clk_rate = 2 * device->hs_rate;
+       if (device->lp_rate)
+               dsi->esc_clk_rate = device->lp_rate;
+
Marek Vasut Sept. 18, 2022, 12:56 p.m. UTC | #5
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@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: dri-devel@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;
>   
>   	ret = devm_mipi_dsi_attach(dev, dsi);
>   	if (ret < 0) {

+CC Maxime -- input would be helpful.
Maxime Ripard Sept. 19, 2022, 1:43 p.m. UTC | #6
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@denx.de>
> > Cc: Jagan Teki <jagan@amarulasolutions.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: dri-devel@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
Marek Vasut Sept. 19, 2022, 6:17 p.m. UTC | #7
On 9/19/22 15:43, Maxime Ripard wrote:
> Hi,

Hello Maxime,

> 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@denx.de>
>>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Robert Foss <robert.foss@linaro.org>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: dri-devel@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.

The use of exact frequency on DSI HS clock for bridges which derive 
their PLL clock frequency from DSI HS clock discussion is separate a 
topic from this patch.

This patch is defining the maximum DSI HS clock frequency this bridge 
supports per datasheet (500 MHz) AND then assumes the DSI host would use 
that frequency on the DSI link (in burst mode, the highest frequency on 
the link permits the link to be in LP mode for longest time and thus be 
as power efficient as possible).

About the assumption -- currently the DSI HS clock frequency expected by 
the DSI bridge driver is calculated by its own driver-private formula ; 
the DSI HS clock frequency generated by the DSI host is calculated by 
the DSI host driver-private formula. Those two might not even match and 
thus each driver might come to different DSI HS clock they expect or 
generate. This cannot work reliably.

With this patch, it is at least possible for the DSI bridge driver to 
indicate what DSI HS clock it expects from the DSI host driver and what 
are its maximum DSI HS clock. The DSI host driver can check the nearest 
bridge flags (to verify the bridge uses burst mode) and hs_rate and 
configure itself accordingly. If the DSI host cannot generate suitable 
DSI HS clock which the bridge expects, it should likely refuse to bind 
with the bridge (also read on below how to deal with this). I consider 
this an improvement over current situation which is a total guesswork.

Regarding DSI hosts which cannot generate the specified hs_rate , they 
should likely just reject binding with the DSI bridge. However, there 
has been a valid point raised that the highest DSI HS clock supported by 
the DSI bridge might not always be the desired clock, e.g. in case EMI 
reduction is required by lowering the clock. I would propose to add a DT 
property which would allow limiting the maximum DSI HS clock frequency 
per DSI OF graph link in the DT, and a helper which would check the DT 
property, compare it with bridge limits, and set hs_rate accordingly.

This should work well for DSI burst mode links in general.

> 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.

The bridge hardware constraints are static, just like the bridge 
hardware itself, so they shouldn't be part of the bridge state, should 
they ?

I agree we do need a way to implement frequency negotiation between DSI 
bridge and DSI host, but that's something which can be added on top of 
the actual bridge HW constraints, and yes, possibly by passing those 
dynamic constraints via bridge state.
Maxime Ripard Sept. 21, 2022, 9:40 a.m. UTC | #8
On Mon, Sep 19, 2022 at 08:17:11PM +0200, Marek Vasut wrote:
> On 9/19/22 15:43, Maxime Ripard wrote:
> > Hi,
> 
> Hello Maxime,
> 
> > 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@denx.de>
> > > > Cc: Jagan Teki <jagan@amarulasolutions.com>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > Cc: Robert Foss <robert.foss@linaro.org>
> > > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > > Cc: dri-devel@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.
> 
> The use of exact frequency on DSI HS clock for bridges which derive their
> PLL clock frequency from DSI HS clock discussion is separate a topic from
> this patch.

Sure, it just happens that the solution to that other patch would also be
a solution for this one.

> This patch is defining the maximum DSI HS clock frequency this bridge
> supports per datasheet (500 MHz) AND then assumes the DSI host would use
> that frequency on the DSI link (in burst mode, the highest frequency on the
> link permits the link to be in LP mode for longest time and thus be as power
> efficient as possible).
> 
> About the assumption -- currently the DSI HS clock frequency expected by the
> DSI bridge driver is calculated by its own driver-private formula ; the DSI
> HS clock frequency generated by the DSI host is calculated by the DSI host
> driver-private formula. Those two might not even match and thus each driver
> might come to different DSI HS clock they expect or generate. This cannot
> work reliably.

I agree so far

> With this patch, it is at least possible for the DSI bridge driver to
> indicate what DSI HS clock it expects from the DSI host driver and what are
> its maximum DSI HS clock. The DSI host driver can check the nearest bridge
> flags (to verify the bridge uses burst mode) and hs_rate and configure
> itself accordingly. If the DSI host cannot generate suitable DSI HS clock
> which the bridge expects, it should likely refuse to bind with the bridge
> (also read on below how to deal with this). I consider this an improvement
> over current situation which is a total guesswork.

But I disagree here. In the above paragraphs you mentioned that you are
making an assumption, which is then unreliable. Again, I agree with that
statement, but you then cannot advocate that it's a good solution. It's
a workaround at best, which kind of changes the semantics around hs_rate.

> Regarding DSI hosts which cannot generate the specified hs_rate , they
> should likely just reject binding with the DSI bridge.

That won't work in all situations. The constraints on the clock feeding
the DSI controller might have changed for example, and doesn't allow to
reach that frequency anymore.

And if the bandwidth is still enough to cover the data bandwidth, why
should we completely break the video pipeline, without any way to
recover?

> However, there has been a valid point raised that the highest DSI HS
> clock supported by the DSI bridge might not always be the desired
> clock, e.g. in case EMI reduction is required by lowering the clock. I
> would propose to add a DT property which would allow limiting the
> maximum DSI HS clock frequency per DSI OF graph link in the DT, and a
> helper which would check the DT property, compare it with bridge
> limits, and set hs_rate accordingly.
> 
> This should work well for DSI burst mode links in general.

hs_rate is documented as "maximum lane frequency for high speed mode in
hertz". It's a maximum. You can't ask to every driver from now on to
treat it as the frequency that they have to apply. Or rather, you can do
that but it will require much more work than a single patch on a bridge.

And EMI reduction is fine, but that still doesn't address the fact that
the host might still not be able to provide that *maximum* frequency,
even after the EMI reduction. And, again, if there's still enough
bandwidth to send that video, there's no problem with that, we shouldn't
make it one.

> > 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.
> 
> The bridge hardware constraints are static, just like the bridge hardware
> itself, so they shouldn't be part of the bridge state, should they ?

Constraints are, sure. But the effective clock rate isn't a constraint,
it's a dynamic thing that will be affected by the mode, format, lanes,
etc. Most of them being part of the state.

> I agree we do need a way to implement frequency negotiation between DSI
> bridge and DSI host, but that's something which can be added on top of the
> actual bridge HW constraints, and yes, possibly by passing those dynamic
> constraints via bridge state.

I guess this is where we disagree. This is the first step to me.

Maxime
diff mbox series

Patch

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;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {