diff mbox series

[3/8] drm/i915/cdclk: Remove the assumption that cd2x div==2 when using squashing

Message ID 20231128115138.13238-4-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: cdclk/voltage_level cleanups and fixes | expand

Commit Message

Ville Syrjälä Nov. 28, 2023, 11:51 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we have a hardcoded assumption that the cd2x divider
is always 2 when squahsing is used. While that is true for all
current platforms it might not hold in the future. So eliminate
the assumption and calculate the correct divider from the other
parameters.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Gustavo Sousa Dec. 1, 2023, 8:13 p.m. UTC | #1 
Quoting Ville Syrjala (2023-11-28 08:51:33-03:00)
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Currently we have a hardcoded assumption that the cd2x divider
>is always 2 when squahsing is used.

It seems the code was actually making the assumption that the
divider is always 1. With the current code, when squashing is used, we
have that bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) is equivalent to
bxt_cdclk_cd2x_div_sel(dev_priv, vco / 2, vco), meaning that the
returned value will always be BXT_CDCLK_CD2X_DIV_SEL_1.

> While that is true for all
>current platforms it might not hold in the future. So eliminate

Not sure about the other platforms, but for MTL, I have checked the
BSpec table and also did some calculations by hand, and it seems like
the cd2x divider is actually always 1.

Unless I'm missing something in my analysis above, I believe s/2/1/ in
the commit message is necessary. With that,

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>the assumption and calculate the correct divider from the other
>parameters.
>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>index 87d5e5b67c4e..d45071675629 100644
>--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>@@ -1899,10 +1899,8 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> 
>         waveform = cdclk_squash_waveform(dev_priv, cdclk);
> 
>-        if (waveform)
>-                clock = vco / 2;
>-        else
>-                clock = cdclk;
>+        clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len,
>+                                  cdclk_squash_divider(waveform));

Nit: maybe take the opportunity to rename "clock" to "unsquashed" for
clarity?

> 
>         if (HAS_CDCLK_SQUASH(dev_priv))
>                 dg2_cdclk_squash_program(dev_priv, waveform);
>-- 
>2.41.0
>
Ville Syrjälä Dec. 4, 2023, 8:05 p.m. UTC | #2 
On Fri, Dec 01, 2023 at 05:13:38PM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjala (2023-11-28 08:51:33-03:00)
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Currently we have a hardcoded assumption that the cd2x divider
> >is always 2 when squahsing is used.
> 
> It seems the code was actually making the assumption that the
> divider is always 1. With the current code, when squashing is used, we
> have that bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) is equivalent to
> bxt_cdclk_cd2x_div_sel(dev_priv, vco / 2, vco), meaning that the
> returned value will always be BXT_CDCLK_CD2X_DIV_SEL_1.

The real cd2x divider is half of our 'divider' because we
specify the full vco->cdclk divider instead of the vco->cd2xclk
divider. 

Alternatively you can think of it as being the cd2x divider
specified in .1 binary fixed point format.

But yeah, saying "cd2x divider is 2" probably isn't the best
way to put this.

> 
> > While that is true for all
> >current platforms it might not hold in the future. So eliminate
> 
> Not sure about the other platforms, but for MTL, I have checked the
> BSpec table and also did some calculations by hand, and it seems like
> the cd2x divider is actually always 1.
> 
> Unless I'm missing something in my analysis above, I believe s/2/1/ in
> the commit message is necessary. With that,
> 
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
> 
> >the assumption and calculate the correct divider from the other
> >parameters.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >index 87d5e5b67c4e..d45071675629 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >@@ -1899,10 +1899,8 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > 
> >         waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > 
> >-        if (waveform)
> >-                clock = vco / 2;
> >-        else
> >-                clock = cdclk;
> >+        clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len,
> >+                                  cdclk_squash_divider(waveform));
> 
> Nit: maybe take the opportunity to rename "clock" to "unsquashed" for
> clarity?
> 
> > 
> >         if (HAS_CDCLK_SQUASH(dev_priv))
> >                 dg2_cdclk_squash_program(dev_priv, waveform);
> >-- 
> >2.41.0
> >
Gustavo Sousa Dec. 4, 2023, 8:46 p.m. UTC | #3 
Quoting Ville Syrjälä (2023-12-04 17:05:46-03:00)
>On Fri, Dec 01, 2023 at 05:13:38PM -0300, Gustavo Sousa wrote:
>> Quoting Ville Syrjala (2023-11-28 08:51:33-03:00)
>> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> >Currently we have a hardcoded assumption that the cd2x divider
>> >is always 2 when squahsing is used.
>> 
>> It seems the code was actually making the assumption that the
>> divider is always 1. With the current code, when squashing is used, we
>> have that bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) is equivalent to
>> bxt_cdclk_cd2x_div_sel(dev_priv, vco / 2, vco), meaning that the
>> returned value will always be BXT_CDCLK_CD2X_DIV_SEL_1.
>
>The real cd2x divider is half of our 'divider' because we
>specify the full vco->cdclk divider instead of the vco->cd2xclk
>divider. 

Got it.

>
>Alternatively you can think of it as being the cd2x divider
>specified in .1 binary fixed point format.
>
>But yeah, saying "cd2x divider is 2" probably isn't the best
>way to put this.

Yeah, maybe because of my little time with the driver code, but I had
interpreted it as the one used right after the PLL output (I'm taking
the diagram from MTL specs as reference).

Did I miss some comment in the code explaning that? Should one be added?

