Re: [PATCH 4/4] drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing

2024-01-05 Thread Matt Roper 
On Fri, Jan 05, 2024 at 11:09:37AM -0300, Gustavo Sousa wrote:
> Quoting Matt Roper (2024-01-04 20:52:32-03:00)
> >On Thu, Jan 04, 2024 at 03:48:34PM -0800, Matt Roper wrote:
> >> On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
> >> > That's the function responsible for deriving that register's value; use
> >> > it.
> >> > 
> >> > Signed-off-by: Gustavo Sousa 
> >> 
> >> Reviewed-by: Matt Roper 
> >
> >Forgot to mention...I think it's a bit jarring when the commit message
> >starts out referring to something in the headline ("That's the
> >function...").  It's probably a bit better to just re-state the function
> >name in the commit message again rather than assuming both get read
> >together.
> 
> Thanks for the suggestion!
> 
> I have sent a v2 updating the body of commit messages, not only for this one 
> but
> also for two more patches. I have kept your r-b's. Let me know if that's okay.

Yep, the r-b's still apply.  Thanks.


Matt

> 
> --
> Gustavo Sousa
> 
> >
> >
> >Matt
> >
> >> 
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++---
> >> >  1 file changed, 3 insertions(+), 23 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
> >> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > index fbe9aba41c35..26200ee3e23f 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private 
> >> > *dev_priv,
> >> >  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >> >  {
> >> >  u32 cdctl, expected;
> >> > -int cdclk, clock, vco;
> >> > +int cdclk, vco;
> >> >  
> >> >  intel_update_cdclk(dev_priv);
> >> >  intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, 
> >> > "Current CDCLK");
> >> > @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct 
> >> > drm_i915_private *dev_priv)
> >> >   * so sanitize this register.
> >> >   */
> >> >  cdctl = intel_de_read(dev_priv, CDCLK_CTL);
> >> > +expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, 
> >> > INVALID_PIPE);
> >> >  
> >> >  /*
> >> >   * Let's ignore the pipe field, since BIOS could have 
> >> > configured the
> >> > @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct 
> >> > drm_i915_private *dev_priv)
> >> >   * (PIPE_NONE).
> >> >   */
> >> >  cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >> > -
> >> > -if (DISPLAY_VER(dev_priv) >= 20)
> >> > -expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
> >> > -else
> >> > -expected = skl_cdclk_decimal(cdclk);
> >> > -
> >> > -/* Figure out what CD2X divider we should be using for this 
> >> > cdclk */
> >> > -if (HAS_CDCLK_SQUASH(dev_priv))
> >> > -clock = dev_priv->display.cdclk.hw.vco / 2;
> >> > -else
> >> > -clock = dev_priv->display.cdclk.hw.cdclk;
> >> > -
> >> > -expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
> >> > -   
> >> > dev_priv->display.cdclk.hw.vco);
> >> > -
> >> > -/*
> >> > - * Disable SSA Precharge when CD clock frequency < 500 MHz,
> >> > - * enable otherwise.
> >> > - */
> >> > -if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> >> > -dev_priv->display.cdclk.hw.cdclk >= 50)
> >> > -expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> >> > +expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >> >  
> >> >  if (cdctl == expected)
> >> >  /* All well; nothing to sanitize */
> >> > -- 
> >> > 2.43.0
> >> > 
> >> 
> >> -- 
> >> Matt Roper
> >> Graphics Software Engineer
> >> Linux GPU Platform Enablement
> >> Intel Corporation
> >
> >-- 
> >Matt Roper
> >Graphics Software Engineer
> >Linux GPU Platform Enablement
> >Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCH 4/4] drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing

