Re: [Intel-gfx] [PATCH 2/6] drm/i915/hsw: use intel_compute_linetime_wm function for linetime wm

[Mahesh Kumar]

Hi,


On Tuesday 13 June 2017 06:42 PM, Ville Syrjälä wrote:

On Tue, Jun 13, 2017 at 11:34:46AM +0530, Mahesh Kumar wrote:

linetime wm is time taken to fill a single display line with given clock
rate, multiplied by 8.
This patch reuses the common code of hsw_compute_linetime_wm &
skl_compute_linetime_wm.

Signed-off-by: Mahesh Kumar 
---
  drivers/gpu/drm/i915/intel_drv.h |  1 +
  drivers/gpu/drm/i915/intel_pm.c  | 23 ++-
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 83dd40905821..ea2d29e68bfe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1877,6 +1877,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
  int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
  struct intel_crtc_state *cstate);
+uint32_t intel_compute_linetime_wm(const struct intel_crtc_state *cstate);
  static inline int intel_enable_rc6(void)
  {
return i915.enable_rc6;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 155f54a1f516..76ffb00c6ce4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2746,8 +2746,7 @@ hsw_compute_linetime_wm(const struct intel_crtc_state 
*cstate)
/* The WM are computed with base on how long it takes to fill a single
 * row at the given clock rate, multiplied by 8.
 * */
-   linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
-adjusted_mode->crtc_clock);
+   linetime = intel_compute_linetime_wm(cstate);

HSW/BDW spec says "The WM_LINETIME register Line Time field (bits 8:0) is
not adjusted for panel fitter down scaling.", so NAK.

ok...



ips_linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
 intel_state->cdclk.logical.cdclk);
  
@@ -4382,7 +4381,7 @@ static uint_fixed_16_16_t skl_wm_method2(uint32_t pixel_rate,

  }
  
  static uint_fixed_16_16_t

-intel_get_linetime_us(struct intel_crtc_state *cstate)
+intel_get_linetime_us(const struct intel_crtc_state *cstate)
  {
uint32_t pixel_rate;
uint32_t crtc_htotal;
@@ -4604,20 +4603,26 @@ skl_compute_wm_levels(const struct drm_i915_private 
*dev_priv,
return 0;
  }
  
-static uint32_t

-skl_compute_linetime_wm(struct intel_crtc_state *cstate)
+uint32_t intel_compute_linetime_wm(const struct intel_crtc_state *cstate)
  {
-   struct drm_atomic_state *state = cstate->base.state;
-   struct drm_i915_private *dev_priv = to_i915(state->dev);
uint_fixed_16_16_t linetime_us;
-   uint32_t linetime_wm;
  
  	linetime_us = intel_get_linetime_us(cstate);
  
  	if (is_fixed16_zero(linetime_us))

return 0;
  
-	linetime_wm = fixed16_to_u32_round_up(mul_u32_fixed16(8, linetime_us));

+   return fixed16_to_u32_round_up(mul_u32_fixed16(8, linetime_us));

Am I the only one that finds this fixed16 stuff totally illegible?
This is also inefficient since it's doing multiple divisions where
one would suffice. I'd prefer people just wrote the code the in
the obvious (and more optimal) way.

better late than never. let's discuss & optimize fixed16 :)

-Mahesh



+}
+
+static uint32_t
+skl_compute_linetime_wm(struct intel_crtc_state *cstate)
+{
+   struct drm_atomic_state *state = cstate->base.state;
+   struct drm_i915_private *dev_priv = to_i915(state->dev);
+   uint32_t linetime_wm;
+
+   linetime_wm = intel_compute_linetime_wm(cstate);
  
  	/* Display WA #1135: bxt. */

if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)
--
2.13.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/hsw: use intel_compute_linetime_wm function for linetime wm

[Ville Syrjälä]

On Tue, Jun 13, 2017 at 11:34:46AM +0530, Mahesh Kumar wrote:
> linetime wm is time taken to fill a single display line with given clock
> rate, multiplied by 8.
> This patch reuses the common code of hsw_compute_linetime_wm &
> skl_compute_linetime_wm.
> 
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 23 ++-
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 83dd40905821..ea2d29e68bfe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1877,6 +1877,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
>  int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> struct intel_crtc_state *cstate);
> +uint32_t intel_compute_linetime_wm(const struct intel_crtc_state *cstate);
>  static inline int intel_enable_rc6(void)
>  {
>   return i915.enable_rc6;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 155f54a1f516..76ffb00c6ce4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2746,8 +2746,7 @@ hsw_compute_linetime_wm(const struct intel_crtc_state 
> *cstate)
>   /* The WM are computed with base on how long it takes to fill a single
>* row at the given clock rate, multiplied by 8.
>* */
> - linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> -  adjusted_mode->crtc_clock);
> + linetime = intel_compute_linetime_wm(cstate);

