On Wed, Sep 24, 2014 at 02:20:25PM -0300, Gustavo Padovan wrote:

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

Move check inside intel_crtc_cursor_set_obj() to intel_check_cursor_plane(), we only use it there so move them out to make the merge of intel_crtc_cursor_set_obj() into intel_check_cursor_plane() easier.

This is another step toward the atomic modesetting support and unification of plane operations such pin/unpin of fb objects on i915.

v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the code v3: take Ville's comment: kept only the restructuring changes, the rest of the code was moved to a separated patch since it is a bug fix (we weren't checking sizes when the fb was the same)

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

Looks like the commit messages for patches 2 and 3 got somehow swapped around. With that fixed patches 2 and 3 get: Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

---

drivers/gpu/drm/i915/intel_display.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 20be2ed..f91a5b0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11928,9 +11928,6 @@ intel_check_cursor_plane(struct drm_plane *plane, if (!obj) return 0;

• if (fb == crtc->cursor->fb)

• return 0;
  

• /* Check for which cursor types we support */ crtc_w = drm_rect_width(&state->orig_dst); crtc_h = drm_rect_height(&state->orig_dst);

@@ -11945,6 +11942,9 @@ intel_check_cursor_plane(struct drm_plane *plane, return -ENOMEM; }

• if (fb == crtc->cursor->fb)

• return 0;
  

• /* we only need to pin inside GTT if cursor is non-phy */ mutex_lock(&dev->struct_mutex); if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {

-- 1.9.3

---

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

On Tue, Oct 07, 2014 at 06:01:10PM +0300, Ville Syrjälä wrote:

On Tue, Oct 07, 2014 at 05:47:52PM +0300, Ville Syrjälä wrote:

...

On Wed, Sep 24, 2014 at 02:20:25PM -0300, Gustavo Padovan wrote:

...

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

Move check inside intel_crtc_cursor_set_obj() to intel_check_cursor_plane(), we only use it there so move them out to make the merge of intel_crtc_cursor_set_obj() into intel_check_cursor_plane() easier.

This is another step toward the atomic modesetting support and unification of plane operations such pin/unpin of fb objects on i915.

v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the code v3: take Ville's comment: kept only the restructuring changes, the rest of the code was moved to a separated patch since it is a bug fix (we weren't checking sizes when the fb was the same)

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

Looks like the commit messages for patches 2 and 3 got somehow swapped around. With that fixed patches 2 and 3 get:

And make that patches 3 and 4 and we're actaully talking about the correct two patches.

Ok, I think I've fixed it up.

...

Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

Merged up to this patch to dinq. -Daniel

...

...

---

drivers/gpu/drm/i915/intel_display.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 20be2ed..f91a5b0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11928,9 +11928,6 @@ intel_check_cursor_plane(struct drm_plane *plane, if (!obj) return 0;

• if (fb == crtc->cursor->fb)
• return 0;
• /* Check for which cursor types we support */ crtc_w = drm_rect_width(&state->orig_dst); crtc_h = drm_rect_height(&state->orig_dst);

@@ -11945,6 +11942,9 @@ intel_check_cursor_plane(struct drm_plane *plane, return -ENOMEM; }

• if (fb == crtc->cursor->fb)
• return 0;
• /* we only need to pin inside GTT if cursor is non-phy */ mutex_lock(&dev->struct_mutex); if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {

-- 1.9.3

---

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

-- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:

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

Merge it into the plane update_plane() callback and make other users use the update_plane() functions instead.

The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj() so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane() and merge both paths into one.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/i915/intel_display.c | 221 +++++++++++++++++------------------ 1 file changed, 106 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f91a5b0..a583abd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev, return true; }

-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,

• 		     struct drm_i915_gem_object *obj,
  

• 		     uint32_t width, uint32_t height)
  

-{

• 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;
• unsigned old_width;
• uint32_t addr;
• int ret;
• /* if we want to turn off the cursor ignore width and height */
• if (!obj) {

• DRM_DEBUG_KMS("cursor off\n");
  

• addr = 0;
  

• mutex_lock(&dev->struct_mutex);
  

• goto finish;
  

• }
• /* we only need to pin inside GTT if cursor is non-phy */
• mutex_lock(&dev->struct_mutex);
• if (!INTEL_INFO(dev)->cursor_needs_physical) {

• unsigned alignment;
  

• /*
  

•  * Global gtt pte registers are special registers which actually
  

•  * forward writes to a chunk of system memory. Which means that
  

•  * there is no risk that the register values disappear as soon
  

•  * as we call intel_runtime_pm_put(), so it is correct to wrap
  

•  * only the pin/unpin/fence and not more.
  

•  */
  

• intel_runtime_pm_get(dev_priv);
  

• /* Note that the w/a also requires 2 PTE of padding following
  

•  * the bo. We currently fill all unused PTE with the shadow
  

•  * page and so we should always have valid PTE following the
  

•  * cursor preventing the VT-d warning.
  

•  */
  

• alignment = 0;
  

• if (need_vtd_wa(dev))
  

• 	alignment = 64*1024;
  

• ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_locked;
  

• }
  

• ret = i915_gem_object_put_fence(obj);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to release fence for cursor");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_unpin;
  

• }
  

• addr = i915_gem_obj_ggtt_offset(obj);
  

• intel_runtime_pm_put(dev_priv);
  

• } else {

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

• ret = i915_gem_object_attach_phys(obj, align);
  

• if (ret) {
  

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

• 	goto fail_locked;
  

• }
  

• addr = obj->phys_handle->busaddr;
  

• }
• finish:
• if (intel_crtc->cursor_bo) {

• if (!INTEL_INFO(dev)->cursor_needs_physical)
  

• 	i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
  

• }
• i915_gem_track_fb(intel_crtc->cursor_bo, obj,

• 	  INTEL_FRONTBUFFER_CURSOR(pipe));
  