2024-01-05 Thread Gustavo Sousa 
Quoting Matt Roper (2024-01-04 20:52:32-03:00)
>On Thu, Jan 04, 2024 at 03:48:34PM -0800, Matt Roper wrote:
>> On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
>> > That's the function responsible for deriving that register's value; use
>> > it.
>> > 
>> > Signed-off-by: Gustavo Sousa 
>> 
>> Reviewed-by: Matt Roper 
>
>Forgot to mention...I think it's a bit jarring when the commit message
>starts out referring to something in the headline ("That's the
>function...").  It's probably a bit better to just re-state the function
>name in the commit message again rather than assuming both get read
>together.

Thanks for the suggestion!

I have sent a v2 updating the body of commit messages, not only for this one but
also for two more patches. I have kept your r-b's. Let me know if that's okay.

--
Gustavo Sousa

>
>
>Matt
>
>> 
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++---
>> >  1 file changed, 3 insertions(+), 23 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
>> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > index fbe9aba41c35..26200ee3e23f 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private 
>> > *dev_priv,
>> >  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> >  {
>> >  u32 cdctl, expected;
>> > -int cdclk, clock, vco;
>> > +int cdclk, vco;
>> >  
>> >  intel_update_cdclk(dev_priv);
>> >  intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, 
>> > "Current CDCLK");
>> > @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct 
>> > drm_i915_private *dev_priv)
>> >   * so sanitize this register.
>> >   */
>> >  cdctl = intel_de_read(dev_priv, CDCLK_CTL);
>> > +expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, 
>> > INVALID_PIPE);
>> >  
>> >  /*
>> >   * Let's ignore the pipe field, since BIOS could have configured 
>> > the
>> > @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct 
>> > drm_i915_private *dev_priv)
>> >   * (PIPE_NONE).
>> >   */
>> >  cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
>> > -
>> > -if (DISPLAY_VER(dev_priv) >= 20)
>> > -expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
>> > -else
>> > -expected = skl_cdclk_decimal(cdclk);
>> > -
>> > -/* Figure out what CD2X divider we should be using for this cdclk 
>> > */
>> > -if (HAS_CDCLK_SQUASH(dev_priv))
>> > -clock = dev_priv->display.cdclk.hw.vco / 2;
>> > -else
>> > -clock = dev_priv->display.cdclk.hw.cdclk;
>> > -
>> > -expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
>> > -   
>> > dev_priv->display.cdclk.hw.vco);
>> > -
>> > -/*
>> > - * Disable SSA Precharge when CD clock frequency < 500 MHz,
>> > - * enable otherwise.
>> > - */
>> > -if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
>> > -dev_priv->display.cdclk.hw.cdclk >= 50)
>> > -expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
>> > +expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
>> >  
>> >  if (cdctl == expected)
>> >  /* All well; nothing to sanitize */
>> > -- 
>> > 2.43.0
>> > 
>> 
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation

Re: [PATCH 4/4] drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing

2024-01-04 Thread Matt Roper 
On Thu, Jan 04, 2024 at 03:48:34PM -0800, Matt Roper wrote:
> On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
> > That's the function responsible for deriving that register's value; use
> > it.
> > 
> > Signed-off-by: Gustavo Sousa 
> 
> Reviewed-by: Matt Roper 

Forgot to mention...I think it's a bit jarring when the commit message
starts out referring to something in the headline ("That's the
function...").  It's probably a bit better to just re-state the function
name in the commit message again rather than assuming both get read
together.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++---
> >  1 file changed, 3 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index fbe9aba41c35..26200ee3e23f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private 
> > *dev_priv,
> >  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >  {
> > u32 cdctl, expected;
> > -   int cdclk, clock, vco;
> > +   int cdclk, vco;
> >  
> > intel_update_cdclk(dev_priv);
> > intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current 
> > CDCLK");
> > @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct 
> > drm_i915_private *dev_priv)
> >  * so sanitize this register.
> >  */
> > cdctl = intel_de_read(dev_priv, CDCLK_CTL);
> > +   expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, 
> > INVALID_PIPE);
> >  
> > /*
> >  * Let's ignore the pipe field, since BIOS could have configured the
> > @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct 
> > drm_i915_private *dev_priv)
> >  * (PIPE_NONE).
> >  */
> > cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> > -
> > -   if (DISPLAY_VER(dev_priv) >= 20)
> > -   expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
> > -   else
> > -   expected = skl_cdclk_decimal(cdclk);
> > -
> > -   /* Figure out what CD2X divider we should be using for this cdclk */
> > -   if (HAS_CDCLK_SQUASH(dev_priv))
> > -   clock = dev_priv->display.cdclk.hw.vco / 2;
> > -   else
> > -   clock = dev_priv->display.cdclk.hw.cdclk;
> > -
> > -   expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
> > -  dev_priv->display.cdclk.hw.vco);
> > -
> > -   /*
> > -* Disable SSA Precharge when CD clock frequency < 500 MHz,
> > -* enable otherwise.
> > -*/
> > -   if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> > -   dev_priv->display.cdclk.hw.cdclk >= 50)
> > -   expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> > +   expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >  
> > if (cdctl == expected)
> > /* All well; nothing to sanitize */
> > -- 
> > 2.43.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCH 4/4] drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing

