On Thu, Aug 27, 2015 at 12:08:30PM +0200, Maarten Lankhorst wrote:
> This appears to make all the cursor tests really slow because of the many calls to skl_update_wm
> when the cursor plane visibility is changed. It performs does 3 vblanks each time it's called, and
> it's probably called more than once on each update.
On all other platforms wm updates (right now at least) don't do any vblank
waits, which means changing cursors actually _does_ cause tons of
underruns. Can we perhaps add a hack in skl to do the same (maybe just for
cursors) so that we can get this in? There's lots other work that really
wants proper universal planes ...
-Daniel
>
> Smarter watermark updates will hopefully fix this..
>
> This patch is tested with Xorg and kms_cursor_crc, and is just a RFC. :)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 05523491ad6f..bf5f814f72ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -263,11 +263,11 @@ struct i915_hotplug {
> for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
> #define for_each_plane(__dev_priv, __pipe, __p) \
> for ((__p) = 0; \
> - (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1; \
> + (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1 + IS_GEN9(__dev_priv); \
> (__p)++)
> #define for_each_sprite(__dev_priv, __p, __s) \
> for ((__s) = 0; \
> - (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)]; \
> + (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)] + IS_GEN9(__dev_priv); \
> (__s)++)
>
> #define for_each_crtc(dev, crtc) \
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92e3f074ae30..307d1374daa5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1402,7 +1402,7 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
>
> if (INTEL_INFO(dev)->gen >= 9) {
> for_each_sprite(dev_priv, pipe, sprite) {
> - val = I915_READ(PLANE_CTL(pipe, sprite));
> + val = I915_READ(PLANE_CTL(pipe, sprite + 1));
> I915_STATE_WARN(val & PLANE_CTL_ENABLE,
> "plane %d assertion failure, should be off on pipe %c but is still active\n",
> sprite, pipe_name(pipe));
> @@ -11563,12 +11563,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> bool mode_changed = needs_modeset(crtc_state);
> bool was_crtc_enabled = crtc->state->active;
> bool is_crtc_enabled = crtc_state->active;
> -
> + enum drm_plane_type plane_type = plane->type;
> bool turn_off, turn_on, visible, was_visible;
> struct drm_framebuffer *fb = plane_state->fb;
>
> + if (IS_GEN9(dev) && plane->type == DRM_PLANE_TYPE_CURSOR)
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> - plane->type != DRM_PLANE_TYPE_CURSOR) {
> + plane_type != DRM_PLANE_TYPE_CURSOR) {
> ret = skl_update_scaler_plane(
> to_intel_crtc_state(crtc_state),
> to_intel_plane_state(plane_state));
> @@ -11601,7 +11604,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> if (turn_on) {
> intel_crtc->atomic.update_wm_pre = true;
> /* must disable cxsr around plane enable/disable */
> - if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> + if (plane_type != DRM_PLANE_TYPE_CURSOR) {
> intel_crtc->atomic.disable_cxsr = true;
> /* to potentially re-enable cxsr */
> intel_crtc->atomic.wait_vblank = true;
> @@ -11610,7 +11613,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> } else if (turn_off) {
> intel_crtc->atomic.update_wm_post = true;
> /* must disable cxsr around plane enable/disable */
> - if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> + if (plane_type != DRM_PLANE_TYPE_CURSOR) {
> if (is_crtc_enabled)
> intel_crtc->atomic.wait_vblank = true;
> intel_crtc->atomic.disable_cxsr = true;
> @@ -11623,7 +11626,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> intel_crtc->atomic.fb_bits |=
> to_intel_plane(plane)->frontbuffer_bit;
>
> - switch (plane->type) {
> + switch (plane_type) {
> case DRM_PLANE_TYPE_PRIMARY:
> intel_crtc->atomic.wait_for_flips = true;
> intel_crtc->atomic.pre_disable_primary = turn_off;
> @@ -13770,7 +13773,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> struct intel_crtc_state *crtc_state = NULL;
> struct drm_plane *primary = NULL;
> struct drm_plane *cursor = NULL;
> - int i, ret;
> + int i, ret, sprite;
>
> intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> if (intel_crtc == NULL)
> @@ -13797,9 +13800,31 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> if (!primary)
> goto fail;
>
> - cursor = intel_cursor_plane_create(dev, pipe);
> - if (!cursor)
> - goto fail;
> + if (!IS_GEN9(dev)) {
> + cursor = intel_cursor_plane_create(dev, pipe);
> + if (!cursor)
> + goto fail;
> + }
> +
> + for_each_sprite(dev_priv, pipe, sprite) {
> + enum drm_plane_type plane_type;
> + struct drm_plane *plane;
> +
> + if (sprite < INTEL_INFO(dev_priv)->num_sprites[pipe])
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> + else
> + plane_type = DRM_PLANE_TYPE_CURSOR;
> +
> + plane = intel_plane_init(dev, pipe, sprite, plane_type);
> + if (IS_ERR(plane)) {
> + DRM_DEBUG_KMS("pipe %c sprite %c init failed: %ld\n",
> + pipe_name(pipe), sprite_name(pipe, sprite), PTR_ERR(plane));
> + goto fail;
> + }
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR)
> + cursor = plane;
> + }
>
> ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> cursor, &intel_crtc_funcs);
> @@ -14724,7 +14749,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
> void intel_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - int sprite, ret;
> enum pipe pipe;
> struct intel_crtc *crtc;
>
> @@ -14778,15 +14802,8 @@ void intel_modeset_init(struct drm_device *dev)
> INTEL_INFO(dev)->num_pipes,
> INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
>
> - for_each_pipe(dev_priv, pipe) {
> + for_each_pipe(dev_priv, pipe)
> intel_crtc_init(dev, pipe);
> - for_each_sprite(dev_priv, pipe, sprite) {
> - ret = intel_plane_init(dev, pipe, sprite);
> - if (ret)
> - DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> - pipe_name(pipe), sprite_name(pipe, sprite), ret);
> - }
> - }
>
> intel_init_dpio(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3908059fc42f..df3bca6e4031 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -890,11 +890,12 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
>
> /*
> * Returns the number of planes for this pipe, ie the number of sprites + 1
> - * (primary plane). This doesn't count the cursor plane then.
> + * (primary plane). This doesn't count the cursor plane except on GEN9,
> + * which has a universal plane masked as cursor plane.
> */
> static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> {
> - return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
> + return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1 + IS_GEN9(crtc->base.dev);
> }
>
> /* intel_fifo_underrun.c */
> @@ -1388,7 +1389,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>
>
> /* intel_sprite.c */
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
> + int plane, enum drm_plane_type plane_type);
> int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void intel_pipe_update_start(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fff0c22682ee..b6ea345e2f1b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2970,7 +2970,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum pipe pipe = intel_crtc->pipe;
> struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
> - uint16_t alloc_size, start, cursor_blocks;
> + uint16_t alloc_size, start;
> uint16_t minimum[I915_MAX_PLANES];
> uint16_t y_minimum[I915_MAX_PLANES];
> unsigned int total_data_rate;
> @@ -2984,12 +2984,16 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
> return;
> }
>
> - cursor_blocks = skl_cursor_allocation(config);
> - ddb->cursor[pipe].start = alloc->end - cursor_blocks;
> - ddb->cursor[pipe].end = alloc->end;
> + if (!IS_GEN9(dev)) {
> + uint16_t cursor_blocks;
>
> - alloc_size -= cursor_blocks;
> - alloc->end -= cursor_blocks;
> + cursor_blocks = skl_cursor_allocation(config);
> + ddb->cursor[pipe].start = alloc->end - cursor_blocks;
> + ddb->cursor[pipe].end = alloc->end;
> +
> + alloc_size -= cursor_blocks;
> + alloc->end -= cursor_blocks;
> + }
>
> /* 1. Allocate the mininum required blocks for each active plane */
> for_each_plane(dev_priv, pipe, plane) {
> @@ -3186,7 +3190,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>
> fb = crtc->cursor->state->fb;
> p->cursor.y_bytes_per_pixel = 0;
> - if (fb) {
> + if (!IS_GEN9(dev) && fb) {
> p->cursor.enabled = true;
> p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> @@ -3202,8 +3206,11 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
> list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> struct intel_plane *intel_plane = to_intel_plane(plane);
>
> - if (intel_plane->pipe == pipe &&
> - plane->type == DRM_PLANE_TYPE_OVERLAY)
> + if (intel_plane->pipe != pipe)
> + continue;
> +
> + if (plane->type == DRM_PLANE_TYPE_OVERLAY ||
> + (IS_GEN9(dev) && plane->type == DRM_PLANE_TYPE_CURSOR))
> p->plane[i++] = intel_plane->wm;
> }
> }
> @@ -3390,11 +3397,13 @@ static void skl_compute_wm_results(struct drm_device *dev,
>
> temp = 0;
>
> - temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
> - temp |= p_wm->wm[level].cursor_res_b;
> + if (!IS_GEN9(dev)) {
> + temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
> + temp |= p_wm->wm[level].cursor_res_b;
>
> - if (p_wm->wm[level].cursor_en)
> - temp |= PLANE_WM_EN;
> + if (p_wm->wm[level].cursor_en)
> + temp |= PLANE_WM_EN;
> + }
>
> r->cursor[pipe][level] = temp;
>
> @@ -3412,10 +3421,12 @@ static void skl_compute_wm_results(struct drm_device *dev,
> }
>
> temp = 0;
> - temp |= p_wm->trans_wm.cursor_res_l << PLANE_WM_LINES_SHIFT;
> - temp |= p_wm->trans_wm.cursor_res_b;
> - if (p_wm->trans_wm.cursor_en)
> - temp |= PLANE_WM_EN;
> + if (!IS_GEN9(dev)) {
> + temp |= p_wm->trans_wm.cursor_res_l << PLANE_WM_LINES_SHIFT;
> + temp |= p_wm->trans_wm.cursor_res_b;
> + if (p_wm->trans_wm.cursor_en)
> + temp |= PLANE_WM_EN;
> + }
>
> r->cursor_trans[pipe] = temp;
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9d8af2f8a875..596fa4319681 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -961,7 +961,8 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> return -EINVAL;
>
> plane = drm_plane_find(dev, set->plane_id);
> - if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY)
> + if (!plane || plane->type == DRM_PLANE_TYPE_PRIMARY ||
> + (!IS_GEN9(dev) && plane->type == DRM_PLANE_TYPE_CURSOR))
> return -ENOENT;
>
> drm_modeset_acquire_init(&ctx, 0);
> @@ -1040,8 +1041,8 @@ static uint32_t skl_plane_formats[] = {
> DRM_FORMAT_VYUY,
> };
>
> -int
> -intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> +struct drm_plane *
> +intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane, enum drm_plane_type plane_type)
> {
> struct intel_plane *intel_plane;
> struct intel_plane_state *state;
> @@ -1051,16 +1052,16 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> int ret;
>
> if (INTEL_INFO(dev)->gen < 5)
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
>
> intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
> if (!intel_plane)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> state = intel_create_plane_state(&intel_plane->base);
> if (!state) {
> kfree(intel_plane);
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
> }
> intel_plane->base.state = &state->base;
>
> @@ -1116,7 +1117,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> break;
> default:
> kfree(intel_plane);
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> intel_plane->pipe = pipe;
> @@ -1128,7 +1129,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
> &intel_plane_funcs,
> plane_formats, num_plane_formats,
> - DRM_PLANE_TYPE_OVERLAY);
> + plane_type);
> if (ret) {
> kfree(intel_plane);
> goto out;
> @@ -1138,6 +1139,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>
> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>
> + return &intel_plane->base;
> +
> out:
> - return ret;
> + return ERR_PTR(ret);
> }
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