• mutex_unlock(&dev->struct_mutex);
• old_width = intel_crtc->cursor_width;
• intel_crtc->cursor_addr = addr;
• intel_crtc->cursor_bo = obj;
• intel_crtc->cursor_width = width;
• intel_crtc->cursor_height = height;
• if (intel_crtc->active) {

• if (old_width != width)
  

• 	intel_update_watermarks(crtc);
  

• intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
  

• }
• intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
• return 0;

-fail_unpin:

• i915_gem_object_unpin_from_display_plane(obj);

-fail_locked:

• mutex_unlock(&dev->struct_mutex);
• return ret;

-}

static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, u16 *blue, uint32_t start, uint32_t size) { @@ -11897,7 +11794,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)

BUG_ON(!plane->crtc);

• return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);

• return plane->funcs->update_plane(plane, plane->crtc, NULL,

• 			  0, 0, 0, 0, 0, 0, 0, 0);
  

}

static int @@ -11961,26 +11859,119 @@ intel_commit_cursor_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_crtc *crtc = state->crtc;

• struct drm_device *dev = crtc->dev;
• struct drm_i915_private *dev_priv = dev->dev_private; struct drm_framebuffer *fb = state->fb; struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
• struct drm_i915_gem_object *obj = intel_fb->obj;
• int crtc_w, crtc_h;

• struct drm_i915_gem_object *obj = intel_fb_obj(fb);

• enum pipe pipe = intel_crtc->pipe;

• unsigned old_width;

• uint32_t addr;

• bool on;

• int ret;

  crtc->cursor_x = state->orig_dst.x1; crtc->cursor_y = state->orig_dst.y1;

• if (fb != crtc->cursor->fb) {

• crtc_w = drm_rect_width(&state->orig_dst);
  

• crtc_h = drm_rect_height(&state->orig_dst);
  

• return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
  

• if (intel_crtc->cursor_bo == obj)

• goto update;
  

• /* if we want to turn off the cursor ignore width and height */
• if (!obj) {

• DRM_DEBUG_KMS("cursor off\n");
  

• addr = 0;
  

• mutex_lock(&dev->struct_mutex);
  

• goto finish;
  

• }
• /* we only need to pin inside GTT if cursor is non-phy */
• mutex_lock(&dev->struct_mutex);
• if (!INTEL_INFO(dev)->cursor_needs_physical) {

• unsigned alignment;
  

• /*
  

•  * Global gtt pte registers are special registers which actually
  

•  * forward writes to a chunk of system memory. Which means that
  

•  * there is no risk that the register values disappear as soon
  

•  * as we call intel_runtime_pm_put(), so it is correct to wrap
  

•  * only the pin/unpin/fence and not more.
  

•  */
  

• intel_runtime_pm_get(dev_priv);
  

• /* Note that the w/a also requires 2 PTE of padding following
  

•  * the bo. We currently fill all unused PTE with the shadow
  

•  * page and so we should always have valid PTE following the
  

•  * cursor preventing the VT-d warning.
  

•  */
  

• alignment = 0;
  

• if (need_vtd_wa(dev))
  

• 	alignment = 64*1024;
  

• ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_locked;
  

• }
  

• ret = i915_gem_object_put_fence(obj);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to release fence for cursor");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_unpin;
  

• }
  

