diff mbox

[06/35] drm/i915: Pass the watermark level to primary WM compute functions

Message ID 1373014667-19484-7-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä July 5, 2013, 8:57 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Passing the level insted of "is_lp" seems easier. The end result is the
same though.

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

Comments

Paulo Zanoni July 30, 2013, 7:49 p.m. UTC | #1 
2013/7/5  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Passing the level insted of "is_lp" seems easier. The end result is the
> same though.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6b820c4..f178e26 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2185,7 +2185,7 @@ enum hsw_data_buf_partitioning {
>  /* For both WM_PIPE and WM_LP. */
>  static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
>                                    uint32_t mem_value,
> -                                  bool is_lp)
> +                                  int level)
>  {
>         uint32_t method1, method2;
>
> @@ -2197,7 +2197,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
>                                  params->pri_bytes_per_pixel,
>                                  mem_value);
>
> -       if (!is_lp)
> +       if (level == 0)
>                 return method1;
>
>         method2 = ilk_wm_method2(params->pixel_rate,
> @@ -2266,7 +2266,7 @@ static bool hsw_compute_lp_wm(uint32_t mem_value, struct hsw_wm_maximums *max,
>         for (pipe = PIPE_A; pipe <= PIPE_C; pipe++) {
>                 struct hsw_pipe_wm_parameters *p = &params[pipe];
>
> -               pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, true);
> +               pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, 1);

But now even when you're processing levels 2/3/4 you're telling the
function that you're doing level 1, which is not true. We don't know
what kind of code changes we'll be doing in January 2023, so we may
use that incorrect "level" variable to do something wrong. IMHO,
either we keep the current code or we pass the actual level.


>                 spr_val[pipe] = ilk_compute_spr_wm(p, mem_value);
>                 cur_val[pipe] = ilk_compute_cur_wm(p, mem_value);
>                 fbc_val[pipe] = ilk_compute_fbc_wm(p, pri_val[pipe], mem_value);
> @@ -2296,7 +2296,7 @@ static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
>  {
>         uint32_t pri_val, cur_val, spr_val;
>
> -       pri_val = ilk_compute_pri_wm(params, mem_value, false);
> +       pri_val = ilk_compute_pri_wm(params, mem_value, 0);
>         spr_val = ilk_compute_spr_wm(params, mem_value);
>         cur_val = ilk_compute_cur_wm(params, mem_value);
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Aug. 1, 2013, 8:01 a.m. UTC | #2 
On Tue, Jul 30, 2013 at 04:49:18PM -0300, Paulo Zanoni wrote:
> 2013/7/5  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Passing the level insted of "is_lp" seems easier. The end result is the
> > same though.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 6b820c4..f178e26 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2185,7 +2185,7 @@ enum hsw_data_buf_partitioning {
> >  /* For both WM_PIPE and WM_LP. */
> >  static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
> >                                    uint32_t mem_value,
> > -                                  bool is_lp)
> > +                                  int level)
> >  {
> >         uint32_t method1, method2;
> >
> > @@ -2197,7 +2197,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
> >                                  params->pri_bytes_per_pixel,
> >                                  mem_value);
> >
> > -       if (!is_lp)
> > +       if (level == 0)
> >                 return method1;
> >
> >         method2 = ilk_wm_method2(params->pixel_rate,
> > @@ -2266,7 +2266,7 @@ static bool hsw_compute_lp_wm(uint32_t mem_value, struct hsw_wm_maximums *max,
> >         for (pipe = PIPE_A; pipe <= PIPE_C; pipe++) {
> >                 struct hsw_pipe_wm_parameters *p = &params[pipe];
> >
> > -               pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, true);
> > +               pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, 1);
> 
> But now even when you're processing levels 2/3/4 you're telling the
> function that you're doing level 1, which is not true. We don't know
> what kind of code changes we'll be doing in January 2023, so we may
> use that incorrect "level" variable to do something wrong. IMHO,
> either we keep the current code or we pass the actual level.

Yeah. I guess we could keep it as bool now. The original reason for
passing the level was that I handled the IVB cursor latency W/A inside
the wm compute funcs, but now with the pre-populated latency info in
dev_priv passing the level makes less sense.

> 
> 
> >                 spr_val[pipe] = ilk_compute_spr_wm(p, mem_value);
> >                 cur_val[pipe] = ilk_compute_cur_wm(p, mem_value);
> >                 fbc_val[pipe] = ilk_compute_fbc_wm(p, pri_val[pipe], mem_value);
> > @@ -2296,7 +2296,7 @@ static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
> >  {
> >         uint32_t pri_val, cur_val, spr_val;
> >
> > -       pri_val = ilk_compute_pri_wm(params, mem_value, false);
> > +       pri_val = ilk_compute_pri_wm(params, mem_value, 0);
> >         spr_val = ilk_compute_spr_wm(params, mem_value);
> >         cur_val = ilk_compute_cur_wm(params, mem_value);
> >
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6b820c4..f178e26 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2185,7 +2185,7 @@  enum hsw_data_buf_partitioning {
 /* For both WM_PIPE and WM_LP. */
 static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
 				   uint32_t mem_value,
-				   bool is_lp)
+				   int level)
 {
 	uint32_t method1, method2;
 
@@ -2197,7 +2197,7 @@  static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
 				 params->pri_bytes_per_pixel,
 				 mem_value);
 
-	if (!is_lp)
+	if (level == 0)
 		return method1;
 
 	method2 = ilk_wm_method2(params->pixel_rate,
@@ -2266,7 +2266,7 @@  static bool hsw_compute_lp_wm(uint32_t mem_value, struct hsw_wm_maximums *max,
 	for (pipe = PIPE_A; pipe <= PIPE_C; pipe++) {
 		struct hsw_pipe_wm_parameters *p = &params[pipe];
 
-		pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, true);
+		pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, 1);
 		spr_val[pipe] = ilk_compute_spr_wm(p, mem_value);
 		cur_val[pipe] = ilk_compute_cur_wm(p, mem_value);
 		fbc_val[pipe] = ilk_compute_fbc_wm(p, pri_val[pipe], mem_value);
@@ -2296,7 +2296,7 @@  static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
 {
 	uint32_t pri_val, cur_val, spr_val;
 
-	pri_val = ilk_compute_pri_wm(params, mem_value, false);
+	pri_val = ilk_compute_pri_wm(params, mem_value, 0);
 	spr_val = ilk_compute_spr_wm(params, mem_value);
 	cur_val = ilk_compute_cur_wm(params, mem_value);