2024-01-04 Thread Matt Roper 
On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
> That's the function responsible for deriving that register's value; use
> it.
> 
> Signed-off-by: Gustavo Sousa 

Reviewed-by: Matt Roper 

> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++---
>  1 file changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index fbe9aba41c35..26200ee3e23f 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private 
> *dev_priv,
>  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  {
>   u32 cdctl, expected;
> - int cdclk, clock, vco;
> + int cdclk, vco;
>  
>   intel_update_cdclk(dev_priv);
>   intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current 
> CDCLK");
> @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>* so sanitize this register.
>*/
>   cdctl = intel_de_read(dev_priv, CDCLK_CTL);
> + expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, 
> INVALID_PIPE);
>  
>   /*
>* Let's ignore the pipe field, since BIOS could have configured the
> @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>* (PIPE_NONE).
>*/
>   cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> -
> - if (DISPLAY_VER(dev_priv) >= 20)
> - expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
> - else
> - expected = skl_cdclk_decimal(cdclk);
> -
> - /* Figure out what CD2X divider we should be using for this cdclk */
> - if (HAS_CDCLK_SQUASH(dev_priv))
> - clock = dev_priv->display.cdclk.hw.vco / 2;
> - else
> - clock = dev_priv->display.cdclk.hw.cdclk;
> -
> - expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
> -dev_priv->display.cdclk.hw.vco);
> -
> - /*
> -  * Disable SSA Precharge when CD clock frequency < 500 MHz,
> -  * enable otherwise.
> -  */
> - if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> - dev_priv->display.cdclk.hw.cdclk >= 50)
> - expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> + expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
>  
>   if (cdctl == expected)
>   /* All well; nothing to sanitize */
> -- 
> 2.43.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

[PATCH 4/4] drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing

2024-01-03 Thread Gustavo Sousa 
That's the function responsible for deriving that register's value; use
it.

Signed-off-by: Gustavo Sousa 
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++---
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index fbe9aba41c35..26200ee3e23f 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private 
*dev_priv,
 static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
 {
u32 cdctl, expected;
-   int cdclk, clock, vco;
+   int cdclk, vco;
 
intel_update_cdclk(dev_priv);
intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current 
CDCLK");
@@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private 
*dev_priv)
 * so sanitize this register.
 */
cdctl = intel_de_read(dev_priv, CDCLK_CTL);
+   expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, 
INVALID_PIPE);
 
/*
 * Let's ignore the pipe field, since BIOS could have configured the
@@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private 
*dev_priv)
 * (PIPE_NONE).
 */
cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
-
-   if (DISPLAY_VER(dev_priv) >= 20)
-   expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
-   else
-   expected = skl_cdclk_decimal(cdclk);
-
-   /* Figure out what CD2X divider we should be using for this cdclk */
-   if (HAS_CDCLK_SQUASH(dev_priv))
-   clock = dev_priv->display.cdclk.hw.vco / 2;
-   else
-   clock = dev_priv->display.cdclk.hw.cdclk;
-
-   expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
-  dev_priv->display.cdclk.hw.vco);
-
-   /*
-* Disable SSA Precharge when CD clock frequency < 500 MHz,
-* enable otherwise.
-*/
-   if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
-   dev_priv->display.cdclk.hw.cdclk >= 50)
-   expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
+   expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
 
if (cdctl == expected)
/* All well; nothing to sanitize */
-- 
2.43.0