• addr = i915_gem_obj_ggtt_offset(obj);
  

• intel_runtime_pm_put(dev_priv);
  

  } else {

• intel_crtc_update_cursor(crtc, state->visible);
  

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

• ret = i915_gem_object_attach_phys(obj, align);
  

• if (ret) {
  

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

• 	goto fail_locked;
  

• }
  

• addr = obj->phys_handle->busaddr;
  

• }

• intel_frontbuffer_flip(crtc->dev,
  

• 		       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
  

+finish:

• if (intel_crtc->cursor_bo) {

• if (!INTEL_INFO(dev)->cursor_needs_physical)
  

• 	i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
  

• }

• return 0;
  

• i915_gem_track_fb(intel_crtc->cursor_bo, obj,

• 	  INTEL_FRONTBUFFER_CURSOR(pipe));
  

• mutex_unlock(&dev->struct_mutex);
• intel_crtc->cursor_addr = addr;
• intel_crtc->cursor_bo = obj;

+update:

• old_width = intel_crtc->cursor_width;
• intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
• intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
• if (intel_crtc->cursor_bo == obj)

• on = state->visible;
  

• else

• on = !obj;
  

That looks fishy. Why do we need to care if the bo changed here, and why would we turn on the cursor when there's no bo?

The cursor is either visible or it isn't, and that's all that intel_crtc_update_cursor() should care about.

• if (intel_crtc->active) {

• if (old_width != intel_crtc->cursor_width)
  

• 	intel_update_watermarks(crtc);
  

• intel_crtc_update_cursor(crtc, on);
  

  }
• intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));

This should probably be done only when crtc->active==true. I see we're not doing it that way currently, so I guess we could have a separate patch to change that.

• return 0;

+fail_unpin:

• i915_gem_object_unpin_from_display_plane(obj);

+fail_locked:

• mutex_unlock(&dev->struct_mutex);
• drm_gem_object_unreference_unlocked(&obj->base);
• return ret;

}

static int

1.9.3

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx

On Fri, Oct 24, 2014 at 02:23:35PM +0100, Gustavo Padovan wrote:

2014-10-07 Ville Syrjälä ville.syrjala@linux.intel.com:

...

On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:

...

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

Merge it into the plane update_plane() callback and make other users use the update_plane() functions instead.

The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj() so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane() and merge both paths into one.

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/i915/intel_display.c | 221 +++++++++++++++++------------------ 1 file changed, 106 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f91a5b0..a583abd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev, return true; }

-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,

• 		     struct drm_i915_gem_object *obj,
  

• 		     uint32_t width, uint32_t height)
  

-{

• 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;
• unsigned old_width;
• uint32_t addr;
• int ret;
• /* if we want to turn off the cursor ignore width and height */
• if (!obj) {

• DRM_DEBUG_KMS("cursor off\n");
  

• addr = 0;
  

• mutex_lock(&dev->struct_mutex);
  

• goto finish;
  

• }
• /* we only need to pin inside GTT if cursor is non-phy */
• mutex_lock(&dev->struct_mutex);
• if (!INTEL_INFO(dev)->cursor_needs_physical) {

• unsigned alignment;
  

• /*
  

•  * Global gtt pte registers are special registers which actually
  

•  * forward writes to a chunk of system memory. Which means that
  

•  * there is no risk that the register values disappear as soon
  

•  * as we call intel_runtime_pm_put(), so it is correct to wrap
  

•  * only the pin/unpin/fence and not more.
  

•  */
  

• intel_runtime_pm_get(dev_priv);
  

• /* Note that the w/a also requires 2 PTE of padding following
  

•  * the bo. We currently fill all unused PTE with the shadow
  

•  * page and so we should always have valid PTE following the
  

•  * cursor preventing the VT-d warning.
  

•  */
  

• alignment = 0;
  

• if (need_vtd_wa(dev))
  

• 	alignment = 64*1024;
  

• ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_locked;
  

• }
  

• ret = i915_gem_object_put_fence(obj);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to release fence for cursor");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_unpin;
  

• }
  

