Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
> -Original Message-
> From: Ville Syrjälä
> Sent: Wednesday, November 9, 2022 3:30 AM
> To: Roper, Matthew D
> Cc: Srivatsa, Anusha ; intel-
> g...@lists.freedesktop.org; Vivekanandan, Balasubramani
>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
>
> On Tue, Nov 08, 2022 at 04:24:30PM -0800, Matt Roper wrote:
> > On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Roper, Matthew D
> > > > Sent: Tuesday, November 8, 2022 3:43 PM
> > > > To: Srivatsa, Anusha
> > > > Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> > > >
> > > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both
> > > > crawl and squash when changing cdclk
> > > >
> > > > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > > > From: Ville Syrjälä
> > > > >
> > > > > For MTL, changing cdclk from between certain frequencies has
> > > > > both squash and crawl. Use the current cdclk config and the
> > > > > new(desired) cdclk config to construtc a mid cdclk config.
> > > > > Set the cdclk twice:
> > > > > - Current cdclk -> mid cdclk
> > > > > - mid cdclk -> desired cdclk
> > > > >
> > > > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> > > > > change via modeset for platforms that support squash_crawl
> > > > > sequences(Ville)
> > > > >
> > > > > v3: Add checks for:
> > > > > - scenario where only slow clock is used and cdclk is actually 0
> > > > > (bringing up display).
> > > > > - PLLs are on before looking up the waveform.
> > > > > - Squash and crawl capability checks.(Ville)
> > > > >
> > > > > v4: Rebase
> > > > > - Move checks to be more consistent (Ville)
> > > > > - Add comments (Bala)
> > > > > v5:
> > > > > - Further small changes. Move checks around.
> > > > > - Make if-else better looking (Ville)
> > > > >
> > > > > v6: MTl should not follow PUnit mailbox communication as the
> > > > > rest of
> > > > > gen11+ platforms.(Anusha)
> > > > >
> > > > > Cc: Clint Taylor
> > > > > Cc: Balasubramani Vivekanandan
> > > >
> > > > > Signed-off-by: Anusha Srivatsa
> > > > > Signed-off-by: Ville Syrjälä
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > > > +
> > > > > 1 file changed, 133 insertions(+), 28 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > index eada931cb1c8..d1e0763513be 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > @@ -1716,37 +1716,74 @@ static void
> > > > > dg2_cdclk_squash_program(struct
> > > > drm_i915_private *i915,
> > > > > intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); }
> > > > >
> > > > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > > - const struct intel_cdclk_config *cdclk_config,
> > > > > - enum pipe pipe)
> > > > > +static int cdclk_squash_divider(u16 waveform) {
> > > > > + return hweight16(waveform ?: 0x); }
> > > > > +
> > > > > +static bool cdclk_crawl_and_squash(struct drm_i915_private
> > > > > +*i915,
> > > >
> > > > Bikeshed: maybe name this "cdclk_compute_crawl_squash_midpoint"
> > > > to help clarify that we're just computing stuff here and not
> > > > actually programming the hardware in this function?
> > > >
> > > > That naming would also help clarify why we're returning false if
> > > > we crawl but don't squash or vice versa (i.e., there's no midpoint in
> those cases).
> > >
> > > Makes sense.
> > >
> > > > > +const struct intel_cdclk_config
>
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On Tue, Nov 08, 2022 at 04:24:30PM -0800, Matt Roper wrote:
> On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
> >
> >
> > > -Original Message-
> > > From: Roper, Matthew D
> > > Sent: Tuesday, November 8, 2022 3:43 PM
> > > To: Srivatsa, Anusha
> > > Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> > >
> > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> > > squash when changing cdclk
> > >
> > > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > > From: Ville Syrjälä
> > > >
> > > > For MTL, changing cdclk from between certain frequencies has both
> > > > squash and crawl. Use the current cdclk config and the new(desired)
> > > > cdclk config to construtc a mid cdclk config.
> > > > Set the cdclk twice:
> > > > - Current cdclk -> mid cdclk
> > > > - mid cdclk -> desired cdclk
> > > >
> > > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > > > modeset for platforms that support squash_crawl sequences(Ville)
> > > >
> > > > v3: Add checks for:
> > > > - scenario where only slow clock is used and cdclk is actually 0
> > > > (bringing up display).
> > > > - PLLs are on before looking up the waveform.
> > > > - Squash and crawl capability checks.(Ville)
> > > >
> > > > v4: Rebase
> > > > - Move checks to be more consistent (Ville)
> > > > - Add comments (Bala)
> > > > v5:
> > > > - Further small changes. Move checks around.
> > > > - Make if-else better looking (Ville)
> > > >
> > > > v6: MTl should not follow PUnit mailbox communication as the rest of
> > > > gen11+ platforms.(Anusha)
> > > >
> > > > Cc: Clint Taylor
> > > > Cc: Balasubramani Vivekanandan
> > >
> > > > Signed-off-by: Anusha Srivatsa
> > > > Signed-off-by: Ville Syrjälä
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > > +
> > > > 1 file changed, 133 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index eada931cb1c8..d1e0763513be 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> > > drm_i915_private *i915,
> > > > intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); }
> > > >
> > > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > - const struct intel_cdclk_config *cdclk_config,
> > > > - enum pipe pipe)
> > > > +static int cdclk_squash_divider(u16 waveform) {
> > > > + return hweight16(waveform ?: 0x); }
> > > > +
> > > > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > >
> > > Bikeshed: maybe name this "cdclk_compute_crawl_squash_midpoint" to
> > > help clarify that we're just computing stuff here and not actually
> > > programming the hardware in this function?
> > >
> > > That naming would also help clarify why we're returning false if we crawl
> > > but
> > > don't squash or vice versa (i.e., there's no midpoint in those cases).
> >
> > Makes sense.
> >
> > > > + const struct intel_cdclk_config
> > > *old_cdclk_config,
> > > > + const struct intel_cdclk_config
> > > *new_cdclk_config,
> > > > + struct intel_cdclk_config
> > > > *mid_cdclk_config)
> > > {
> > > > + u16 old_waveform, new_waveform, mid_waveform;
> > > > + int size = 16;
> > > > + int div = 2;
> > > > +
> > > > + /* Return if both Squash and Crawl are not present */
> > > > + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > > + return false;
> > > > +
> > > > + old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> > > >cdclk);
> &
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On Tue, Nov 08, 2022 at 03:56:23PM -0800, Srivatsa, Anusha wrote:
>
>
> > -Original Message-
> > From: Roper, Matthew D
> > Sent: Tuesday, November 8, 2022 3:43 PM
> > To: Srivatsa, Anusha
> > Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
> >
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> > squash when changing cdclk
> >
> > On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > > From: Ville Syrjälä
> > >
> > > For MTL, changing cdclk from between certain frequencies has both
> > > squash and crawl. Use the current cdclk config and the new(desired)
> > > cdclk config to construtc a mid cdclk config.
> > > Set the cdclk twice:
> > > - Current cdclk -> mid cdclk
> > > - mid cdclk -> desired cdclk
> > >
> > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > > modeset for platforms that support squash_crawl sequences(Ville)
> > >
> > > v3: Add checks for:
> > > - scenario where only slow clock is used and cdclk is actually 0
> > > (bringing up display).
> > > - PLLs are on before looking up the waveform.
> > > - Squash and crawl capability checks.(Ville)
> > >
> > > v4: Rebase
> > > - Move checks to be more consistent (Ville)
> > > - Add comments (Bala)
> > > v5:
> > > - Further small changes. Move checks around.
> > > - Make if-else better looking (Ville)
> > >
> > > v6: MTl should not follow PUnit mailbox communication as the rest of
> > > gen11+ platforms.(Anusha)
> > >
> > > Cc: Clint Taylor
> > > Cc: Balasubramani Vivekanandan
> >
> > > Signed-off-by: Anusha Srivatsa
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > > +
> > > 1 file changed, 133 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index eada931cb1c8..d1e0763513be 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> > drm_i915_private *i915,
> > > intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); }
> > >
> > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > - const struct intel_cdclk_config *cdclk_config,
> > > - enum pipe pipe)
> > > +static int cdclk_squash_divider(u16 waveform) {
> > > + return hweight16(waveform ?: 0x); }
> > > +
> > > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> >
> > Bikeshed: maybe name this "cdclk_compute_crawl_squash_midpoint" to
> > help clarify that we're just computing stuff here and not actually
> > programming the hardware in this function?
> >
> > That naming would also help clarify why we're returning false if we crawl
> > but
> > don't squash or vice versa (i.e., there's no midpoint in those cases).
>
> Makes sense.
>
> > > +const struct intel_cdclk_config
> > *old_cdclk_config,
> > > +const struct intel_cdclk_config
> > *new_cdclk_config,
> > > +struct intel_cdclk_config *mid_cdclk_config)
> > {
> > > + u16 old_waveform, new_waveform, mid_waveform;
> > > + int size = 16;
> > > + int div = 2;
> > > +
> > > + /* Return if both Squash and Crawl are not present */
> > > + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > > + return false;
> > > +
> > > + old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> > >cdclk);
> > > + new_waveform = cdclk_squash_waveform(i915, new_cdclk_config-
> > >cdclk);
> > > +
> > > + /* Return if Squash only or Crawl only is the desired action */
> > > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> >
> > Isn't vco unsigned? "== 0" should be fine here I think.
>
> You mean the new_cdclk_config->vco right?
Both of them I think. The vco field of intel_cdclk_config can't take on
negative values because it's defined as unsigned:
struct intel_cdclk_config {
uns
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
> -Original Message-
> From: Roper, Matthew D
> Sent: Tuesday, November 8, 2022 3:43 PM
> To: Srivatsa, Anusha
> Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
>
> On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> > From: Ville Syrjälä
> >
> > For MTL, changing cdclk from between certain frequencies has both
> > squash and crawl. Use the current cdclk config and the new(desired)
> > cdclk config to construtc a mid cdclk config.
> > Set the cdclk twice:
> > - Current cdclk -> mid cdclk
> > - mid cdclk -> desired cdclk
> >
> > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > modeset for platforms that support squash_crawl sequences(Ville)
> >
> > v3: Add checks for:
> > - scenario where only slow clock is used and cdclk is actually 0
> > (bringing up display).
> > - PLLs are on before looking up the waveform.
> > - Squash and crawl capability checks.(Ville)
> >
> > v4: Rebase
> > - Move checks to be more consistent (Ville)
> > - Add comments (Bala)
> > v5:
> > - Further small changes. Move checks around.
> > - Make if-else better looking (Ville)
> >
> > v6: MTl should not follow PUnit mailbox communication as the rest of
> > gen11+ platforms.(Anusha)
> >
> > Cc: Clint Taylor
> > Cc: Balasubramani Vivekanandan
>
> > Signed-off-by: Anusha Srivatsa
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 161
> > +
> > 1 file changed, 133 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index eada931cb1c8..d1e0763513be 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> drm_i915_private *i915,
> > intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); }
> >
> > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > - const struct intel_cdclk_config *cdclk_config,
> > - enum pipe pipe)
> > +static int cdclk_squash_divider(u16 waveform) {
> > + return hweight16(waveform ?: 0x); }
> > +
> > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
>
> Bikeshed: maybe name this "cdclk_compute_crawl_squash_midpoint" to
> help clarify that we're just computing stuff here and not actually
> programming the hardware in this function?
>
> That naming would also help clarify why we're returning false if we crawl but
> don't squash or vice versa (i.e., there's no midpoint in those cases).
Makes sense.
> > + const struct intel_cdclk_config
> *old_cdclk_config,
> > + const struct intel_cdclk_config
> *new_cdclk_config,
> > + struct intel_cdclk_config *mid_cdclk_config)
> {
> > + u16 old_waveform, new_waveform, mid_waveform;
> > + int size = 16;
> > + int div = 2;
> > +
> > + /* Return if both Squash and Crawl are not present */
> > + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > + return false;
> > +
> > + old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> >cdclk);
> > + new_waveform = cdclk_squash_waveform(i915, new_cdclk_config-
> >cdclk);
> > +
> > + /* Return if Squash only or Crawl only is the desired action */
> > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
>
> Isn't vco unsigned? "== 0" should be fine here I think.
You mean the new_cdclk_config->vco right?
> > + old_cdclk_config->vco == new_cdclk_config->vco ||
> > + old_waveform == new_waveform)
> > + return false;
> > +
> > + *mid_cdclk_config = *new_cdclk_config;
> > +
> > + /* Populate the mid_cdclk_config accordingly.
> > +* - If moving to a higher cdclk, the desired action is squashing.
> > +* The mid cdclk config should have the new (squash) waveform.
> > +* - If moving to a lower cdclk, the desired action is crawling.
> > +* The mid cdclk config should have the new vco.
> > +*/
> > +
> > + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> > +
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On Fri, Nov 04, 2022 at 03:26:41PM -0700, Anusha Srivatsa wrote:
> From: Ville Syrjälä
>
> For MTL, changing cdclk from between certain frequencies has
> both squash and crawl. Use the current cdclk config and
> the new(desired) cdclk config to construtc a mid cdclk config.
> Set the cdclk twice:
> - Current cdclk -> mid cdclk
> - mid cdclk -> desired cdclk
>
> v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> change via modeset for platforms that support squash_crawl sequences(Ville)
>
> v3: Add checks for:
> - scenario where only slow clock is used and
> cdclk is actually 0 (bringing up display).
> - PLLs are on before looking up the waveform.
> - Squash and crawl capability checks.(Ville)
>
> v4: Rebase
> - Move checks to be more consistent (Ville)
> - Add comments (Bala)
> v5:
> - Further small changes. Move checks around.
> - Make if-else better looking (Ville)
>
> v6: MTl should not follow PUnit mailbox communication as the rest of
> gen11+ platforms.(Anusha)
>
> Cc: Clint Taylor
> Cc: Balasubramani Vivekanandan
> Signed-off-by: Anusha Srivatsa
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 161 +
> 1 file changed, 133 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index eada931cb1c8..d1e0763513be 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> drm_i915_private *i915,
> intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);
> }
>
> -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> - const struct intel_cdclk_config *cdclk_config,
> - enum pipe pipe)
> +static int cdclk_squash_divider(u16 waveform)
> +{
> + return hweight16(waveform ?: 0x);
> +}
> +
> +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
Bikeshed: maybe name this "cdclk_compute_crawl_squash_midpoint" to help
clarify that we're just computing stuff here and not actually
programming the hardware in this function?
That naming would also help clarify why we're returning false if we
crawl but don't squash or vice versa (i.e., there's no midpoint in those
cases).
> +const struct intel_cdclk_config
> *old_cdclk_config,
> +const struct intel_cdclk_config
> *new_cdclk_config,
> +struct intel_cdclk_config *mid_cdclk_config)
> +{
> + u16 old_waveform, new_waveform, mid_waveform;
> + int size = 16;
> + int div = 2;
> +
> + /* Return if both Squash and Crawl are not present */
> + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> + return false;
> +
> + old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
> + new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
> +
> + /* Return if Squash only or Crawl only is the desired action */
> + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
Isn't vco unsigned? "== 0" should be fine here I think.
> + old_cdclk_config->vco == new_cdclk_config->vco ||
> + old_waveform == new_waveform)
> + return false;
> +
> + *mid_cdclk_config = *new_cdclk_config;
> +
> + /* Populate the mid_cdclk_config accordingly.
> + * - If moving to a higher cdclk, the desired action is squashing.
> + * The mid cdclk config should have the new (squash) waveform.
> + * - If moving to a lower cdclk, the desired action is crawling.
> + * The mid cdclk config should have the new vco.
> + */
> +
> + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> + mid_cdclk_config->vco = old_cdclk_config->vco;
> + mid_waveform = new_waveform;
> + } else {
> + mid_cdclk_config->vco = new_cdclk_config->vco;
> + mid_waveform = old_waveform;
> + }
> +
> + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> + mid_cdclk_config->vco, size
> * div);
> +
> + /* make sure the mid clock came out sane */
> +
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> + i915->display.cdclk.max_cdclk_freq);
> + drm_WARN_ON(>drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> + mid_waveform);
> +
> + return true;
> +}
> +
> +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> +const struct intel_cdclk_config *cdclk_config,
> +enum pipe pipe)
> {
> int cdclk = cdclk_config->cdclk;
> int vco =
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
> -Original Message-
> From: Ville Syrjälä
> Sent: Friday, October 28, 2022 2:05 AM
> To: Srivatsa, Anusha
> Cc: intel-gfx@lists.freedesktop.org; Vivekanandan, Balasubramani
>
> Subject: Re: [PATCH 1/2] drm/i915/display: Do both crawl and squash when
> changing cdclk
>
> On Wed, Oct 26, 2022 at 04:22:56PM -0700, Anusha Srivatsa wrote:
> > From: Ville Syrjälä
> >
> > For MTL, changing cdclk from between certain frequencies has both
> > squash and crawl. Use the current cdclk config and the new(desired)
> > cdclk config to construtc a mid cdclk config.
> > Set the cdclk twice:
> > - Current cdclk -> mid cdclk
> > - mid cdclk -> desired cdclk
> >
> > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > modeset for platforms that support squash_crawl sequences(Ville)
> >
> > v3: Add checks for:
> > - scenario where only slow clock is used and cdclk is actually 0
> > (bringing up display).
> > - PLLs are on before looking up the waveform.
> > - Squash and crawl capability checks.(Ville)
> >
> > v4: Rebase
> > - Move checks to be more consistent (Ville)
> > - Add comments (Bala)
> >
> > Cc: Balasubramani Vivekanandan
>
> > Signed-off-by: Anusha Srivatsa
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 157
> > +
> > 1 file changed, 129 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index eada931cb1c8..6a775367f02a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> drm_i915_private *i915,
> > intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); }
> >
> > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > - const struct intel_cdclk_config *cdclk_config,
> > - enum pipe pipe)
> > +static int cdclk_squash_divider(u16 waveform) {
> > + return hweight16(waveform ?: 0x); }
> > +
> > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > + const struct intel_cdclk_config
> *old_cdclk_config,
> > + const struct intel_cdclk_config
> *new_cdclk_config,
> > + struct intel_cdclk_config *mid_cdclk_config)
> {
> > + u16 old_waveform, new_waveform, mid_waveform;
> > + int size = 16;
> > + int div = 2;
> > +
> > + /* Return if both Squash and Crawl are not present */
> > + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> > + return false;
> > +
> > + /* Return if Squash only or Crawl only is the desired action */
> > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > + old_cdclk_config->vco == new_cdclk_config->vco ||
> > + old_waveform == new_waveform)
>
> Those are not yet initialized.
*facepalm*
> > + return false;
> > +
> > + old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> >cdclk);
> > + new_waveform = cdclk_squash_waveform(i915, new_cdclk_config-
> >cdclk);
> > +
> > + *mid_cdclk_config = *new_cdclk_config;
> > +
> > + /* Populate the mid_cdclk_config accordingly.
> > +* - If moving to a higher cdclk, the desired action is squashing.
> > +* The mid cdclk config should have the new (squash) waveform.
> > +* - If moving to a lower cdclk, the desired action is crawling.
> > +* The mid cdclk config should have the new vco.
> > +*/
> > +
> > + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> > + mid_cdclk_config->vco = old_cdclk_config->vco;
> > + mid_waveform = new_waveform;
> > + } else {
> > + mid_cdclk_config->vco = new_cdclk_config->vco;
> > + mid_waveform = old_waveform;
> > + }
> > +
> > + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> > + mid_cdclk_config->vco, size
> * div);
> > +
> > + /* make sure the mid clock came out sane */
> > +
> > + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> > + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> > + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> > + i915->display.cdclk.max_cdclk_freq);
> > + drm_WARN_ON(>drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> > + mid_waveform);
> > +
> > + return true;
> > +}
> > +
> > +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > + const struct intel_cdclk_config *cdclk_config,
> > + enum pipe pipe)
> > {
> > int cdclk = cdclk_config->cdclk;
> > int vco = cdclk_config->vco;
> > u32 val;
> > u16 waveform;
> > int clock;
> > - int ret;
> > -
> > - /* Inform power controller of upcoming frequency change. */
> > -
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On Wed, Oct 26, 2022 at 04:22:56PM -0700, Anusha Srivatsa wrote:
> From: Ville Syrjälä
>
> For MTL, changing cdclk from between certain frequencies has
> both squash and crawl. Use the current cdclk config and
> the new(desired) cdclk config to construtc a mid cdclk config.
> Set the cdclk twice:
> - Current cdclk -> mid cdclk
> - mid cdclk -> desired cdclk
>
> v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> change via modeset for platforms that support squash_crawl sequences(Ville)
>
> v3: Add checks for:
> - scenario where only slow clock is used and
> cdclk is actually 0 (bringing up display).
> - PLLs are on before looking up the waveform.
> - Squash and crawl capability checks.(Ville)
>
> v4: Rebase
> - Move checks to be more consistent (Ville)
> - Add comments (Bala)
>
> Cc: Balasubramani Vivekanandan
> Signed-off-by: Anusha Srivatsa
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 157 +
> 1 file changed, 129 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index eada931cb1c8..6a775367f02a 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1716,37 +1716,74 @@ static void dg2_cdclk_squash_program(struct
> drm_i915_private *i915,
> intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl);
> }
>
> -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> - const struct intel_cdclk_config *cdclk_config,
> - enum pipe pipe)
> +static int cdclk_squash_divider(u16 waveform)
> +{
> + return hweight16(waveform ?: 0x);
> +}
> +
> +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> +const struct intel_cdclk_config
> *old_cdclk_config,
> +const struct intel_cdclk_config
> *new_cdclk_config,
> +struct intel_cdclk_config *mid_cdclk_config)
> +{
> + u16 old_waveform, new_waveform, mid_waveform;
> + int size = 16;
> + int div = 2;
> +
> + /* Return if both Squash and Crawl are not present */
> + if (!HAS_CDCLK_CRAWL(i915) || !HAS_CDCLK_SQUASH(i915))
> + return false;
> +
> + /* Return if Squash only or Crawl only is the desired action */
> + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> + old_cdclk_config->vco == new_cdclk_config->vco ||
> + old_waveform == new_waveform)
Those are not yet initialized.
> + return false;
> +
> + old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
> + new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
> +
> + *mid_cdclk_config = *new_cdclk_config;
> +
> + /* Populate the mid_cdclk_config accordingly.
> + * - If moving to a higher cdclk, the desired action is squashing.
> + * The mid cdclk config should have the new (squash) waveform.
> + * - If moving to a lower cdclk, the desired action is crawling.
> + * The mid cdclk config should have the new vco.
> + */
> +
> + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> + mid_cdclk_config->vco = old_cdclk_config->vco;
> + mid_waveform = new_waveform;
> + } else {
> + mid_cdclk_config->vco = new_cdclk_config->vco;
> + mid_waveform = old_waveform;
> + }
> +
> + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> + mid_cdclk_config->vco, size
> * div);
> +
> + /* make sure the mid clock came out sane */
> +
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> + i915->display.cdclk.max_cdclk_freq);
> + drm_WARN_ON(>drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> + mid_waveform);
> +
> + return true;
> +}
> +
> +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> +const struct intel_cdclk_config *cdclk_config,
> +enum pipe pipe)
> {
> int cdclk = cdclk_config->cdclk;
> int vco = cdclk_config->vco;
> u32 val;
> u16 waveform;
> int clock;
> - int ret;
> -
> - /* Inform power controller of upcoming frequency change. */
> - if (DISPLAY_VER(dev_priv) >= 11)
> - ret = skl_pcode_request(_priv->uncore,
> SKL_PCODE_CDCLK_CONTROL,
> - SKL_CDCLK_PREPARE_FOR_CHANGE,
> - SKL_CDCLK_READY_FOR_CHANGE,
> - SKL_CDCLK_READY_FOR_CHANGE, 3);
> - else
> - /*
> - * BSpec requires us to
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
> -Original Message-
> From: Ville Syrjälä
> Sent: Thursday, October 20, 2022 8:15 AM
> To: Vivekanandan, Balasubramani
>
> Cc: Srivatsa, Anusha ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
>
> On Thu, Oct 20, 2022 at 08:12:04PM +0530, Balasubramani Vivekanandan
> wrote:
> > On 13.10.2022 16:32, Anusha Srivatsa wrote:
> > > From: Ville Syrjälä
> > >
> > > For MTL, changing cdclk from between certain frequencies has both
> > > squash and crawl. Use the current cdclk config and the new(desired)
> > > cdclk config to construtc a mid cdclk config.
> > > Set the cdclk twice:
> > > - Current cdclk -> mid cdclk
> > > - mid cdclk -> desired cdclk
> > >
> > > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change
> > > via modeset for platforms that support squash_crawl sequences(Ville)
> > >
> > > v3: Add checks for:
> > > - scenario where only slow clock is used and cdclk is actually 0
> > > (bringing up display).
> > > - PLLs are on before looking up the waveform.
> > > - Squash and crawl capability checks.(Ville)
> > >
> > > Signed-off-by: Anusha Srivatsa
> > > Signed-off-by: Ville Syrjälä
> >
> > Please add the Bspec number.
> >
> > > ---
> > > drivers/gpu/drm/i915/display/intel_cdclk.c | 157
> > > +
> > > 1 file changed, 128 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index ad401357ab66..430b4cb0a8ab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1675,7 +1675,7 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> > > const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> > > int i;
> > >
> > > - if (cdclk == dev_priv->display.cdclk.hw.bypass)
> > > + if (cdclk == dev_priv->display.cdclk.hw.bypass || cdclk == 0)
> > > return 0;
> > >
> > > for (i = 0; table[i].refclk; i++)
> > > @@ -1689,37 +1689,72 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> > > return 0x;
> > > }
> > >
> > > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > - const struct intel_cdclk_config *cdclk_config,
> > > - enum pipe pipe)
> > > +static int cdclk_squash_divider(u16 waveform) {
> > > + return hweight16(waveform ?: 0x); }
> > > +
> > > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > > +const struct intel_cdclk_config
> *old_cdclk_config,
> > > +const struct intel_cdclk_config
> *new_cdclk_config,
> > > +struct intel_cdclk_config *mid_cdclk_config)
> {
> >
> > I was thinking of asking to rename this function to a more descriptive
> > one, but then I myself was not able to come up with one.
> > For a fresh eyes, it is difficult to make out what this function is
> > actually doing. Can you please add a summary as a comment above this
> > function pointing out what is mid_cdclk and whats the meaning of its
> > return value.
> >
> > > + u16 old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> >cdclk);
> > > + u16 new_waveform = cdclk_squash_waveform(i915,
> new_cdclk_config->cdclk);
> > > + u16 mid_waveform;
> > > + int size = 16;
> > > + int div = 2;
> > > +
> > > + /* Return if both Squash and Crawl are not present */
> > > + if (!HAS_CDCLK_CRAWL(i915) || !has_cdclk_squasher(i915))
> > > + return false;
> >
> > Can cdclk_squasher feature availability be also made a part of
> > device_info structure like cdclk_crawl and create a macro similar to
> > HAS_CDCLK_CRAWL?
> > Like Ville said it looks bit weird. Also we would avoid adding
> > platform checks inside has_cdclk_squasher() function like it is done
> > now in your second patch.
> >
> > > +
> > > + /* Return if Squash only or Crawl only is the desired action */
> > > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > > + old_cdclk_config->vco == new_cdclk_config->
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
> -Original Message-
> From: Vivekanandan, Balasubramani
>
> Sent: Thursday, October 20, 2022 7:42 AM
> To: Srivatsa, Anusha ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and
> squash when changing cdclk
>
> On 13.10.2022 16:32, Anusha Srivatsa wrote:
> > From: Ville Syrjälä
> >
> > For MTL, changing cdclk from between certain frequencies has both
> > squash and crawl. Use the current cdclk config and the new(desired)
> > cdclk config to construtc a mid cdclk config.
> > Set the cdclk twice:
> > - Current cdclk -> mid cdclk
> > - mid cdclk -> desired cdclk
> >
> > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > modeset for platforms that support squash_crawl sequences(Ville)
> >
> > v3: Add checks for:
> > - scenario where only slow clock is used and cdclk is actually 0
> > (bringing up display).
> > - PLLs are on before looking up the waveform.
> > - Squash and crawl capability checks.(Ville)
> >
> > Signed-off-by: Anusha Srivatsa
> > Signed-off-by: Ville Syrjälä
>
> Please add the Bspec number.
Will do.
>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 157
> > +
> > 1 file changed, 128 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index ad401357ab66..430b4cb0a8ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1675,7 +1675,7 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> > const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> > int i;
> >
> > - if (cdclk == dev_priv->display.cdclk.hw.bypass)
> > + if (cdclk == dev_priv->display.cdclk.hw.bypass || cdclk == 0)
> > return 0;
> >
> > for (i = 0; table[i].refclk; i++)
> > @@ -1689,37 +1689,72 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> > return 0x;
> > }
> >
> > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > - const struct intel_cdclk_config *cdclk_config,
> > - enum pipe pipe)
> > +static int cdclk_squash_divider(u16 waveform) {
> > + return hweight16(waveform ?: 0x); }
> > +
> > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > + const struct intel_cdclk_config
> *old_cdclk_config,
> > + const struct intel_cdclk_config
> *new_cdclk_config,
> > + struct intel_cdclk_config *mid_cdclk_config)
> {
>
> I was thinking of asking to rename this function to a more descriptive one,
> but then I myself was not able to come up with one.
> For a fresh eyes, it is difficult to make out what this function is actually
> doing.
> Can you please add a summary as a comment above this function pointing
> out what is mid_cdclk and whats the meaning of its return value.
I thought the commit message was explaining the needful. But I agree if it is
indeed confusing, adding comments in code would help, will churn that change.
>
> > + u16 old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> >cdclk);
> > + u16 new_waveform = cdclk_squash_waveform(i915,
> new_cdclk_config->cdclk);
> > + u16 mid_waveform;
> > + int size = 16;
> > + int div = 2;
> > +
> > + /* Return if both Squash and Crawl are not present */
> > + if (!HAS_CDCLK_CRAWL(i915) || !has_cdclk_squasher(i915))
> > + return false;
>
> Can cdclk_squasher feature availability be also made a part of device_info
> structure like cdclk_crawl and create a macro similar to HAS_CDCLK_CRAWL?
> Like Ville said it looks bit weird. Also we would avoid adding platform checks
> inside has_cdclk_squasher() function like it is done now in your second
> patch.
Yes will make that change.
>
> > +
> > + /* Return if Squash only or Crawl only is the desired action */
> > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > + old_cdclk_config->vco == new_cdclk_config->vco ||
> > + old_waveform == new_waveform)
> > + return false;
> > +
> > + *mid_cdclk_config = *new_cdclk_config;
> > +
> > + /* If moving to a higher cdclk(squash) the mid cdclk config
> > +* sho
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
> -Original Message-
> From: Ville Syrjälä
> Sent: Thursday, October 20, 2022 4:33 AM
> To: Srivatsa, Anusha
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/i915/display: Do both crawl and squash when
> changing cdclk
>
> On Thu, Oct 13, 2022 at 04:32:22PM -0700, Anusha Srivatsa wrote:
> > From: Ville Syrjälä
> >
> > For MTL, changing cdclk from between certain frequencies has both
> > squash and crawl. Use the current cdclk config and the new(desired)
> > cdclk config to construtc a mid cdclk config.
> > Set the cdclk twice:
> > - Current cdclk -> mid cdclk
> > - mid cdclk -> desired cdclk
> >
> > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk change via
> > modeset for platforms that support squash_crawl sequences(Ville)
> >
> > v3: Add checks for:
> > - scenario where only slow clock is used and cdclk is actually 0
> > (bringing up display).
> > - PLLs are on before looking up the waveform.
> > - Squash and crawl capability checks.(Ville)
> >
> > Signed-off-by: Anusha Srivatsa
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 157
> > +
> > 1 file changed, 128 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index ad401357ab66..430b4cb0a8ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1675,7 +1675,7 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> > const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> > int i;
> >
> > - if (cdclk == dev_priv->display.cdclk.hw.bypass)
> > + if (cdclk == dev_priv->display.cdclk.hw.bypass || cdclk == 0)
>
> cdclk should never be zero. Why is that needed? Hmm. Ah, we do set it to
> zero during sanitation. But that shouldn't matter since we shouldn't call this
> in that case...
>
> > return 0;
> >
> > for (i = 0; table[i].refclk; i++)
> > @@ -1689,37 +1689,72 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> > return 0x;
> > }
> >
> > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > - const struct intel_cdclk_config *cdclk_config,
> > - enum pipe pipe)
> > +static int cdclk_squash_divider(u16 waveform) {
> > + return hweight16(waveform ?: 0x); }
> > +
> > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > + const struct intel_cdclk_config
> *old_cdclk_config,
> > + const struct intel_cdclk_config
> *new_cdclk_config,
> > + struct intel_cdclk_config *mid_cdclk_config)
> {
> > + u16 old_waveform = cdclk_squash_waveform(i915, old_cdclk_config-
> >cdclk);
> > + u16 new_waveform = cdclk_squash_waveform(i915,
> > +new_cdclk_config->cdclk);
>
> ... but here we do call it. Just moving these to be called after the vco
> checks
> should take care of that issue.
Ok. Makes sense.
> > + u16 mid_waveform;
> > + int size = 16;
> > + int div = 2;
> > +
> > + /* Return if both Squash and Crawl are not present */
> > + if (!HAS_CDCLK_CRAWL(i915) || !has_cdclk_squasher(i915))
> > + return false;
> > +
> > + /* Return if Squash only or Crawl only is the desired action */
> > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > + old_cdclk_config->vco == new_cdclk_config->vco ||
> > + old_waveform == new_waveform)
> > + return false;
> > +
> > + *mid_cdclk_config = *new_cdclk_config;
> > +
> > + /* If moving to a higher cdclk(squash) the mid cdclk config
> > +* should have the new (squash) waveform.
> > +* If moving to a lower cdclk (crawl) the mid cdclk config
> > +* should have the new vco.
> > +*/
> > +
> > + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> > + mid_cdclk_config->vco = old_cdclk_config->vco;
> > + mid_waveform = new_waveform;
> > + } else {
> > + mid_cdclk_config->vco = new_cdclk_config->vco;
> > + mid_waveform = old_waveform;
> > + }
> > +
> > + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> > + mid_cdclk_config->vco, size
> * div);
> > +
> > + /* make sure the mid clock came out sane */
> > +
> > + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> > + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> > + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> > + i915->display.cdclk.max_cdclk_freq);
> > + drm_WARN_ON(>drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> > + mid_waveform);
> > +
> > + return true;
> > +}
> > +
> > +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > +
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On Thu, Oct 20, 2022 at 08:12:04PM +0530, Balasubramani Vivekanandan wrote:
> On 13.10.2022 16:32, Anusha Srivatsa wrote:
> > From: Ville Syrjälä
> >
> > For MTL, changing cdclk from between certain frequencies has
> > both squash and crawl. Use the current cdclk config and
> > the new(desired) cdclk config to construtc a mid cdclk config.
> > Set the cdclk twice:
> > - Current cdclk -> mid cdclk
> > - mid cdclk -> desired cdclk
> >
> > v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> > change via modeset for platforms that support squash_crawl sequences(Ville)
> >
> > v3: Add checks for:
> > - scenario where only slow clock is used and
> > cdclk is actually 0 (bringing up display).
> > - PLLs are on before looking up the waveform.
> > - Squash and crawl capability checks.(Ville)
> >
> > Signed-off-by: Anusha Srivatsa
> > Signed-off-by: Ville Syrjälä
>
> Please add the Bspec number.
>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 157 +
> > 1 file changed, 128 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index ad401357ab66..430b4cb0a8ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1675,7 +1675,7 @@ static u32 cdclk_squash_waveform(struct
> > drm_i915_private *dev_priv,
> > const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> > int i;
> >
> > - if (cdclk == dev_priv->display.cdclk.hw.bypass)
> > + if (cdclk == dev_priv->display.cdclk.hw.bypass || cdclk == 0)
> > return 0;
> >
> > for (i = 0; table[i].refclk; i++)
> > @@ -1689,37 +1689,72 @@ static u32 cdclk_squash_waveform(struct
> > drm_i915_private *dev_priv,
> > return 0x;
> > }
> >
> > -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > - const struct intel_cdclk_config *cdclk_config,
> > - enum pipe pipe)
> > +static int cdclk_squash_divider(u16 waveform)
> > +{
> > + return hweight16(waveform ?: 0x);
> > +}
> > +
> > +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> > + const struct intel_cdclk_config
> > *old_cdclk_config,
> > + const struct intel_cdclk_config
> > *new_cdclk_config,
> > + struct intel_cdclk_config *mid_cdclk_config)
> > +{
>
> I was thinking of asking to rename this function to a more descriptive
> one, but then I myself was not able to come up with one.
> For a fresh eyes, it is difficult to make out what this function is
> actually doing. Can you please add a summary as a comment above this
> function pointing out what is mid_cdclk and whats the meaning of its
> return value.
>
> > + u16 old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
> > + u16 new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
> > + u16 mid_waveform;
> > + int size = 16;
> > + int div = 2;
> > +
> > + /* Return if both Squash and Crawl are not present */
> > + if (!HAS_CDCLK_CRAWL(i915) || !has_cdclk_squasher(i915))
> > + return false;
>
> Can cdclk_squasher feature availability be also made a part of
> device_info structure like cdclk_crawl and create a macro similar to
> HAS_CDCLK_CRAWL?
> Like Ville said it looks bit weird. Also we would avoid adding platform
> checks inside has_cdclk_squasher() function like it is done now in your
> second patch.
>
> > +
> > + /* Return if Squash only or Crawl only is the desired action */
> > + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> > + old_cdclk_config->vco == new_cdclk_config->vco ||
> > + old_waveform == new_waveform)
> > + return false;
> > +
> > + *mid_cdclk_config = *new_cdclk_config;
> > +
> > + /* If moving to a higher cdclk(squash) the mid cdclk config
> > +* should have the new (squash) waveform.
> > +* If moving to a lower cdclk (crawl) the mid cdclk config
> > +* should have the new vco.
> > +*/
> > +
> > + if (cdclk_squash_divider(new_waveform) >
> > cdclk_squash_divider(old_waveform)) {
> > + mid_cdclk_config->vco = old_cdclk_config->vco;
> > + mid_waveform = new_waveform;
> > + } else {
> > + mid_cdclk_config->vco = new_cdclk_config->vco;
> > + mid_waveform = old_waveform;
> > + }
> > +
> > + mid_cdclk_config->cdclk =
> > DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> > + mid_cdclk_config->vco, size
> > * div);
> > +
> > + /* make sure the mid clock came out sane */
> > +
> > + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> > + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> > + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> > + i915->display.cdclk.max_cdclk_freq);
> >
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On 13.10.2022 16:32, Anusha Srivatsa wrote:
> From: Ville Syrjälä
>
> For MTL, changing cdclk from between certain frequencies has
> both squash and crawl. Use the current cdclk config and
> the new(desired) cdclk config to construtc a mid cdclk config.
> Set the cdclk twice:
> - Current cdclk -> mid cdclk
> - mid cdclk -> desired cdclk
>
> v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> change via modeset for platforms that support squash_crawl sequences(Ville)
>
> v3: Add checks for:
> - scenario where only slow clock is used and
> cdclk is actually 0 (bringing up display).
> - PLLs are on before looking up the waveform.
> - Squash and crawl capability checks.(Ville)
>
> Signed-off-by: Anusha Srivatsa
> Signed-off-by: Ville Syrjälä
Please add the Bspec number.
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 157 +
> 1 file changed, 128 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index ad401357ab66..430b4cb0a8ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1675,7 +1675,7 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> int i;
>
> - if (cdclk == dev_priv->display.cdclk.hw.bypass)
> + if (cdclk == dev_priv->display.cdclk.hw.bypass || cdclk == 0)
> return 0;
>
> for (i = 0; table[i].refclk; i++)
> @@ -1689,37 +1689,72 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> return 0x;
> }
>
> -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> - const struct intel_cdclk_config *cdclk_config,
> - enum pipe pipe)
> +static int cdclk_squash_divider(u16 waveform)
> +{
> + return hweight16(waveform ?: 0x);
> +}
> +
> +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> +const struct intel_cdclk_config
> *old_cdclk_config,
> +const struct intel_cdclk_config
> *new_cdclk_config,
> +struct intel_cdclk_config *mid_cdclk_config)
> +{
I was thinking of asking to rename this function to a more descriptive
one, but then I myself was not able to come up with one.
For a fresh eyes, it is difficult to make out what this function is
actually doing. Can you please add a summary as a comment above this
function pointing out what is mid_cdclk and whats the meaning of its
return value.
> + u16 old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
> + u16 new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
> + u16 mid_waveform;
> + int size = 16;
> + int div = 2;
> +
> + /* Return if both Squash and Crawl are not present */
> + if (!HAS_CDCLK_CRAWL(i915) || !has_cdclk_squasher(i915))
> + return false;
Can cdclk_squasher feature availability be also made a part of
device_info structure like cdclk_crawl and create a macro similar to
HAS_CDCLK_CRAWL?
Like Ville said it looks bit weird. Also we would avoid adding platform
checks inside has_cdclk_squasher() function like it is done now in your
second patch.
> +
> + /* Return if Squash only or Crawl only is the desired action */
> + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> + old_cdclk_config->vco == new_cdclk_config->vco ||
> + old_waveform == new_waveform)
> + return false;
> +
> + *mid_cdclk_config = *new_cdclk_config;
> +
> + /* If moving to a higher cdclk(squash) the mid cdclk config
> + * should have the new (squash) waveform.
> + * If moving to a lower cdclk (crawl) the mid cdclk config
> + * should have the new vco.
> + */
> +
> + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> + mid_cdclk_config->vco = old_cdclk_config->vco;
> + mid_waveform = new_waveform;
> + } else {
> + mid_cdclk_config->vco = new_cdclk_config->vco;
> + mid_waveform = old_waveform;
> + }
> +
> + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> + mid_cdclk_config->vco, size
> * div);
> +
> + /* make sure the mid clock came out sane */
> +
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> + i915->display.cdclk.max_cdclk_freq);
> + drm_WARN_ON(>drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> + mid_waveform);
> +
> + return true;
> +}
> +
> +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> +
Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Do both crawl and squash when changing cdclk
On Thu, Oct 13, 2022 at 04:32:22PM -0700, Anusha Srivatsa wrote:
> From: Ville Syrjälä
>
> For MTL, changing cdclk from between certain frequencies has
> both squash and crawl. Use the current cdclk config and
> the new(desired) cdclk config to construtc a mid cdclk config.
> Set the cdclk twice:
> - Current cdclk -> mid cdclk
> - mid cdclk -> desired cdclk
>
> v2: Add check in intel_modeset_calc_cdclk() to avoid cdclk
> change via modeset for platforms that support squash_crawl sequences(Ville)
>
> v3: Add checks for:
> - scenario where only slow clock is used and
> cdclk is actually 0 (bringing up display).
> - PLLs are on before looking up the waveform.
> - Squash and crawl capability checks.(Ville)
>
> Signed-off-by: Anusha Srivatsa
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 157 +
> 1 file changed, 128 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index ad401357ab66..430b4cb0a8ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1675,7 +1675,7 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> int i;
>
> - if (cdclk == dev_priv->display.cdclk.hw.bypass)
> + if (cdclk == dev_priv->display.cdclk.hw.bypass || cdclk == 0)
cdclk should never be zero. Why is that needed? Hmm. Ah, we do set it to
zero during sanitation. But that shouldn't matter since we shouldn't
call this in that case...
> return 0;
>
> for (i = 0; table[i].refclk; i++)
> @@ -1689,37 +1689,72 @@ static u32 cdclk_squash_waveform(struct
> drm_i915_private *dev_priv,
> return 0x;
> }
>
> -static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> - const struct intel_cdclk_config *cdclk_config,
> - enum pipe pipe)
> +static int cdclk_squash_divider(u16 waveform)
> +{
> + return hweight16(waveform ?: 0x);
> +}
> +
> +static bool cdclk_crawl_and_squash(struct drm_i915_private *i915,
> +const struct intel_cdclk_config
> *old_cdclk_config,
> +const struct intel_cdclk_config
> *new_cdclk_config,
> +struct intel_cdclk_config *mid_cdclk_config)
> +{
> + u16 old_waveform = cdclk_squash_waveform(i915, old_cdclk_config->cdclk);
> + u16 new_waveform = cdclk_squash_waveform(i915, new_cdclk_config->cdclk);
... but here we do call it. Just moving these to be called after
the vco checks should take care of that issue.
> + u16 mid_waveform;
> + int size = 16;
> + int div = 2;
> +
> + /* Return if both Squash and Crawl are not present */
> + if (!HAS_CDCLK_CRAWL(i915) || !has_cdclk_squasher(i915))
> + return false;
> +
> + /* Return if Squash only or Crawl only is the desired action */
> + if (old_cdclk_config->vco <= 0 || new_cdclk_config->vco <= 0 ||
> + old_cdclk_config->vco == new_cdclk_config->vco ||
> + old_waveform == new_waveform)
> + return false;
> +
> + *mid_cdclk_config = *new_cdclk_config;
> +
> + /* If moving to a higher cdclk(squash) the mid cdclk config
> + * should have the new (squash) waveform.
> + * If moving to a lower cdclk (crawl) the mid cdclk config
> + * should have the new vco.
> + */
> +
> + if (cdclk_squash_divider(new_waveform) >
> cdclk_squash_divider(old_waveform)) {
> + mid_cdclk_config->vco = old_cdclk_config->vco;
> + mid_waveform = new_waveform;
> + } else {
> + mid_cdclk_config->vco = new_cdclk_config->vco;
> + mid_waveform = old_waveform;
> + }
> +
> + mid_cdclk_config->cdclk =
> DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
> + mid_cdclk_config->vco, size
> * div);
> +
> + /* make sure the mid clock came out sane */
> +
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk <
> + min(old_cdclk_config->cdclk, new_cdclk_config->cdclk));
> + drm_WARN_ON(>drm, mid_cdclk_config->cdclk >
> + i915->display.cdclk.max_cdclk_freq);
> + drm_WARN_ON(>drm, cdclk_squash_waveform(i915,
> mid_cdclk_config->cdclk) !=
> + mid_waveform);
> +
> + return true;
> +}
> +
> +static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> +const struct intel_cdclk_config *cdclk_config,
> +enum pipe pipe)
> {
> int cdclk = cdclk_config->cdclk;
> int vco = cdclk_config->vco;
> u32 val;
> u16 waveform;
> int clock;
> - int ret;
> -
> - /* Inform power controller of upcoming frequency change. */
> - if (DISPLAY_VER(dev_priv) >= 11)
> -
