Re: [PATCH v2 6/6] drm/i915: Add a cursor hack to allow converting legacy page flip to atomic, v3.

On Mon, Dec 12, 2016 at 11:34:55AM +0100, Maarten Lankhorst wrote:

Do something similar to vc4, only allow updating the cursor state in-place through a fastpath when the watermarks are unaffected. This will allow cursor movement to be smooth, but changing cursor size or showing/hiding cursor will still fall back so watermarks can be updated.

Only moving and changing fb is allowed.

Changes since v1:

• Set page flip to always_unused for trybot.
• Copy fence correctly, ignore plane_state->state, should be NULL.
• Check crtc_state for !active and modeset, go to slowpath if the case.

Changes since v2:

• Make error handling work correctly. (Matthew Auld)

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

Pulled in the entire pile through drm-misc, thanks. -Daniel

---

drivers/gpu/drm/i915/intel_atomic_plane.c | 47 +++++++---- drivers/gpu/drm/i915/intel_display.c | 132 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 163 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index dbe9fb41ae53..60d75ce8a989 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -103,36 +103,24 @@ intel_plane_destroy_state(struct drm_plane *plane, drm_atomic_helper_plane_destroy_state(plane, state); }

-static int intel_plane_atomic_check(struct drm_plane *plane,

• 		    struct drm_plane_state *state)
  

+int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

• 			struct intel_plane_state *intel_state)
  

{

• struct drm_plane *plane = intel_state->base.plane; struct drm_i915_private *dev_priv = to_i915(plane->dev);

• struct drm_crtc *crtc = state->crtc;
• struct intel_crtc *intel_crtc;
• struct intel_crtc_state *crtc_state;

• struct drm_plane_state *state = &intel_state->base; struct intel_plane *intel_plane = to_intel_plane(plane);

• struct intel_plane_state *intel_state = to_intel_plane_state(state);

• struct drm_crtc_state *drm_crtc_state; int ret;

• crtc = crtc ? crtc : plane->state->crtc;

• intel_crtc = to_intel_crtc(crtc);

• /*

  • Both crtc and plane->crtc could be NULL if we're updating a
  • property while the plane is disabled. We don't actually have
  • anything driver-specific we need to test in that case, so
  • just return success.

  */

• if (!crtc)

• if (!intel_state->base.crtc && !plane->state->crtc) return 0;

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

• return -EINVAL;
  

• crtc_state = to_intel_crtc_state(drm_crtc_state);
• /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ intel_state->clip.x1 = 0; intel_state->clip.y1 = 0;

@@ -184,6 +172,31 @@ static int intel_plane_atomic_check(struct drm_plane *plane, return intel_plane_atomic_calc_changes(&crtc_state->base, state); }

+static int intel_plane_atomic_check(struct drm_plane *plane,

• 		    struct drm_plane_state *state)
  

+{

• struct drm_crtc *crtc = state->crtc;
• struct drm_crtc_state *drm_crtc_state;
• crtc = crtc ? crtc : plane->state->crtc;
• /*

• * Both crtc and plane->crtc could be NULL if we're updating a
  

• * property while the plane is disabled.  We don't actually have
  

• * anything driver-specific we need to test in that case, so
  

• * just return success.
  

• */
  

• if (!crtc)

• return 0;
  

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

• return -EINVAL;
  

• return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),

• 				   to_intel_plane_state(state));
  

+}

static void intel_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9eaf1e5bdae9..5568ecac2edc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15028,6 +15028,136 @@ const struct drm_plane_funcs intel_plane_funcs = { .atomic_destroy_state = intel_plane_destroy_state, };

+static int +intel_legacy_cursor_update(struct drm_plane *plane,

• 	   struct drm_crtc *crtc,
  

• 	   struct drm_framebuffer *fb,
  

• 	   int crtc_x, int crtc_y,
  

• 	   unsigned int crtc_w, unsigned int crtc_h,
  

• 	   uint32_t src_x, uint32_t src_y,
  

• 	   uint32_t src_w, uint32_t src_h)
  

+{

• struct drm_i915_private *dev_priv = to_i915(crtc->dev);
• int ret;
• struct drm_plane_state *old_plane_state, *new_plane_state;
• struct intel_plane *intel_plane = to_intel_plane(plane);
• struct drm_framebuffer *old_fb;
• struct drm_crtc_state *crtc_state = crtc->state;
• /*

• * When crtc is inactive or there is a modeset pending,
  

• * wait for it to complete in the slowpath
  

• */
  

• if (!crtc_state->active || needs_modeset(crtc_state) ||

•    to_intel_crtc_state(crtc_state)->update_pipe)
  

• goto slow;
  

• old_plane_state = plane->state;
• /*

• * If any parameters change that may affect watermarks,
  

• * take the slowpath. Only changing fb or position should be
  

• * in the fastpath.
  

• */
  

• if (old_plane_state->crtc != crtc ||

•    old_plane_state->src_w != src_w ||
  

•    old_plane_state->src_h != src_h ||
  

•    old_plane_state->crtc_w != crtc_w ||
  

•    old_plane_state->crtc_h != crtc_h ||
  

•    !old_plane_state->visible ||
  

•    old_plane_state->fb->modifier != fb->modifier)
  

• goto slow;
  

• new_plane_state = intel_plane_duplicate_state(plane);
• if (!new_plane_state)

• return -ENOMEM;
  

• drm_atomic_set_fb_for_plane(new_plane_state, fb);
• new_plane_state->src_x = src_x;
• new_plane_state->src_y = src_y;
• new_plane_state->src_w = src_w;
• new_plane_state->src_h = src_h;
• new_plane_state->crtc_x = crtc_x;
• new_plane_state->crtc_y = crtc_y;
• new_plane_state->crtc_w = crtc_w;
• new_plane_state->crtc_h = crtc_h;
• ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),

• 				  to_intel_plane_state(new_plane_state));
  

• if (ret)

• goto out_free;
  

• /* Visibility changed, must take slowpath. */
• if (!new_plane_state->visible)

• goto slow_free;
  

• ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
• if (ret)

• goto out_free;
  

• if (INTEL_INFO(dev_priv)->cursor_needs_physical) {

• int align = IS_I830(dev_priv) ? 16 * 1024 : 256;
  

• ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to attach phys object\n");
  

• 	goto out_unlock;
  

• }
  

• } else {

• struct i915_vma *vma;
  

• vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
  

• if (IS_ERR(vma)) {
  

• 	DRM_DEBUG_KMS("failed to pin object\n");
  

• 	ret = PTR_ERR(vma);
  

• 	goto out_unlock;
  

• }
  

• }
• old_fb = old_plane_state->fb;
• i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),

• 	  intel_plane->frontbuffer_bit);
  

• /* Swap plane state */
• new_plane_state->fence = old_plane_state->fence;
• *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
• new_plane_state->fence = NULL;
• new_plane_state->fb = old_fb;
• intel_plane->update_plane(plane,

• 		  to_intel_crtc_state(crtc->state),
  

• 		  to_intel_plane_state(plane->state));
  

• intel_cleanup_plane_fb(plane, new_plane_state);

+out_unlock:

• mutex_unlock(&dev_priv->drm.struct_mutex);

+out_free:

• intel_plane_destroy_state(plane, new_plane_state);
• return ret;

+slow_free:

• intel_plane_destroy_state(plane, new_plane_state);

+slow:

• return drm_atomic_helper_update_plane(plane, crtc, fb,

• 			      crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			      src_x, src_y, src_w, src_h);
  

+}

+static const struct drm_plane_funcs intel_cursor_plane_funcs = {

• .update_plane = intel_legacy_cursor_update,
• .disable_plane = drm_atomic_helper_disable_plane,
• .destroy = intel_plane_destroy,
• .set_property = drm_atomic_helper_plane_set_property,
• .atomic_get_property = intel_plane_atomic_get_property,
• .atomic_set_property = intel_plane_atomic_set_property,
• .atomic_duplicate_state = intel_plane_duplicate_state,
• .atomic_destroy_state = intel_plane_destroy_state,

+};

static struct intel_plane * intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) { @@ -15274,7 +15404,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) cursor->disable_plane = intel_disable_cursor_plane;

ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,

• 		       0, &intel_plane_funcs,
  

• 		       0, &intel_cursor_plane_funcs,
  	       intel_cursor_formats,
  	       ARRAY_SIZE(intel_cursor_formats),
  	       DRM_PLANE_TYPE_CURSOR,
  

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8f4ddca0f521..8b6f39506d45 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1844,6 +1844,8 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; +int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,

• 			struct intel_plane_state *intel_state);
  
  

/* intel_color.c */ void intel_color_init(struct drm_crtc *crtc); -- 2.7.4

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel