[Intel-gfx] [PATCH 1/7] drm/i915: Calculate gen3- watermarks semi-atomically.
Mahesh Kumar
mahesh1.kumar at intel.com
Tue Aug 8 12:51:36 UTC 2017
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 <maarten.lankhorst at linux.intel.com>
> ---
> 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 = &i945_wm_info;
> + return &i945_wm_info;
> else if (!IS_GEN2(dev_priv))
> - wm_info = &i915_wm_info;
> + return &i915_wm_info;
> + else if (plane->plane == PLANE_A)
> + return &i830_a_wm_info;
> else
> - wm_info = &i830_a_wm_info;
> + return &i830_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 = &crtc_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(&wm_state->sr, 0, sizeof(wm_state->sr));
> +
> + if (crtc_state->base.plane_mask & BIT(drm_plane_index(&plane->base)))
> + plane_state = __drm_atomic_get_current_plane_state(&state->base, &plane->base);
> +
> + 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.
> + 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 =
> - &crtc->config->base.adjusted_mode;
> - const struct drm_framebuffer *fb =
> - crtc->base.primary->state->fb;
> - int cpp;
> + &crtc_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(&crtc->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).
> +
> + wm_state->plane_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
> + wm_info, fifo_size, cpp,
> + pessimal_latency_ns);
> +
> + DRM_DEBUG_KMS("FIFO watermarks - plane watermarks: %d\n", wm_state->plane_wm);
> +
> + if (IS_I915GM(dev_priv) && i915_gem_object_is_tiled(intel_fb_obj(fb)))
> + may_cxsr = false;
> +
> + if (may_cxsr) {
> + static const int sr_latency_ns = 6000;
> + unsigned long entries;
> +
> + entries = intel_wm_method2(adjusted_mode->crtc_clock,
> + adjusted_mode->crtc_htotal,
> + crtc_state->pipe_src_w, cpp,
> + sr_latency_ns / 100);
> + entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
> +
> + 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 ?
> + 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.
> + }
> +
> + DRM_DEBUG_KMS("FIFO watermarks - plane watermarks: %d, can cxsr: %s, SR size: %d\n",
> + wm_state->plane_wm, yesno(wm_state->cxsr), wm_state->sr.plane);
> }
>
> - if (IS_GEN2(dev_priv))
> - wm_info = &i830_bc_wm_info;
> + return 0;
> +}
>
> - fifo_size = dev_priv->display.get_fifo_size(dev_priv, 1);
> - crtc = intel_get_crtc_for_plane(dev_priv, 1);
> - if (intel_crtc_active(crtc)) {
> - const struct drm_display_mode *adjusted_mode =
> - &crtc->config->base.adjusted_mode;
> - const struct drm_framebuffer *fb =
> - crtc->base.primary->state->fb;
> - int cpp;
> +void i9xx_wm_get_hw_state(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *crtc;
> + uint32_t fwater_lo, planea_wm, planeb_wm;
> +
> + fwater_lo = I915_READ(FW_BLC);
> +
> + planea_wm = fwater_lo & 0x3f;
> + planeb_wm = (fwater_lo >> 16) & 0x3f;
> +
> + for_each_intel_crtc(dev, crtc) {
> + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state);
> + struct i9xx_wm_state *wm_state = &cstate->wm.i9xx.optimal;
> + struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> +
> + memset(&wm_state->sr, 0, sizeof(wm_state->sr));
> + wm_state->cxsr = false;
>
> - 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
> + 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.
> +
> + crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
> +
> + crtc = intel_get_crtc_for_plane(dev_priv, 0);
> + planea_wm = crtc->wm.active.i9xx.plane_wm;
> + if (intel_crtc_active(crtc))
> + enabled = crtc;
> +
> + crtc = intel_get_crtc_for_plane(dev_priv, 1);
> + if (intel_crtc_active(crtc)) {
> if (enabled == NULL)
> enabled = crtc;
> else
> enabled = NULL;
> - } else {
> - planeb_wm = fifo_size - wm_info->guard_size;
> - if (planeb_wm > (long)wm_info->max_wm)
> - planeb_wm = wm_info->max_wm;
> }
> + planeb_wm = crtc->wm.active.i9xx.plane_wm;
>
> DRM_DEBUG_KMS("FIFO watermarks - A: %d, B: %d\n", planea_wm, planeb_wm);
>
> - if (IS_I915GM(dev_priv) && enabled) {
> - struct drm_i915_gem_object *obj;
> -
> - obj = intel_fb_obj(enabled->base.primary->state->fb);
> -
> - /* self-refresh seems busted with untiled */
> - if (!i915_gem_object_is_tiled(obj))
> - enabled = NULL;
> - }
> + if (!enabled->wm.active.i9xx.cxsr)
> + enabled = NULL;
>
> /*
> * Overlay gets an aggressive default since video jitter is bad.
> @@ -2317,31 +2382,8 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
> intel_set_memory_cxsr(dev_priv, false);
>
> /* Calc sr entries for one plane configs */
> - if (HAS_FW_BLC(dev_priv) && enabled) {
> - /* self-refresh has much higher latency */
> - static const int sr_latency_ns = 6000;
> - const struct drm_display_mode *adjusted_mode =
> - &enabled->config->base.adjusted_mode;
> - const struct drm_framebuffer *fb =
> - enabled->base.primary->state->fb;
> - int clock = adjusted_mode->crtc_clock;
> - int htotal = adjusted_mode->crtc_htotal;
> - int hdisplay = enabled->config->pipe_src_w;
> - int cpp;
> - int entries;
> -
> - if (IS_I915GM(dev_priv) || IS_I945GM(dev_priv))
> - cpp = 4;
> - else
> - cpp = fb->format->cpp[0];
> -
> - entries = intel_wm_method2(clock, htotal, hdisplay, cpp,
> - sr_latency_ns / 100);
> - entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
> - DRM_DEBUG_KMS("self-refresh entries: %d\n", entries);
> - srwm = wm_info->fifo_size - entries;
> - if (srwm < 0)
> - srwm = 1;
> + if (enabled) {
> + srwm = enabled->wm.active.i9xx.sr.plane;
>
> if (IS_I945G(dev_priv) || IS_I945GM(dev_priv))
> I915_WRITE(FW_BLC_SELF,
> @@ -2367,23 +2409,18 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
> intel_set_memory_cxsr(dev_priv, true);
> }
>
> -static void i845_update_wm(struct intel_crtc *unused_crtc)
> +static void i845_update_wm(struct intel_crtc *crtc)
> {
> - struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
> - struct intel_crtc *crtc;
> - const struct drm_display_mode *adjusted_mode;
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> uint32_t fwater_lo;
> int planea_wm;
>
> - crtc = single_enabled_crtc(dev_priv);
> - if (crtc == NULL)
earlier we were checking if enabled crtc's != 1 then return, but here
logic got changed.
Oh it seems ok, as platform calling i845 will have only one crtc.
-Mahesh
> + if (!intel_crtc_active(crtc))
> return;
>
> - adjusted_mode = &crtc->config->base.adjusted_mode;
> - planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
> - &i845_wm_info,
> - dev_priv->display.get_fifo_size(dev_priv, 0),
> - 4, pessimal_latency_ns);
> + crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
> + planea_wm = crtc->wm.active.i9xx.plane_wm;
> +
> fwater_lo = I915_READ(FW_BLC) & ~0xfff;
> fwater_lo |= (3<<8) | planea_wm;
>
> @@ -8813,9 +8850,13 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
> } else if (IS_GEN4(dev_priv)) {
> dev_priv->display.update_wm = i965_update_wm;
> } else if (IS_GEN3(dev_priv)) {
> + dev_priv->display.compute_pipe_wm = i9xx_compute_pipe_wm;
> dev_priv->display.update_wm = i9xx_update_wm;
> +
> dev_priv->display.get_fifo_size = i9xx_get_fifo_size;
> } else if (IS_GEN2(dev_priv)) {
> + dev_priv->display.compute_pipe_wm = i9xx_compute_pipe_wm;
> +
> if (INTEL_INFO(dev_priv)->num_pipes == 1) {
> dev_priv->display.update_wm = i845_update_wm;
> dev_priv->display.get_fifo_size = i845_get_fifo_size;
More information about the Intel-gfx
mailing list