HSW/BDW spec says "The WM_LINETIME register Line Time field (bits 8:0) is
not adjusted for panel fitter down scaling.", so NAK.

>   ips_linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
>intel_state->cdclk.logical.cdclk);
>  
> @@ -4382,7 +4381,7 @@ static uint_fixed_16_16_t skl_wm_method2(uint32_t 
> pixel_rate,
>  }
>  
>  static uint_fixed_16_16_t
> -intel_get_linetime_us(struct intel_crtc_state *cstate)
> +intel_get_linetime_us(const struct intel_crtc_state *cstate)
>  {
>   uint32_t pixel_rate;
>   uint32_t crtc_htotal;
> @@ -4604,20 +4603,26 @@ skl_compute_wm_levels(const struct drm_i915_private 
> *dev_priv,
>   return 0;
>  }
>  
> -static uint32_t
> -skl_compute_linetime_wm(struct intel_crtc_state *cstate)
> +uint32_t intel_compute_linetime_wm(const struct intel_crtc_state *cstate)
>  {
> - struct drm_atomic_state *state = cstate->base.state;
> - struct drm_i915_private *dev_priv = to_i915(state->dev);
>   uint_fixed_16_16_t linetime_us;
> - uint32_t linetime_wm;
>  
>   linetime_us = intel_get_linetime_us(cstate);
>  
>   if (is_fixed16_zero(linetime_us))
>   return 0;
>  
> - linetime_wm = fixed16_to_u32_round_up(mul_u32_fixed16(8, linetime_us));
> + return fixed16_to_u32_round_up(mul_u32_fixed16(8, linetime_us));

Am I the only one that finds this fixed16 stuff totally illegible?
This is also inefficient since it's doing multiple divisions where
one would suffice. I'd prefer people just wrote the code the in
the obvious (and more optimal) way.

> +}
> +
> +static uint32_t
> +skl_compute_linetime_wm(struct intel_crtc_state *cstate)
> +{
> + struct drm_atomic_state *state = cstate->base.state;
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> + uint32_t linetime_wm;
> +
> + linetime_wm = intel_compute_linetime_wm(cstate);
>  
>   /* Display WA #1135: bxt. */
>   if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)
> -- 
> 2.13.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/hsw: use intel_compute_linetime_wm function for linetime wm

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 13-06-2017 om 11:34 [+0530]:
> linetime wm is time taken to fill a single display line with given
> clock
> rate, multiplied by 8.
> This patch reuses the common code of hsw_compute_linetime_wm &
> skl_compute_linetime_wm.
> 
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 23 ++-
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 83dd40905821..ea2d29e68bfe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1877,6 +1877,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
>  int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>     struct intel_crtc_state *cstate);
> +uint32_t intel_compute_linetime_wm(const struct intel_crtc_state
> *cstate);
This function is only used inside intel_pm.c, so should only be
declared there. :)

~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/hsw: use intel_compute_linetime_wm function for linetime wm