• addr = i915_gem_obj_ggtt_offset(obj);
  

• intel_runtime_pm_put(dev_priv);
  

• } else {

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

• ret = i915_gem_object_attach_phys(obj, align);
  

• if (ret) {
  

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

• 	goto fail_locked;
  

• }
  

• addr = obj->phys_handle->busaddr;
  

• }
• finish:
• if (intel_crtc->cursor_bo) {

• if (!INTEL_INFO(dev)->cursor_needs_physical)
  

• 	i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
  

• }
• i915_gem_track_fb(intel_crtc->cursor_bo, obj,

• 	  INTEL_FRONTBUFFER_CURSOR(pipe));
  

• mutex_unlock(&dev->struct_mutex);
• old_width = intel_crtc->cursor_width;
• intel_crtc->cursor_addr = addr;
• intel_crtc->cursor_bo = obj;
• intel_crtc->cursor_width = width;
• intel_crtc->cursor_height = height;
• if (intel_crtc->active) {

• if (old_width != width)
  

• 	intel_update_watermarks(crtc);
  

• intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
  

• }
• intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
• return 0;

-fail_unpin:

• i915_gem_object_unpin_from_display_plane(obj);

-fail_locked:

• mutex_unlock(&dev->struct_mutex);
• return ret;

-}

static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, u16 *blue, uint32_t start, uint32_t size) { @@ -11897,7 +11794,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)

BUG_ON(!plane->crtc);

• return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);

• return plane->funcs->update_plane(plane, plane->crtc, NULL,

• 			  0, 0, 0, 0, 0, 0, 0, 0);
  

}

static int @@ -11961,26 +11859,119 @@ intel_commit_cursor_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_crtc *crtc = state->crtc;

• struct drm_device *dev = crtc->dev;
• struct drm_i915_private *dev_priv = dev->dev_private; struct drm_framebuffer *fb = state->fb; struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
• struct drm_i915_gem_object *obj = intel_fb->obj;
• int crtc_w, crtc_h;

• struct drm_i915_gem_object *obj = intel_fb_obj(fb);

• enum pipe pipe = intel_crtc->pipe;

• unsigned old_width;

• uint32_t addr;

• bool on;

• int ret;

  crtc->cursor_x = state->orig_dst.x1; crtc->cursor_y = state->orig_dst.y1;

• if (fb != crtc->cursor->fb) {

• crtc_w = drm_rect_width(&state->orig_dst);
  

• crtc_h = drm_rect_height(&state->orig_dst);
  

• return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
  

• if (intel_crtc->cursor_bo == obj)

• goto update;
  

• /* if we want to turn off the cursor ignore width and height */
• if (!obj) {

• DRM_DEBUG_KMS("cursor off\n");
  

• addr = 0;
  

• mutex_lock(&dev->struct_mutex);
  

• goto finish;
  

• }
• /* we only need to pin inside GTT if cursor is non-phy */
• mutex_lock(&dev->struct_mutex);
• if (!INTEL_INFO(dev)->cursor_needs_physical) {

• unsigned alignment;
  

• /*
  

•  * Global gtt pte registers are special registers which actually
  

•  * forward writes to a chunk of system memory. Which means that
  

•  * there is no risk that the register values disappear as soon
  

•  * as we call intel_runtime_pm_put(), so it is correct to wrap
  

•  * only the pin/unpin/fence and not more.
  

•  */
  

• intel_runtime_pm_get(dev_priv);
  

• /* Note that the w/a also requires 2 PTE of padding following
  

•  * the bo. We currently fill all unused PTE with the shadow
  

•  * page and so we should always have valid PTE following the
  

•  * cursor preventing the VT-d warning.
  

•  */
  

• alignment = 0;
  

• if (need_vtd_wa(dev))
  

• 	alignment = 64*1024;
  

• ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_locked;
  

• }
  

• ret = i915_gem_object_put_fence(obj);
  

• if (ret) {
  

• 	DRM_DEBUG_KMS("failed to release fence for cursor");
  

• 	intel_runtime_pm_put(dev_priv);
  

• 	goto fail_unpin;
  

• }
  

• addr = i915_gem_obj_ggtt_offset(obj);
  

• intel_runtime_pm_put(dev_priv);
  

  } else {

• intel_crtc_update_cursor(crtc, state->visible);
  

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

• ret = i915_gem_object_attach_phys(obj, align);
  

• if (ret) {
  

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

• 	goto fail_locked;
  

• }
  

• addr = obj->phys_handle->busaddr;
  

• }

