Due to integer calculations, the rounding off can cause errors in the final
value propagated in the registers.
Considering the example of 1080p (very common resolution), the mode->clock
is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
clock frequency would come as 444 when we are expecting the value 445.5
which would reflect in SN_DSIA_CLK_FREQ_REG.
So move the division to be the last operation where rounding off will not
impact the register value.
Also according to the SN65DSI86 datasheet[0], the minimum value for that
reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
check for that.
[0]: <https://www.ti.com/lit/gpn/sn65dsi86>
Fixes: ca1b885cbe9e ("drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates")
Signed-off-by: Jayesh Choudhary <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 48 +++++++++++++++++++++------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 84698a0b27a8..f9cf6b14d85e 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -111,7 +111,14 @@
#define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
#define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
-#define MIN_DSI_CLK_FREQ_MHZ 40
+/*
+ * NOTE: DSI clock frequency range: [40MHz,755MHz)
+ * DSI clock frequency range is in 5-MHz increments
+ * So minimum frequency 40MHz translates to 0x08
+ * And maximum frequency 755MHz translates to 0x97
+ */
+#define MIN_DSI_CLK_RANGE 0x8
+#define MAX_DSI_CLK_RANGE 0x97
/* fudge factor required to account for 8b/10b encoding */
#define DP_CLK_FUDGE_NUM 10
@@ -820,22 +827,37 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
}
-static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
+static int ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
{
- unsigned int bit_rate_mhz, clk_freq_mhz;
+ unsigned int bit_rate_khz;
unsigned int val;
struct drm_display_mode *mode =
&pdata->bridge.encoder->crtc->state->adjusted_mode;
- /* set DSIA clk frequency */
- bit_rate_mhz = (mode->clock / 1000) *
- mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
- clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+ /*
+ * Set DSIA clk frequency
+ * Maximum supported value of bit_rate_khz turns out to be
+ * 6040000 which can be put in 32-bit variable so no overflow
+ * possible in this calculation.
+ */
+ bit_rate_khz = mode->clock *
+ mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+
+ /*
+ * For each increment in val, frequency increases by 5MHz
+ * and the factor of 1000 comes from kHz to MHz conversion
+ */
+ val = (bit_rate_khz / (pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
+
+ if (val >= MAX_DSI_CLK_RANGE || val < MIN_DSI_CLK_RANGE) {
+ drm_err(pdata->bridge.dev,
+ "DSI clock frequency not in the supported range\n");
+ return -EINVAL;
+ }
- /* for each increment in val, frequency increases by 5MHz */
- val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
- (((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
+
+ return 0;
}
static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
@@ -1104,7 +1126,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
pdata->ln_polrs << LN_POLRS_OFFSET);
/* set dsi clk frequency value */
- ti_sn_bridge_set_dsi_rate(pdata);
+ ret = ti_sn_bridge_set_dsi_rate(pdata);
+ if (ret) {
+ DRM_DEV_ERROR(pdata->dev, "Failed to set dsi rate :%d\n", ret);
+ return;
+ }
/*
* The SN65DSI86 only supports ASSR Display Authentication method and
--
2.25.1
Hi,
On Mon, Apr 8, 2024 at 12:36 AM Jayesh Choudhary <[email protected]> wrote:
>
> Due to integer calculations, the rounding off can cause errors in the final
> value propagated in the registers.
> Considering the example of 1080p (very common resolution), the mode->clock
> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
> clock frequency would come as 444 when we are expecting the value 445.5
> which would reflect in SN_DSIA_CLK_FREQ_REG.
> So move the division to be the last operation where rounding off will not
> impact the register value.
Given that this driver is used on a whole pile of shipping Chromebooks
and those devices have been working just fine (with 1080p resolution)
for years, I'm curious how you noticed this. Was it actually causing
real problems for you, or did you notice it just from code inspection?
You should include this information in the commit message.
I'm travelling for the next two weeks so I can't actually check on a
device to see if your patch makes any difference on hardware I have,
but I'd presume that things were working "well enough" with the old
value and they'll still work with the new value?
> Also according to the SN65DSI86 datasheet[0], the minimum value for that
> reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
> check for that.
Maybe the range checking should be a separate patch?
> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
>
> Fixes: ca1b885cbe9e ("drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates")
Are you sure that's the commit you're fixing? In the commit text of
that commit I wrote that it was supposed to "have zero functional
change". Looking back at the change I still believe it had zero
functional change. The rounding error looks like it predates the
patch.
As far as I can tell the rounding error has been there since commit
a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver").
> Signed-off-by: Jayesh Choudhary <[email protected]>
It's great to see a TI engineer contributing to this driver! Awesome!
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 48 +++++++++++++++++++++------
> 1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 84698a0b27a8..f9cf6b14d85e 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -111,7 +111,14 @@
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
>
> -#define MIN_DSI_CLK_FREQ_MHZ 40
> +/*
> + * NOTE: DSI clock frequency range: [40MHz,755MHz)
> + * DSI clock frequency range is in 5-MHz increments
> + * So minimum frequency 40MHz translates to 0x08
> + * And maximum frequency 755MHz translates to 0x97
> + */
> +#define MIN_DSI_CLK_RANGE 0x8
> +#define MAX_DSI_CLK_RANGE 0x97
It's a little weird to call this min/max and have one be inclusive and
one be exclusive. Be consistent and say that this is the minimum legal
value and the maximum legal value. I think that means the MAX should
be 0x96.
> /* fudge factor required to account for 8b/10b encoding */
> #define DP_CLK_FUDGE_NUM 10
> @@ -820,22 +827,37 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
> }
>
> -static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> +static int ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> {
> - unsigned int bit_rate_mhz, clk_freq_mhz;
> + unsigned int bit_rate_khz;
> unsigned int val;
> struct drm_display_mode *mode =
> &pdata->bridge.encoder->crtc->state->adjusted_mode;
>
> - /* set DSIA clk frequency */
> - bit_rate_mhz = (mode->clock / 1000) *
> - mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> - clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
> + /*
> + * Set DSIA clk frequency
> + * Maximum supported value of bit_rate_khz turns out to be
> + * 6040000 which can be put in 32-bit variable so no overflow
> + * possible in this calculation.
The way you've written this comment makes me worried. You're saying
that the only supported result of the math has to fit in 32-bits so
we're OK. ...and then _after_ you do the math you check to see if the
clock rate is within the supported range. It makes me feel like you
could still overflow.
I think your comment here should say something like:
The maximum value returned by mipi_dsi_pixel_format_to_bpp() is 24.
That means that as long as "mode->clock" is less than 178,956,971 kHz
then the calculation can't overflow and can fit in 32-bits.
If you wanted to be really good you could even put a check earlier in
the function to make sure that mode->clock wasn't something totally
crazy (could confirm it's < 100GHz maybe?) so you absolutely knew it
couldn't overflow.
> + */
> + bit_rate_khz = mode->clock *
> + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> +
> + /*
> + * For each increment in val, frequency increases by 5MHz
> + * and the factor of 1000 comes from kHz to MHz conversion
> + */
> + val = (bit_rate_khz / (pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
> +
> + if (val >= MAX_DSI_CLK_RANGE || val < MIN_DSI_CLK_RANGE) {
> + drm_err(pdata->bridge.dev,
> + "DSI clock frequency not in the supported range\n");
> + return -EINVAL;
> + }
Shouldn't the above be in atomic_check()? There's a reason why
atomic_enable() can't return error codes.
-Doug
Hello Doug,
Thanks for the review.
On 08/04/24 14:33, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 8, 2024 at 12:36 AM Jayesh Choudhary <[email protected]> wrote:
>>
>> Due to integer calculations, the rounding off can cause errors in the final
>> value propagated in the registers.
>> Considering the example of 1080p (very common resolution), the mode->clock
>> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
>> clock frequency would come as 444 when we are expecting the value 445.5
>> which would reflect in SN_DSIA_CLK_FREQ_REG.
>> So move the division to be the last operation where rounding off will not
>> impact the register value.
>
> Given that this driver is used on a whole pile of shipping Chromebooks
> and those devices have been working just fine (with 1080p resolution)
> for years, I'm curious how you noticed this. Was it actually causing
> real problems for you, or did you notice it just from code inspection?
> You should include this information in the commit message.
I am trying to add display support for TI SoC which uses this particular
bridge. While debugging, I was trying to get all the register value in
sync with the datasheet and it was then that I observed this issue while
inspecting the code.
Maybe Chromebooks are using different set of parameters which does not
expose this issue. Since parameters for my display (mentioned in commit
message) yields the frequency at the border, I saw this issue. My debug
is still ongoing but since the value is not in sync with the
documentation, I sent out this patch.
>
> I'm travelling for the next two weeks so I can't actually check on a
> device to see if your patch makes any difference on hardware I have,
> but I'd presume that things were working "well enough" with the old
> value and they'll still work with the new value?
>
>
Yes, ideally they should still work well with this change.
>> Also according to the SN65DSI86 datasheet[0], the minimum value for that
>> reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
>> check for that.
>
> Maybe the range checking should be a separate patch?
Check should be done before propagating the register value so I added it
in the same function and hence in the same patch.
>
>
>> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
>>
>> Fixes: ca1b885cbe9e ("drm/bridge: ti-sn65dsi86: Split the setting of the dp and dsi rates")
>
> Are you sure that's the commit you're fixing? In the commit text of
> that commit I wrote that it was supposed to "have zero functional
> change". Looking back at the change I still believe it had zero
> functional change. The rounding error looks like it predates the
> patch.
>
> As far as I can tell the rounding error has been there since commit
> a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver").
>
Yes. Now I see that it fixes the initial support patch.
I will fix that.
>
>> Signed-off-by: Jayesh Choudhary <[email protected]>
>
> It's great to see a TI engineer contributing to this driver! Awesome!
>
>
>> ---
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 48 +++++++++++++++++++++------
>> 1 file changed, 37 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index 84698a0b27a8..f9cf6b14d85e 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -111,7 +111,14 @@
>> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
>> #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6)
>>
>> -#define MIN_DSI_CLK_FREQ_MHZ 40
>> +/*
>> + * NOTE: DSI clock frequency range: [40MHz,755MHz)
>> + * DSI clock frequency range is in 5-MHz increments
>> + * So minimum frequency 40MHz translates to 0x08
>> + * And maximum frequency 755MHz translates to 0x97
>> + */
>> +#define MIN_DSI_CLK_RANGE 0x8
>> +#define MAX_DSI_CLK_RANGE 0x97
>
> It's a little weird to call this min/max and have one be inclusive and
> one be exclusive. Be consistent and say that this is the minimum legal
> value and the maximum legal value. I think that means the MAX should
> be 0x96.
The comment above does specify the inclusive/exclusive behavior.
Since a value corresponds to 5MHz range, associating the value with
the range makes more sense if I keep it 0x97 (0x97 * 5 -> 755MHz)
0x96 corresponds to the range of [750Mz,755MHz).
If this argument does not make sense, I can change it to 0x96 and handle
it with the inequalities in the function call.
>
>
>> /* fudge factor required to account for 8b/10b encoding */
>> #define DP_CLK_FUDGE_NUM 10
>> @@ -820,22 +827,37 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
>> regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
>> }
>>
>> -static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>> +static int ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>> {
>> - unsigned int bit_rate_mhz, clk_freq_mhz;
>> + unsigned int bit_rate_khz;
>> unsigned int val;
>> struct drm_display_mode *mode =
>> &pdata->bridge.encoder->crtc->state->adjusted_mode;
>>
>> - /* set DSIA clk frequency */
>> - bit_rate_mhz = (mode->clock / 1000) *
>> - mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> - clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>> + /*
>> + * Set DSIA clk frequency
>> + * Maximum supported value of bit_rate_khz turns out to be
>> + * 6040000 which can be put in 32-bit variable so no overflow
>> + * possible in this calculation.
>
> The way you've written this comment makes me worried. You're saying
> that the only supported result of the math has to fit in 32-bits so
> we're OK. ...and then _after_ you do the math you check to see if the
> clock rate is within the supported range. It makes me feel like you
> could still overflow.
I did use reverse calculation for the value that I used in comments. xD
>
> I think your comment here should say something like:
>
> The maximum value returned by mipi_dsi_pixel_format_to_bpp() is 24.
> That means that as long as "mode->clock" is less than 178,956,971 kHz
> then the calculation can't overflow and can fit in 32-bits.
>
> If you wanted to be really good you could even put a check earlier in
> the function to make sure that mode->clock wasn't something totally
> crazy (could confirm it's < 100GHz maybe?) so you absolutely knew it
> couldn't overflow.
Okay. This makes sense. Will take this into account for v2.
>
>> + */
>> + bit_rate_khz = mode->clock *
>> + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> +
>> + /*
>> + * For each increment in val, frequency increases by 5MHz
>> + * and the factor of 1000 comes from kHz to MHz conversion
>> + */
>> + val = (bit_rate_khz / (pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
>> +
>> + if (val >= MAX_DSI_CLK_RANGE || val < MIN_DSI_CLK_RANGE) {
>> + drm_err(pdata->bridge.dev,
>> + "DSI clock frequency not in the supported range\n");
>> + return -EINVAL;
>> + }
>
> Shouldn't the above be in atomic_check()? There's a reason why
> atomic_enable() can't return error codes.
Oops.
I will handle it how we are handling errors in case of link_training:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/ti-sn65dsi86.c#L1152
That should be okay I guess?
Warm Regards,
Jayesh
>
> -Doug
Hi,
On Wed, Apr 10, 2024 at 4:42 AM Jayesh Choudhary <[email protected]> wrote:
>
> Hello Doug,
>
> Thanks for the review.
>
> On 08/04/24 14:33, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 8, 2024 at 12:36 AM Jayesh Choudhary <[email protected]> wrote:
> >>
> >> Due to integer calculations, the rounding off can cause errors in the final
> >> value propagated in the registers.
> >> Considering the example of 1080p (very common resolution), the mode->clock
> >> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
> >> clock frequency would come as 444 when we are expecting the value 445.5
> >> which would reflect in SN_DSIA_CLK_FREQ_REG.
> >> So move the division to be the last operation where rounding off will not
> >> impact the register value.
> >
> > Given that this driver is used on a whole pile of shipping Chromebooks
> > and those devices have been working just fine (with 1080p resolution)
> > for years, I'm curious how you noticed this. Was it actually causing
> > real problems for you, or did you notice it just from code inspection?
> > You should include this information in the commit message.
>
> I am trying to add display support for TI SoC which uses this particular
> bridge. While debugging, I was trying to get all the register value in
> sync with the datasheet and it was then that I observed this issue while
> inspecting the code.
> Maybe Chromebooks are using different set of parameters which does not
> expose this issue. Since parameters for my display (mentioned in commit
> message) yields the frequency at the border, I saw this issue. My debug
> is still ongoing but since the value is not in sync with the
> documentation, I sent out this patch.
OK, sounds good. It would be good to include some of this type of into
in the patch description for the next version.
> > I'm travelling for the next two weeks so I can't actually check on a
> > device to see if your patch makes any difference on hardware I have,
> > but I'd presume that things were working "well enough" with the old
> > value and they'll still work with the new value?
> >
> >
>
> Yes, ideally they should still work well with this change.
OK, I can validate it in a few weeks.
> >> Also according to the SN65DSI86 datasheet[0], the minimum value for that
> >> reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
> >> check for that.
> >
> > Maybe the range checking should be a separate patch?
>
> Check should be done before propagating the register value so I added it
> in the same function and hence in the same patch.
I was thinking you could have patch #1 add the checks. ...then patch
#2 could fix the math.
> >> -#define MIN_DSI_CLK_FREQ_MHZ 40
> >> +/*
> >> + * NOTE: DSI clock frequency range: [40MHz,755MHz)
> >> + * DSI clock frequency range is in 5-MHz increments
> >> + * So minimum frequency 40MHz translates to 0x08
> >> + * And maximum frequency 755MHz translates to 0x97
> >> + */
> >> +#define MIN_DSI_CLK_RANGE 0x8
> >> +#define MAX_DSI_CLK_RANGE 0x97
> >
> > It's a little weird to call this min/max and have one be inclusive and
> > one be exclusive. Be consistent and say that this is the minimum legal
> > value and the maximum legal value. I think that means the MAX should
> > be 0x96.
>
> The comment above does specify the inclusive/exclusive behavior.
> Since a value corresponds to 5MHz range, associating the value with
> the range makes more sense if I keep it 0x97 (0x97 * 5 -> 755MHz)
> 0x96 corresponds to the range of [750Mz,755MHz).
>
> If this argument does not make sense, I can change it to 0x96 and handle
> it with the inequalities in the function call.
Right that the comment is correct so that's good, but I'd still like
to see the constants changing. For instance, if I had code like this:
/*
* I know 2 * 2 is not really 5, but it makes my math work out
* so we'll just define it that way.
*/
#define TWO_TIMES_TWO 5
..and then later you had code:
if (x * y >= TWO_TIMES_TWO)
When you read the code you probably wouldn't go back and read the
comment so you'd be confused. AKA the above would be better as:
#define TWO_TIMES_TWO 4
if (x * y > TWO_TIMES_TWO)
Better to make the name of the #define make sense on its own. In this
case "min" and "max" should be the minimum legal value and the maximum
legal value, not "one past".
> >> + */
> >> + bit_rate_khz = mode->clock *
> >> + mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> >> +
> >> + /*
> >> + * For each increment in val, frequency increases by 5MHz
> >> + * and the factor of 1000 comes from kHz to MHz conversion
> >> + */
> >> + val = (bit_rate_khz / (pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
> >> +
> >> + if (val >= MAX_DSI_CLK_RANGE || val < MIN_DSI_CLK_RANGE) {
> >> + drm_err(pdata->bridge.dev,
> >> + "DSI clock frequency not in the supported range\n");
> >> + return -EINVAL;
> >> + }
> >
> > Shouldn't the above be in atomic_check()? There's a reason why
> > atomic_enable() can't return error codes.
>
> Oops.
> I will handle it how we are handling errors in case of link_training:
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/ti-sn65dsi86.c#L1152
>
> That should be okay I guess?
I'm pretty sure it should be in atomic_check(). The atomic_check() is
supposed to confirm that all parameters are within valid ranges and
the enable function shouldn't fail because the caller passed bad
parameters. Specifically this could allow the caller to try different
parameters and see if those would work instead.
In the case of the link training failure it's not something we could
have detected until we actually tried to enable, so there's no choice.
-Doug