>
>> 
>> > While that is true for all
>> >current platforms it might not hold in the future. So eliminate
>> 
>> Not sure about the other platforms, but for MTL, I have checked the
>> BSpec table and also did some calculations by hand, and it seems like
>> the cd2x divider is actually always 1.
>> 
>> Unless I'm missing something in my analysis above, I believe s/2/1/ in
>> the commit message is necessary. With that,
>> 
>> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

Considering that the above understanding about the divider is the common
sense, the r-b stands without the tweaks in the commit messages. Thanks
for the clarification!

--
Gustavo Sousa

>> 
>> >the assumption and calculate the correct divider from the other
>> >parameters.
>> >
>> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++----
>> > 1 file changed, 2 insertions(+), 4 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >index 87d5e5b67c4e..d45071675629 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >@@ -1899,10 +1899,8 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> > 
>> >         waveform = cdclk_squash_waveform(dev_priv, cdclk);
>> > 
>> >-        if (waveform)
>> >-                clock = vco / 2;
>> >-        else
>> >-                clock = cdclk;
>> >+        clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len,
>> >+                                  cdclk_squash_divider(waveform));
>> 
>> Nit: maybe take the opportunity to rename "clock" to "unsquashed" for
>> clarity?
>> 
>> > 
>> >         if (HAS_CDCLK_SQUASH(dev_priv))
>> >                 dg2_cdclk_squash_program(dev_priv, waveform);
>> >-- 
>> >2.41.0
>> >
>
>-- 
>Ville Syrjälä
>Intel
Ville Syrjälä Dec. 5, 2023, 2:50 p.m. UTC | #4 
On Mon, Dec 04, 2023 at 05:46:28PM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjälä (2023-12-04 17:05:46-03:00)
> >On Fri, Dec 01, 2023 at 05:13:38PM -0300, Gustavo Sousa wrote:
> >> Quoting Ville Syrjala (2023-11-28 08:51:33-03:00)
> >> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> >Currently we have a hardcoded assumption that the cd2x divider
> >> >is always 2 when squahsing is used.
> >> 
> >> It seems the code was actually making the assumption that the
> >> divider is always 1. With the current code, when squashing is used, we
> >> have that bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) is equivalent to
> >> bxt_cdclk_cd2x_div_sel(dev_priv, vco / 2, vco), meaning that the
> >> returned value will always be BXT_CDCLK_CD2X_DIV_SEL_1.
> >
> >The real cd2x divider is half of our 'divider' because we
> >specify the full vco->cdclk divider instead of the vco->cd2xclk
> >divider. 
> 
> Got it.
> 
> >
> >Alternatively you can think of it as being the cd2x divider
> >specified in .1 binary fixed point format.
> >
> >But yeah, saying "cd2x divider is 2" probably isn't the best
> >way to put this.
> 
> Yeah, maybe because of my little time with the driver code, but I had
> interpreted it as the one used right after the PLL output (I'm taking
> the diagram from MTL specs as reference).
> 
> Did I miss some comment in the code explaning that? Should one be added?

Looks like we've at least attempted to document this:

struct intel_cdclk_vals {
	u32 cdclk;
	u16 refclk;
	u16 waveform;
	u8 divider;     /* CD2X divider * 2 */
	u8 ratio;
};

> 
> >
> >> 
> >> > While that is true for all
> >> >current platforms it might not hold in the future. So eliminate
> >> 
> >> Not sure about the other platforms, but for MTL, I have checked the
> >> BSpec table and also did some calculations by hand, and it seems like
> >> the cd2x divider is actually always 1.
> >> 
> >> Unless I'm missing something in my analysis above, I believe s/2/1/ in
> >> the commit message is necessary. With that,
> >> 
> >> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
> 
> Considering that the above understanding about the divider is the common
> sense, the r-b stands without the tweaks in the commit messages. Thanks
> for the clarification!

I can try to tweak it a bit anyway for extra clarity.

And thanks for the review.

> 
> --
> Gustavo Sousa
> 
> >> 
> >> >the assumption and calculate the correct divider from the other
> >> >parameters.
> >> >
> >> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++----
> >> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> >index 87d5e5b67c4e..d45071675629 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> >@@ -1899,10 +1899,8 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >> > 
> >> >         waveform = cdclk_squash_waveform(dev_priv, cdclk);
> >> > 
> >> >-        if (waveform)
> >> >-                clock = vco / 2;
> >> >-        else
> >> >-                clock = cdclk;
> >> >+        clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len,
> >> >+                                  cdclk_squash_divider(waveform));
> >> 
> >> Nit: maybe take the opportunity to rename "clock" to "unsquashed" for
> >> clarity?
> >> 
> >> > 
> >> >         if (HAS_CDCLK_SQUASH(dev_priv))
> >> >                 dg2_cdclk_squash_program(dev_priv, waveform);
> >> >-- 
> >> >2.41.0
> >> >
> >
> >-- 
> >Ville Syrjälä
> >Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 87d5e5b67c4e..d45071675629 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1899,10 +1899,8 @@  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
 
 	waveform = cdclk_squash_waveform(dev_priv, cdclk);
 
-	if (waveform)
-		clock = vco / 2;
-	else
-		clock = cdclk;
+	clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len,
+				  cdclk_squash_divider(waveform));
 
 	if (HAS_CDCLK_SQUASH(dev_priv))
 		dg2_cdclk_squash_program(dev_priv, waveform);