• intel_frontbuffer_flip(crtc->dev,
  

• 		       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
  

+finish:

• if (intel_crtc->cursor_bo) {

• if (!INTEL_INFO(dev)->cursor_needs_physical)
  

• 	i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
  

• }

• return 0;
  

• i915_gem_track_fb(intel_crtc->cursor_bo, obj,

• 	  INTEL_FRONTBUFFER_CURSOR(pipe));
  

• mutex_unlock(&dev->struct_mutex);
• intel_crtc->cursor_addr = addr;
• intel_crtc->cursor_bo = obj;

+update:

• old_width = intel_crtc->cursor_width;
• intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
• intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
• if (intel_crtc->cursor_bo == obj)

• on = state->visible;
  

• else

• on = !obj;
  

That looks fishy. Why do we need to care if the bo changed here, and why would we turn on the cursor when there's no bo?

Yes, this should actually have been on = !!obj. But all I was doing here is reordering the code, so this is how it was before. The flow is the same.

...

The cursor is either visible or it isn't, and that's all that intel_crtc_update_cursor() should care about.

Right, I'll remove the if-else and call intel_crtc_update_cursor() with state->visible as parameter.

I just realized that we don't seem to actually compute state->visible correctly when there's no fb. So that definitely needs to be done if we want to share the .update_plane() as .disable_plane().

...

...

• if (intel_crtc->active) {

• if (old_width != intel_crtc->cursor_width)
  

• 	intel_update_watermarks(crtc);
  

• intel_crtc_update_cursor(crtc, on);
  

  }
• intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));

This should probably be done only when crtc->active==true. I see we're not doing it that way currently, so I guess we could have a separate patch to change that.

Okay, I have created a separated patch for this.

Gustavo

On Wed, Sep 24, 2014 at 02:20:27PM -0300, Gustavo Padovan wrote:

From: Daniel Stone daniels@collabora.com

Start the work of splitting the intel_crtc_page_flip() for later use by the atomic modesetting API.

At this time this doesn't really do anything so I don't see much point in applying it, for now at least.

Signed-off-by: Daniel Stone daniels@collabora.com Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

---

drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a583abd..3cb092c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9641,23 +9641,11 @@ void intel_check_page_flip(struct drm_device *dev, int pipe) spin_unlock(&dev->event_lock); }

-static int intel_crtc_page_flip(struct drm_crtc *crtc,

• 		struct drm_framebuffer *fb,
  

• 		struct drm_pending_vblank_event *event,
  

• 		uint32_t page_flip_flags)
  

+static int intel_crtc_check_page_flip(struct drm_crtc *crtc,

• 		      struct drm_framebuffer *fb)
  

{ struct drm_device *dev = crtc->dev;

• struct drm_i915_private *dev_priv = dev->dev_private; struct drm_framebuffer *old_fb = crtc->primary->fb;

• struct drm_i915_gem_object *obj = intel_fb_obj(fb);

• struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• enum pipe pipe = intel_crtc->pipe;

• struct intel_unpin_work *work;

• struct intel_engine_cs *ring;

• int ret;

• //trigger software GT busyness calculation

• gen8_flip_interrupt(dev);

  /*

  • drm_mode_page_flip_ioctl() should already catch this, but double

@@ -9680,6 +9668,27 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, fb->pitches[0] != crtc->primary->fb->pitches[0])) return -EINVAL;

• return 0;

+}

+static int intel_crtc_commit_page_flip(struct drm_crtc *crtc,

• 		       struct drm_framebuffer *fb,
  

• 		       struct drm_pending_vblank_event *event,
  

• 		       uint32_t page_flip_flags)
  

+{

• struct drm_device *dev = crtc->dev;
• struct drm_i915_private *dev_priv = dev->dev_private;
• struct drm_framebuffer *old_fb = crtc->primary->fb;
• struct drm_i915_gem_object *obj = intel_fb_obj(fb);
• struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
• enum pipe pipe = intel_crtc->pipe;
• struct intel_unpin_work *work;
• struct intel_engine_cs *ring;
• int ret;
• /* trigger software GT busyness calculation */
• gen8_flip_interrupt(dev);
• if (i915_terminally_wedged(&dev_priv->gpu_error)) goto out_hang;

@@ -9823,6 +9832,20 @@ out_hang: return ret; }

+static int intel_crtc_page_flip(struct drm_crtc *crtc,

• 		struct drm_framebuffer *fb,
  

• 		struct drm_pending_vblank_event *event,
  

• 		uint32_t page_flip_flags)
  

+{

• int ret;
• ret = intel_crtc_check_page_flip(crtc, fb);
• if (ret)

• return ret;
  

• return intel_crtc_commit_page_flip(crtc, fb, event, page_flip_flags);

+}

static struct drm_crtc_helper_funcs intel_helper_funcs = { .mode_set_base_atomic = intel_pipe_set_base_atomic, .load_lut = intel_crtc_load_lut, -- 1.9.3

---

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

On Wed, Sep 24, 2014 at 02:20:28PM -0300, Gustavo Padovan wrote:

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

We need to get hdisplay and vdisplay in a few places so create a helper to make our job easier.

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

---

drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++------- drivers/gpu/drm/i915/intel_display.c | 6 +++--- include/drm/drm_crtc.h | 2 ++ 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b702106..7c0bf9f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2490,6 +2490,17 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) } EXPORT_SYMBOL(drm_mode_set_config_internal);

+void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,

• 	    int *hdisplay, int *vdisplay)
  

+{

• struct drm_display_mode adjusted = *mode;
• drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
• *hdisplay = adjusted.crtc_hdisplay;
• *vdisplay = adjusted.crtc_vdisplay;

Hmm. Now that I read drm_mode_set_crtcinfo() it would seem we'll do the wrong thing when dealing with doublescan or vscan>1. That problem is already true without your patch, but we should do something to fix it anyway.

Maybe the best option would be to yank the stereo doubling logic from drm_mode_set_crtcinfo() into a separate function and call it both here and in drm_mode_set_crtcinfo(). That would avoid this problem without duplicating the specifics of stereo doubling in multiple places.

+} +EXPORT_SYMBOL(drm_crtc_get_hv_timing);

/**

• drm_crtc_check_viewport - Checks that a framebuffer is big enough for the

• CRTC viewport
  

@@ -2510,13 +2521,8 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc, hdisplay = mode->hdisplay; vdisplay = mode->vdisplay;

With the suggested doublescan/vscan fix you could kill these assignments too and just call drm_crtc_get_hv_timing() unconditionally.

• if (drm_mode_is_stereo(mode)) {

• struct drm_display_mode adjusted = *mode;
  

• drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
  

• hdisplay = adjusted.crtc_hdisplay;
  

• vdisplay = adjusted.crtc_vdisplay;
  

• }

• if (drm_mode_is_stereo(mode))

• drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
  
  

  if (crtc->invert_dimensions) swap(hdisplay, vdisplay);

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3cb092c..dfe9ad5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10154,9 +10154,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, * computation to clearly distinguish it from the adjusted mode, which * can be changed by the connectors in the below retry loop. */

• drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
• pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
• pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;

• drm_crtc_get_hv_timing(&pipe_config->requested_mode,

• 	       &pipe_config->pipe_src_w,
  

• 	       &pipe_config->pipe_src_h);
  
  

encoder_retry: /* Ensure the port clock defaults are reset when retrying. */ diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c40070a..9b2f6b5 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -950,6 +950,8 @@ extern int drm_plane_init(struct drm_device *dev, extern void drm_plane_cleanup(struct drm_plane *plane); extern unsigned int drm_plane_index(struct drm_plane *plane); extern void drm_plane_force_disable(struct drm_plane *plane); +extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,

• 		   int *hdisplay, int *vdisplay);
  

extern int drm_crtc_check_viewport(const struct drm_crtc *crtc, int x, int y, const struct drm_display_mode *mode, -- 1.9.3

---

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

2014-10-07 Ville Syrjälä ville.syrjala@linux.intel.com:

On Wed, Sep 24, 2014 at 02:20:32PM -0300, Gustavo Padovan wrote:

...

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

After some refactor intel_primary_plane_setplane() does the same as intel_pipe_set_base() so we can get rid of it and replace the calls with intel_primary_plane_setplane().

v2: take Ville's comments:

• get the right arguments for update_plane()
• use drm_crtc_get_hv_timing()

Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk

drivers/gpu/drm/i915/intel_display.c | 89 ++++++++---------------------------- 1 file changed, 18 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9dd4952..f7c2e5f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2857,74 +2857,6 @@ static void intel_update_pipe_size(struct intel_crtc *crtc) crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay; }

-static int -intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,

•     struct drm_framebuffer *fb)
  

-{

• 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 = crtc->primary->fb;
• struct drm_i915_gem_object *obj = intel_fb_obj(fb);
• struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
• int ret;
• if (intel_crtc_has_pending_flip(crtc)) {

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

• return -EBUSY;
  

• }
• /* no fb bound */
• if (!fb) {

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

• return 0;
  

• }
• if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {

• DRM_ERROR("no plane for crtc: plane %c, num_pipes %d\n",
  

• 	  plane_name(intel_crtc->plane),
  

• 	  INTEL_INFO(dev)->num_pipes);
  

• return -EINVAL;
  

• }
• 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);
• if (ret != 0) {

• DRM_ERROR("pin & fence failed\n");
  

• return ret;
  

• }
• intel_update_pipe_size(intel_crtc);
• dev_priv->display.update_primary_plane(crtc, fb, x, y);
• if (intel_crtc->active)

• intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
  

• crtc->primary->fb = fb;
• crtc->x = x;
• crtc->y = y;
• if (old_fb) {

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

• 	intel_wait_for_vblank(dev, intel_crtc->pipe);
  

• mutex_lock(&dev->struct_mutex);
  

• intel_unpin_fb_obj(old_obj);
  

• mutex_unlock(&dev->struct_mutex);
  

• }
• mutex_lock(&dev->struct_mutex);
• intel_update_fbc(dev);
• mutex_unlock(&dev->struct_mutex);
• return 0;

-}

static void intel_fdi_normal_train(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -9681,6 +9613,8 @@ static int intel_crtc_commit_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *old_fb = crtc->primary->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

• struct drm_plane *primary = crtc->primary;
• struct intel_plane *intel_plane = to_intel_plane(primary); enum pipe pipe = intel_crtc->pipe; struct intel_unpin_work *work; struct intel_engine_cs *ring;

@@ -9822,7 +9756,15 @@ free_work: if (ret == -EIO) { out_hang: intel_crtc_wait_for_pending_flips(crtc);

• ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
  

• ret = primary->funcs->update_plane(primary, crtc, fb,
  

• 				   intel_plane->crtc_x,
  

• 				   intel_plane->crtc_y,
  

• 				   intel_plane->crtc_h,
  

• 				   intel_plane->crtc_w,
  

• 				   intel_plane->src_x,
  

• 				   intel_plane->src_y,
  

• 				   intel_plane->src_h,
  

• 				   intel_plane->src_w);
  

  if (ret == 0 && event) { spin_lock_irq(&dev->event_lock); drm_send_vblank_event(dev, pipe, event);

@@ -11359,11 +11301,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set) set->x, set->y, set->fb); } else if (config->fb_changed) { struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);

• struct drm_plane *primary = set->crtc->primary;
  

• int vdisplay, hdisplay;
  
  

  intel_crtc_wait_for_pending_flips(set->crtc);

• ret = intel_pipe_set_base(set->crtc,
  

• 			  set->x, set->y, set->fb);
  

• drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
  

• ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
  

• 				   0, 0, hdisplay, vdisplay,
  

• 				   set->x << 16, set->y << 16,
  

• 				   hdisplay << 16, vdisplay << 16);
  
  

These two look like they should do the right thing. But we still have the mode_change==true path to worry about. If we don't call update_plane() there we migth never populate intel_plane->crtc_x & co.

So I think you'll just need to replace the small piece of pin/unpin code in __intel_set_mode() with a call to update_plane(). Since the crtc is not active at that point it will skip the actual plane register programming and intel_enable_primary_hw_plane() will later do all that.

Yes, this is a pending patch that I have here. It does exactly what you are saying but it breaks everything. It needs more work I think, the simple replacement by an update_plane() doesn't work.

Gustavo