Re: [Intel-gfx] [PATCH 1/7] drm/i915: Calculate gen3- watermarks semi-atomically.

[Maarten Lankhorst]

Op 08-08-17 om 14:51 schreef Mahesh Kumar:

> Hi,
>> +
>> +struct {
>> +uint16_t plane;
> should this also be named as plane_wm, because it's again wm with SR. 
> just a nitpick but not a stopper.
Ack, will change (and for the other patches in this series too).
>
>> @@ -591,6 +600,9 @@ struct intel_crtc_wm_state {
>>   /* optimal watermarks */
>>   struct g4x_wm_state optimal;
>>   } g4x;
> new line please to match the coding style of function
+1
>> +if (!plane_state || !intel_wm_plane_visible(crtc_state, 
>> to_intel_plane_state(plane_state))) {
>> +wm_state->plane_wm = fifo_size - wm_info->guard_size;
>> +if (wm_state->plane_wm > (long)wm_info->max_wm)
>> +wm_state->plane_wm = wm_info->max_wm;
>> +} else if (HAS_FW_BLC(dev_priv)) {
> This part will be called only for (GEN > 2,) So we'll never be 
> calculating wm for GEN2. but you are changing GEN2 wm also to 2 stages 
> compute & update in intel_init_pm.
Well spotted, should be fixed in a bit.
>> +struct drm_framebuffer *fb = plane_state->fb;
>> +unsigned cpp = IS_GEN2(dev_priv) ? 4 : fb->format->cpp[0];
>>   const struct drm_display_mode *adjusted_mode =
>> ->config->base.adjusted_mode;
>> -const struct drm_framebuffer *fb =
>> -crtc->base.primary->state->fb;
>> -int cpp;
>> +_state->base.adjusted_mode;
>> +unsigned active_crtcs;
>> +bool may_cxsr;
>>   
>> -if (IS_GEN2(dev_priv))
>> -cpp = 4;
>> +if (state->modeset)
>> +active_crtcs = state->active_crtcs;
>>   else
>> -cpp = fb->format->cpp[0];
>> +active_crtcs = dev_priv->active_crtcs;
>>   
>> -planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
>> -   wm_info, fifo_size, cpp,
>> -   pessimal_latency_ns);
>> -enabled = crtc;
>> -} else {
>> -planea_wm = fifo_size - wm_info->guard_size;
>> -if (planea_wm > (long)wm_info->max_wm)
>> -planea_wm = wm_info->max_wm;
>> +may_cxsr = active_crtcs == drm_crtc_mask(>base);
> It would be good to have a comment stating if only one CRTC is enabled 
> we can go to cxsr.
> we can use hweight32() function to check if only one CRTC is enabled 
> (not mandatory).
Added the comment, I think this makes it more clear for now.
>> +DRM_DEBUG_KMS("self-refresh entries: %ld\n", entries);
>> +
>> +if (wm_info->fifo_size >= entries) {
> Earlier if (fifo_size < entries) we were assigning srwm = 1 & not 
> disabling cxsr, Is there any specific Bspec documentation to not enable 
> cxsr if fifo_size < entries ?
Not sure, but it felt like this was an overflow.. Maybe I can find something on 
bspec about it.
>> +wm_state->cxsr = true;
>> +wm_state->sr.plane = wm_info->fifo_size - entries;
>> +} else
>> +may_cxsr = false;
> may_cxsr is not used later, so no need to overwrite the value.
True, I dump wm_state->cxsr now.
>> -if (IS_GEN2(dev_priv))
>> -cpp = 4;
>> +if (plane == PLANE_A)
> here plane is of type intel_plane but you are comparing with enum plane, 
> I think you meant plane->plane

Right, nicely caught! Compiler didn't warn because PLANE_A == 0, it did 
complain if I changed it to PLANE_B:

drivers/gpu/drm/i915/intel_pm.c: In function ‘i9xx_wm_get_hw_state’:
drivers/gpu/drm/i915/intel_pm.c:2351:13: error: comparison between pointer and 
integer [-Werror]
   if (plane == PLANE_B)

>> +wm_state->plane_wm = planea_wm;
>>   else
>> -cpp = fb->format->cpp[0];
>> +wm_state->plane_wm = planeb_wm;
>> +
>> +crtc->wm.active.i9xx = *wm_state;
>> +}
>> +}
>>   
>> -planeb_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
>> -   wm_info, fifo_size, cpp,
>> -   pessimal_latency_ns);
>> +static void i9xx_update_wm(struct intel_crtc *crtc)
>> +{
>> +struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +uint32_t fwater_lo;
>> +uint32_t fwater_hi;
>> +int cwm, srwm = -1;
>> +int planea_wm, planeb_wm;
>> +struct intel_crtc *enabled = NULL;
> enabled is used to check if cxsr can be enabled, IMHO it would be good 
> to take a bool variable for same & replace use of enabled with 
> respective intel_crtc variable.
> if it seems complex then this approach is also good with me.

Yeah agreed, it should at least look at wm_state->cxsr. I need to keep the 
enabled check
in case pipe A gets enabled after pipe B. In that case watermarks on A are 
updated before
B, and we should really kill CXSR already at that point..

I'll just add the cxsr check, that should do it. :)

>> +
>> +crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
>> +
>> +crtc = intel_get_crtc_for_plane(dev_priv, 0);
>> +   

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Calculate gen3- watermarks semi-atomically.

[Mahesh Kumar]

Hi,


On Monday 07 August 2017 04:18 PM, Maarten Lankhorst wrote:

The gen3 watermark calculations are converted to atomic,
but the wm update calls are still done through the legacy
functions.

This will make it easier to bisect things if they go wrong.

Signed-off-by: Maarten Lankhorst 
---
  drivers/gpu/drm/i915/intel_display.c |   3 +-
  drivers/gpu/drm/i915/intel_drv.h |  14 +++
  drivers/gpu/drm/i915/intel_pm.c  | 231 +--
  3 files changed, 152 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 974d4b577189..234b94cafc7d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14709,7 +14709,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
skl_wm_get_hw_state(dev);
} else if (HAS_PCH_SPLIT(dev_priv)) {
ilk_wm_get_hw_state(dev);
-   }
+   } else if (INTEL_GEN(dev_priv) <= 3 && !IS_PINEVIEW(dev_priv))
+   i9xx_wm_get_hw_state(dev);
  
  	for_each_intel_crtc(dev, crtc) {

u64 put_domains;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f91de9cb9697..d53d34756048 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -547,6 +547,15 @@ struct g4x_wm_state {
bool fbc_en;
  };
  
+struct i9xx_wm_state {

+   uint16_t plane_wm;
+   bool cxsr;
+
+   struct {
+   uint16_t plane;
should this also be named as plane_wm, because it's again wm with SR. 
just a nitpick but not a stopper.

+   } sr;
+};
+
  struct intel_crtc_wm_state {
union {
struct {
@@ -591,6 +600,9 @@ struct intel_crtc_wm_state {
/* optimal watermarks */
struct g4x_wm_state optimal;
} g4x;

new line please to match the coding style of function

+   struct {
+   struct i9xx_wm_state optimal;
+   } i9xx;
};
  
  	/*

@@ -823,6 +835,7 @@ struct intel_crtc {
struct intel_pipe_wm ilk;
struct vlv_wm_state vlv;
struct g4x_wm_state g4x;
+   struct i9xx_wm_state i9xx;
} active;
} wm;
  
@@ -1845,6 +1858,7 @@ void gen6_rps_boost(struct drm_i915_gem_request *rq,

struct intel_rps_client *rps);
  void intel_queue_rps_boost_for_request(struct drm_i915_gem_request *req);
  void g4x_wm_get_hw_state(struct drm_device *dev);
+void i9xx_wm_get_hw_state(struct drm_device *dev);
  void vlv_wm_get_hw_state(struct drm_device *dev);
  void ilk_wm_get_hw_state(struct drm_device *dev);
  void skl_wm_get_hw_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6e393b217450..807c9a730020 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2224,89 +2224,154 @@ static void i965_update_wm(struct intel_crtc 
*unused_crtc)
  
  #undef FW_WM
  
-static void i9xx_update_wm(struct intel_crtc *unused_crtc)

+static const struct intel_watermark_params *i9xx_get_wm_info(struct 
drm_i915_private *dev_priv,
+struct intel_crtc 
*crtc)
  {
-   struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
-   const struct intel_watermark_params *wm_info;
-   uint32_t fwater_lo;
-   uint32_t fwater_hi;
-   int cwm, srwm = 1;
-   int fifo_size;
-   int planea_wm, planeb_wm;
-   struct intel_crtc *crtc, *enabled = NULL;
+   struct intel_plane *plane = to_intel_plane(crtc->base.primary);
  
  	if (IS_I945GM(dev_priv))

-   wm_info = _wm_info;
+   return _wm_info;
else if (!IS_GEN2(dev_priv))
-   wm_info = _wm_info;
+   return _wm_info;
+   else if (plane->plane == PLANE_A)
+   return _a_wm_info;
else
-   wm_info = _a_wm_info;
+   return _bc_wm_info;
+}
  
-	fifo_size = dev_priv->display.get_fifo_size(dev_priv, 0);

-   crtc = intel_get_crtc_for_plane(dev_priv, 0);
-   if (intel_crtc_active(crtc)) {
+static int i9xx_compute_pipe_wm(struct intel_crtc_state *crtc_state)
+{
+   struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+   struct intel_atomic_state *state =
+   to_intel_atomic_state(crtc_state->base.state);
+   struct i9xx_wm_state *wm_state = _state->wm.i9xx.optimal;
+   struct intel_plane *plane = to_intel_plane(crtc->base.primary);
+   const struct drm_plane_state *plane_state = NULL;
+   int fifo_size;
+   const struct intel_watermark_params *wm_info;
+
+   fifo_size = 

[Intel-gfx] [PATCH 1/7] drm/i915: Calculate gen3- watermarks semi-atomically.

[Maarten Lankhorst]

The gen3 watermark calculations are converted to atomic,
but the wm update calls are still done through the legacy
functions.

This will make it easier to bisect things if they go wrong.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/intel_display.c |   3 +-
 drivers/gpu/drm/i915/intel_drv.h |  14 +++
 drivers/gpu/drm/i915/intel_pm.c  | 231 +--
 3 files changed, 152 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 974d4b577189..234b94cafc7d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14709,7 +14709,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
skl_wm_get_hw_state(dev);
} else if (HAS_PCH_SPLIT(dev_priv)) {
ilk_wm_get_hw_state(dev);
-   }
+   } else if (INTEL_GEN(dev_priv) <= 3 && !IS_PINEVIEW(dev_priv))
+   i9xx_wm_get_hw_state(dev);
 
for_each_intel_crtc(dev, crtc) {
u64 put_domains;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f91de9cb9697..d53d34756048 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -547,6 +547,15 @@ struct g4x_wm_state {
bool fbc_en;
 };
 
+struct i9xx_wm_state {
+   uint16_t plane_wm;
+   bool cxsr;
+
+   struct {
+   uint16_t plane;
+   } sr;
+};
+
 struct intel_crtc_wm_state {
union {
struct {
@@ -591,6 +600,9 @@ struct intel_crtc_wm_state {
/* optimal watermarks */
struct g4x_wm_state optimal;
} g4x;
+   struct {
+   struct i9xx_wm_state optimal;
+   } i9xx;
};
 
/*
@@ -823,6 +835,7 @@ struct intel_crtc {
struct intel_pipe_wm ilk;
struct vlv_wm_state vlv;
struct g4x_wm_state g4x;
+   struct i9xx_wm_state i9xx;
} active;
} wm;
 
@@ -1845,6 +1858,7 @@ void gen6_rps_boost(struct drm_i915_gem_request *rq,
struct intel_rps_client *rps);
 void intel_queue_rps_boost_for_request(struct drm_i915_gem_request *req);
 void g4x_wm_get_hw_state(struct drm_device *dev);
+void i9xx_wm_get_hw_state(struct drm_device *dev);
 void vlv_wm_get_hw_state(struct drm_device *dev);
 void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6e393b217450..807c9a730020 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2224,89 +2224,154 @@ static void i965_update_wm(struct intel_crtc 
*unused_crtc)
 
 #undef FW_WM
 
-static void i9xx_update_wm(struct intel_crtc *unused_crtc)
+static const struct intel_watermark_params *i9xx_get_wm_info(struct 
drm_i915_private *dev_priv,
+struct intel_crtc 
*crtc)
 {
-   struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
-   const struct intel_watermark_params *wm_info;
-   uint32_t fwater_lo;
-   uint32_t fwater_hi;
-   int cwm, srwm = 1;
-   int fifo_size;
-   int planea_wm, planeb_wm;
-   struct intel_crtc *crtc, *enabled = NULL;
+   struct intel_plane *plane = to_intel_plane(crtc->base.primary);
 
if (IS_I945GM(dev_priv))
-   wm_info = _wm_info;
+   return _wm_info;
else if (!IS_GEN2(dev_priv))
-   wm_info = _wm_info;
+   return _wm_info;
+   else if (plane->plane == PLANE_A)
+   return _a_wm_info;
else
-   wm_info = _a_wm_info;
+   return _bc_wm_info;
+}
 
-   fifo_size = dev_priv->display.get_fifo_size(dev_priv, 0);
-   crtc = intel_get_crtc_for_plane(dev_priv, 0);
-   if (intel_crtc_active(crtc)) {
+static int i9xx_compute_pipe_wm(struct intel_crtc_state *crtc_state)
+{
+   struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+   struct intel_atomic_state *state =
+   to_intel_atomic_state(crtc_state->base.state);
+   struct i9xx_wm_state *wm_state = _state->wm.i9xx.optimal;
+   struct intel_plane *plane = to_intel_plane(crtc->base.primary);
+   const struct drm_plane_state *plane_state = NULL;
+   int fifo_size;
+   const struct intel_watermark_params *wm_info;
+
+   fifo_size = dev_priv->display.get_fifo_size(dev_priv, plane->plane);
+
+   wm_info = i9xx_get_wm_info(dev_priv, crtc);
+
+   wm_state->cxsr = false;
+   memset(_state->sr, 0, sizeof(wm_state->sr));
+
+   if (crtc_state->base.plane_mask & BIT(drm_plane_index(>base)))
+