[Mahesh Kumar]

On Tuesday 13 June 2017 01:00 PM, Lankhorst, Maarten wrote:

Mahesh Kumar schreef op di 13-06-2017 om 11:34 [+0530]:

linetime wm is time taken to fill a single display line with given
clock
rate, multiplied by 8.
This patch reuses the common code of hsw_compute_linetime_wm &
skl_compute_linetime_wm.

Signed-off-by: Mahesh Kumar 
---
  drivers/gpu/drm/i915/intel_drv.h |  1 +
  drivers/gpu/drm/i915/intel_pm.c  | 23 ++-
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index 83dd40905821..ea2d29e68bfe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1877,6 +1877,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
enable_rc6);
  int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
  struct intel_crtc_state *cstate);
+uint32_t intel_compute_linetime_wm(const struct intel_crtc_state
*cstate);

This function is only used inside intel_pm.c, so should only be
declared there. :)

ok will reorganize..
-Mahesh


~Maarten


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/6] drm/i915/hsw: use intel_compute_linetime_wm function for linetime wm

[Mahesh Kumar]

linetime wm is time taken to fill a single display line with given clock
rate, multiplied by 8.
This patch reuses the common code of hsw_compute_linetime_wm &
skl_compute_linetime_wm.

Signed-off-by: Mahesh Kumar 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 23 ++-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 83dd40905821..ea2d29e68bfe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1877,6 +1877,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
 int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
  struct intel_crtc_state *cstate);
+uint32_t intel_compute_linetime_wm(const struct intel_crtc_state *cstate);
 static inline int intel_enable_rc6(void)
 {
return i915.enable_rc6;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 155f54a1f516..76ffb00c6ce4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2746,8 +2746,7 @@ hsw_compute_linetime_wm(const struct intel_crtc_state 
*cstate)
/* The WM are computed with base on how long it takes to fill a single
 * row at the given clock rate, multiplied by 8.
 * */
-   linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
-adjusted_mode->crtc_clock);
+   linetime = intel_compute_linetime_wm(cstate);
ips_linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
 intel_state->cdclk.logical.cdclk);
 
@@ -4382,7 +4381,7 @@ static uint_fixed_16_16_t skl_wm_method2(uint32_t 
pixel_rate,
 }
 
 static uint_fixed_16_16_t
-intel_get_linetime_us(struct intel_crtc_state *cstate)
+intel_get_linetime_us(const struct intel_crtc_state *cstate)
 {
uint32_t pixel_rate;
uint32_t crtc_htotal;
@@ -4604,20 +4603,26 @@ skl_compute_wm_levels(const struct drm_i915_private 
*dev_priv,
return 0;
 }
 
-static uint32_t
-skl_compute_linetime_wm(struct intel_crtc_state *cstate)
+uint32_t intel_compute_linetime_wm(const struct intel_crtc_state *cstate)
 {
-   struct drm_atomic_state *state = cstate->base.state;
-   struct drm_i915_private *dev_priv = to_i915(state->dev);
uint_fixed_16_16_t linetime_us;
-   uint32_t linetime_wm;
 
linetime_us = intel_get_linetime_us(cstate);
 
if (is_fixed16_zero(linetime_us))
return 0;
 
-   linetime_wm = fixed16_to_u32_round_up(mul_u32_fixed16(8, linetime_us));
+   return fixed16_to_u32_round_up(mul_u32_fixed16(8, linetime_us));
+}
+
+static uint32_t
+skl_compute_linetime_wm(struct intel_crtc_state *cstate)
+{
+   struct drm_atomic_state *state = cstate->base.state;
+   struct drm_i915_private *dev_priv = to_i915(state->dev);
+   uint32_t linetime_wm;
+
+   linetime_wm = intel_compute_linetime_wm(cstate);
 
/* Display WA #1135: bxt. */
if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)
-- 
2.13.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx