Re: [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes

On Thu, Sep 18, 2014 at 04:43:12PM -0300, Gustavo Padovan wrote:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

Fold intel_pipe_set_base() in the update primary plane path merging pieces of code that are common to both paths.

Basically the the pin/unpin procedures are the same for both paths and some checks can also be shared (some of the were moved to the check() stage)

v2: take Ville's comments:

• remove unnecessary plane check
• move mutex lock to inside the conditional
• make the pin fail message a debug one
• add a fixme for the fastboot hack
• call intel_frontbuffer_flip() after FBC update

v3: take more Ville's comments:

• fold update code under if (intel_crtc->active), and do the

visible/!visible split inside.

• check ret inside the same conditional we assign it

v4: don't use intel_enable_primary_hw_plane(), the primary_enabled check inside will break page flips

Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

---

drivers/gpu/drm/i915/intel_display.c | 141 +++++++++++++++++++++-------------- 1 file changed, 84 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5b05ddb..1fd9b70 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11792,12 +11792,23 @@ intel_check_primary_plane(struct drm_plane *plane, struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip;

• int ret;

• return drm_plane_helper_check_update(plane, crtc, fb,

• ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, false, true, &state->visible);
• if (ret)

• return ret;
  

• /* no fb bound */
• if (state->visible && !fb) {

• DRM_ERROR("No FB bound\n");
  

• return -EINVAL;
  

• }
• return 0;

}

static int @@ -11809,6 +11820,8 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• enum pipe pipe = intel_crtc->pipe;
• struct drm_framebuffer *old_fb = plane->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); struct intel_plane *intel_plane = to_intel_plane(plane);

@@ -11817,67 +11830,28 @@ intel_commit_primary_plane(struct drm_plane *plane,

intel_crtc_wait_for_pending_flips(crtc);

• /*

• * If clipping results in a non-visible primary plane, we'll disable
  

• * the primary plane.  Note that this is a bit different than what
  

• * happens if userspace explicitly disables the plane by passing fb=0
  

• * because plane->fb still gets set and pinned.
  

• */
  

• if (!state->visible) {

• mutex_lock(&dev->struct_mutex);
  

• /*
  

•  * Try to pin the new fb first so that we can bail out if we
  

•  * fail.
  

•  */
  

• if (plane->fb != fb) {
  

• 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
  

• 	if (ret) {
  

• 		mutex_unlock(&dev->struct_mutex);
  

• 		return ret;
  

• 	}
  

• }
  

• i915_gem_track_fb(old_obj, obj,
  

• 		  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
  

• if (intel_crtc->primary_enabled)
  

• 	intel_disable_primary_hw_plane(plane, crtc);
  

• if (plane->fb != fb)
  

• 	if (plane->fb)
  

• 		intel_unpin_fb_obj(old_obj);
  

• if (intel_crtc_has_pending_flip(crtc)) {

• DRM_ERROR("pipe is still busy with an old pageflip\n");
  

• return -EBUSY;
  

• }

• if (plane->fb != fb) {

• mutex_lock(&dev->struct_mutex);
  

• ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
  

• if (ret == 0)
  

• 	i915_gem_track_fb(old_obj, obj,
  

• 			  INTEL_FRONTBUFFER_PRIMARY(pipe));
  

  mutex_unlock(&dev->struct_mutex);

• } else {

• if (intel_crtc && intel_crtc->active &&
  

•     intel_crtc->primary_enabled) {
  

• 	/*
  

• 	 * FBC does not work on some platforms for rotated
  

• 	 * planes, so disable it when rotation is not 0 and
  

• 	 * update it when rotation is set back to 0.
  

• 	 *
  

• 	 * FIXME: This is redundant with the fbc update done in
  

• 	 * the primary plane enable function except that that
  

• 	 * one is done too late. We eventually need to unify
  

• 	 * this.
  

• 	 */
  

• 	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
  

• 	    dev_priv->fbc.plane == intel_crtc->plane &&
  

• 	    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
  

• 		intel_disable_fbc(dev);
  

• 	}
  

• }
  

• ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
  

• if (ret)
  

• if (ret != 0) {
  

• 	DRM_DEBUG_KMS("pin & fence failed\n");
  return ret;
  

• if (!intel_crtc->primary_enabled)
  

• 	intel_enable_primary_hw_plane(plane, crtc);
  

• }
  

  }

• crtc->primary->fb = fb;

• crtc->x = src->x1;

• crtc->y = src->y1;

• intel_plane->crtc_x = state->orig_dst.x1; intel_plane->crtc_y = state->orig_dst.y1; intel_plane->crtc_w = drm_rect_width(&state->orig_dst);

@@ -11888,6 +11862,59 @@ intel_commit_primary_plane(struct drm_plane *plane, intel_plane->src_h = drm_rect_height(&state->orig_src); intel_plane->obj = obj;

• if (intel_crtc->active) {

• /*
  

•  * FBC does not work on some platforms for rotated
  

•  * planes, so disable it when rotation is not 0 and
  

•  * update it when rotation is set back to 0.
  

•  *
  

•  * FIXME: This is redundant with the fbc update done in
  

•  * the primary plane enable function except that that
  

•  * one is done too late. We eventually need to unify
  

•  * this.
  

•  */
  

• if (intel_crtc->primary_enabled &&
  

•     INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
  

•     dev_priv->fbc.plane == intel_crtc->plane &&
  

•     intel_plane->rotation != BIT(DRM_ROTATE_0)) {
  

• 	intel_disable_fbc(dev);
  

• }
  

• if (state->visible) {
  

• 	/* FIXME: kill this fastboot hack */
  

• 	intel_update_pipe_size(intel_crtc);
  

Need to set primary_enabled=true here.

• 	dev_priv->display.update_primary_plane(crtc, plane->fb,
  

• 			crtc->x, crtc->y);
  

And you need to duplicate the BDW wait_for_vblank hack from intel_enable_primary_hw_plane() here. But that wait should only be done if the plane was previosuly disabled.

• } else {
  

• 	/*
  

• 	 * If clipping results in a non-visible primary plane,
  

• 	 * we'll disable the primary plane.  Note that this is
  

• 	 * a bit different than what happens if userspace
  

• 	 * explicitly disables the plane by passing fb=0
  

• 	 * because plane->fb still gets set and pinned.
  

• 	 */
  

• 	intel_disable_primary_hw_plane(plane, crtc);
  

• }
  

• intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
  

• mutex_lock(&dev->struct_mutex);
  

• intel_update_fbc(dev);
  

• mutex_unlock(&dev->struct_mutex);
  

• }
• if (old_fb) {

Still could unify the old_fb!=fb checks with this one.

if (old_fb && old_fb != fb) {

• if (intel_crtc->active && old_fb != fb)
  

• 	intel_wait_for_vblank(dev, intel_crtc->pipe);
  

• if (old_fb != fb) {
  

• 	mutex_lock(&dev->struct_mutex);
  

• 	intel_unpin_fb_obj(old_obj);
  

• 	mutex_unlock(&dev->struct_mutex);
  

• }
  

• }
• return 0;

}

-- 1.9.3

---

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