Op 23-06-16 om 14:36 schreef Chi Ding:
> From: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>
> Rename vlv_compute_wm to vlv_compute_pipe_wm to compute optimal watermark
> Add vlv_compute_intermediate_wm to computer intermediate watermark
> Add vlv_initial_watermarks to write intermediate watermark into hardware
> Add vlv_optimize_watermarks to write optimal watermark into hardware
> Change valleyview_crtc_enable to call .initial_watermarks handler
>
> This patch adds the handlers for two-level atomic watermark for VLV/CHV.
> It makes use of the optimal and intermediate watermark fields added in
> the previous commits to calculate the optimal and intermediate state.
> It sets the intermediate watermark which is the safer value of the
> currently active and the optimal watermark pre-vblank. Then it sets the
> optimal watermark after-vblank.
>
> v2:
> - use macro drm_atomic_crtc_state_for_each_plane_state
> - remove redundant debug statements in vlv_pipe_set_fifo_size
>
> v3:
> - use macro drm_atomic_crtc_state_for_each_plane_state to simplify the code
> - check !new_state->active || modeset in computing intermediate watermark
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Chi Ding <chix.ding@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 8 +-
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pm.c | 150 +++++++++++++++++++++++++++--------
> 3 files changed, 121 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1141b86..1ceda24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4602,8 +4602,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>
> intel_frontbuffer_flip(dev, pipe_config->fb_bits);
>
> - crtc->wm.cxsr_allowed = true;
> -
> if (pipe_config->update_wm_post && pipe_config->base.active)
> intel_update_watermarks(&crtc->base);
>
> @@ -4649,8 +4647,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> }
>
> if (pipe_config->disable_cxsr && HAS_GMCH_DISPLAY(dev)) {
> - crtc->wm.cxsr_allowed = false;
> -
> /*
> * Vblank time updates from the shadow to live plane control register
> * are blocked if the memory self-refresh mode is active at that
> @@ -6180,7 +6176,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
> intel_color_load_luts(&pipe_config->base);
>
> - intel_update_watermarks(crtc);
> + dev_priv->display.initial_watermarks(pipe_config);
> intel_enable_pipe(intel_crtc);
>
> assert_vblank_disabled(crtc);
> @@ -14591,8 +14587,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> intel_crtc->cursor_cntl = ~0;
> intel_crtc->cursor_size = ~0;
>
> - intel_crtc->wm.cxsr_allowed = true;
> -
> BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8e77adb..30a6304 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -443,6 +443,7 @@ struct intel_crtc_wm_state {
> } ilk;
>
> struct {
> + struct vlv_wm_state intermediate;
> struct vlv_wm_state optimal;
> } vlv;
>
> @@ -683,8 +684,6 @@ struct intel_crtc {
> struct vlv_wm_state vlv;
> } active;
>
> - /* allow CxSR on this pipe */
> - bool cxsr_allowed;
> } wm;
>
> int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cc43c1e..9a5e6e9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,6 +31,7 @@
> #include "intel_drv.h"
> #include "../../../platform/x86/intel_ips.h"
> #include <linux/module.h>
> +#include <drm/drm_atomic_helper.h>
>
> /**
> * DOC: RC6
> @@ -989,6 +990,7 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> clock = cstate->base.adjusted_mode.crtc_clock;
> htotal = cstate->base.adjusted_mode.crtc_htotal;
> width = cstate->pipe_src_w;
> +
> if (WARN_ON(htotal == 0))
> htotal = 1;
>
> @@ -1011,27 +1013,28 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> static void vlv_compute_fifo(struct intel_crtc_state *cstate,
> struct vlv_wm_state *wm_state)
> {
> - struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> - struct drm_device *dev = crtc->base.dev;
> - struct intel_plane *plane;
> unsigned int total_rate = 0;
> const int fifo_size = 512 - 1;
> int fifo_extra, fifo_left = fifo_size;
> int rate[I915_MAX_PLANES] = {};
> int i;
> + const struct drm_plane_state *pstate;
> + struct drm_plane *plane;
>
> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
> struct intel_plane_state *state =
> - to_intel_plane_state(plane->base.state);
> + to_intel_plane_state(pstate);
> + struct intel_plane *intel_plane =
> + to_intel_plane(plane);
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> continue;
>
> if (state->visible) {
> wm_state->num_active_planes++;
> - rate[wm_plane_id(plane)] =
> + rate[wm_plane_id(intel_plane)] =
> drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> - total_rate += rate[wm_plane_id(plane)];
> + total_rate += rate[wm_plane_id(intel_plane)];
> }
> }
>
> @@ -1114,18 +1117,19 @@ static void vlv_invert_wms(struct intel_crtc *crtc,
> }
> }
>
> -static int vlv_compute_wm(struct intel_crtc_state *cstate)
> +static int vlv_compute_pipe_wm(struct intel_crtc_state *cstate)
> {
> struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> struct drm_device *dev = crtc->base.dev;
> struct vlv_wm_state *wm_state = &cstate->wm.vlv.optimal;
> - struct intel_plane *plane;
> + struct drm_plane *plane;
> int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> int level;
> + const struct drm_plane_state *pstate;
>
> memset(wm_state, 0, sizeof(*wm_state));
>
> - wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
> + wm_state->cxsr = crtc->pipe != PIPE_C;
> wm_state->num_levels = to_i915(dev)->wm.max_level + 1;
>
> wm_state->num_active_planes = 0;
> @@ -1142,31 +1146,33 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
> }
> }
>
> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
> struct intel_plane_state *state =
> - to_intel_plane_state(plane->base.state);
> + to_intel_plane_state(pstate);
> + struct intel_plane *intel_plane =
> + to_intel_plane(plane);
>
> if (!state->visible)
> continue;
>
> /* normal watermarks */
> for (level = 0; level < wm_state->num_levels; level++) {
> - int wm = vlv_compute_wm_level(plane, cstate, state, level);
> - int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> + int wm = vlv_compute_wm_level(intel_plane, cstate, state, level);
> + int max_wm = intel_plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>
> if (level == 0 && wm > max_wm) {
> DRM_DEBUG_KMS("Requested display configuration "
> "exceeds system watermark limitations\n");
> DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
> - crtc->pipe,
> - drm_plane_index(&plane->base), wm, max_wm);
> + crtc->pipe,
> + drm_plane_index(&intel_plane->base), wm, max_wm);
> return -EINVAL;
> }
>
> - if (wm > wm_state->fifo_size[wm_plane_id(plane)])
> + if (wm > wm_state->fifo_size[wm_plane_id(intel_plane)])
> break;
>
> - switch (plane->base.type) {
> + switch (intel_plane->base.type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = wm;
> @@ -1175,7 +1181,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
> wm_state->wm[level].primary = wm;
> break;
> case DRM_PLANE_TYPE_OVERLAY:
> - sprite = plane->plane;
> + sprite = intel_plane->plane;
> wm_state->wm[level].sprite[sprite] = wm;
> break;
> }
> @@ -1187,7 +1193,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
> continue;
>
> /* maxfifo watermarks */
> - switch (plane->base.type) {
> + switch (intel_plane->base.type) {
> int sprite, level;
> case DRM_PLANE_TYPE_CURSOR:
> for (level = 0; level < wm_state->num_levels; level++)
> @@ -1201,7 +1207,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
> wm_state->wm[level].primary);
> break;
> case DRM_PLANE_TYPE_OVERLAY:
> - sprite = plane->plane;
> + sprite = intel_plane->plane;
> for (level = 0; level < wm_state->num_levels; level++)
> wm_state->sr[level].plane =
> min(wm_state->sr[level].plane,
> @@ -1221,6 +1227,65 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
> return 0;
> }
>
> +
> +static int vlv_compute_intermediate_wm(struct drm_device *dev,
> + struct intel_crtc *intel_crtc,
> + struct intel_crtc_state *newstate)
> +{
> + struct vlv_wm_state *a = &newstate->wm.vlv.intermediate;
> + struct vlv_wm_state *b = &intel_crtc->wm.active.vlv;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int level, max_level = dev_priv->wm.max_level;
> +
> + /*
> + * Start with the final, target watermarks, then combine with the
> + * currently active watermarks to get values that are safe both before
> + * and after the vblank.
> + */
> + *a = newstate->wm.vlv.optimal;
> +
> + if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> + return 0;
> +
> + a->num_levels = min(a->num_levels, b->num_levels);
> + a->cxsr &= b->cxsr;
I think we lost a check for disable_cxsr here. This might also be required during modeset
since we don't enable planes until after the modeset, initial watermarks during modeset
probably requires that cxsr is disabled until after optimal watermarks.
> + for (level = 0; level < a->num_levels; level++) {
> + struct vlv_pipe_wm *a_wm = &a->wm[level];
> + const struct vlv_pipe_wm *b_wm = &b->wm[level];
> + struct vlv_sr_wm *a_sr = &a->sr[level];
> + const struct vlv_sr_wm *b_sr = &b->sr[level];
> +
> + a_wm->primary = min(a_wm->primary, b_wm->primary);
> + a_wm->sprite[0] = min(a_wm->sprite[0], b_wm->sprite[0]);
> + a_wm->sprite[1] = min(a_wm->sprite[1], b_wm->sprite[1]);
> + a_wm->cursor = min(a_wm->cursor, b_wm->cursor);
Again, why min?
Don't make me write an atomic testcase that does this:
primary: set fb1,
sprite0: set null fb
commit
primary: set null fb
sprite0: set fb1
commit
In fact I will do so, it should be easy to catch FIFO underruns like that..
> + if (a->cxsr) {
> + a_sr->plane = min(a_sr->plane, b_sr->plane);
> + a_sr->cursor = min(a_sr->cursor, b_sr->cursor);
> + } else {
> + a_sr->plane = b_sr->plane;
> + a_sr->cursor = b_sr->cursor;
> + }
^The else seems redundant. The values are unused when cxsr is disabled.
Rest seems ok.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
