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
v5: take more Ville's comments: - set primary_enabled to true and add BDW hack - unify if (old_fb) and if (old_fb != fb)
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 | 142 +++++++++++++++++++++-------------- 1 file changed, 87 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 858011d..bef37dc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11578,12 +11578,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 @@ -11595,6 +11606,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); @@ -11603,76 +11616,95 @@ 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) { + 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); + if (ret != 0) { + DRM_DEBUG_KMS("pin & fence failed\n"); + return ret; + } + } + + 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); + intel_plane->crtc_h = drm_rect_height(&state->orig_dst); + intel_plane->src_x = state->orig_src.x1; + intel_plane->src_y = state->orig_src.y1; + intel_plane->src_w = drm_rect_width(&state->orig_src); + intel_plane->src_h = drm_rect_height(&state->orig_src); + intel_plane->obj = obj; + + if (intel_crtc->active) { /* - * Try to pin the new fb first so that we can bail out if we - * fail. + * 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 (plane->fb != fb) { - ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); - if (ret) { - mutex_unlock(&dev->struct_mutex); - return ret; - } + 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); }
- 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 (state->visible) { + int was_enabled = intel_crtc->primary_enabled;
+ /* FIXME: kill this fastboot hack */ + intel_update_pipe_size(intel_crtc);
- if (plane->fb != fb) - if (plane->fb) - intel_unpin_fb_obj(old_obj); + intel_crtc->primary_enabled = true;
- mutex_unlock(&dev->struct_mutex); + dev_priv->display.update_primary_plane(crtc, plane->fb, + crtc->x, crtc->y);
- } else { - if (intel_crtc && intel_crtc->active && - intel_crtc->primary_enabled) { + if (IS_BROADWELL(dev) && was_enabled) + intel_wait_for_vblank(dev, intel_crtc->pipe); + } else { /* - * 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 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 (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); - } + intel_disable_primary_hw_plane(plane, crtc); } - ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb); - if (ret) - return ret;
- if (!intel_crtc->primary_enabled) - intel_enable_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); }
- 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); - intel_plane->crtc_h = drm_rect_height(&state->orig_dst); - intel_plane->src_x = state->orig_src.x1; - intel_plane->src_y = state->orig_src.y1; - intel_plane->src_w = drm_rect_width(&state->orig_src); - intel_plane->src_h = drm_rect_height(&state->orig_src); - intel_plane->obj = obj; + if (old_fb && old_fb != fb) { + if (intel_crtc->active) + intel_wait_for_vblank(dev, intel_crtc->pipe); + + mutex_lock(&dev->struct_mutex); + intel_unpin_fb_obj(old_obj); + mutex_unlock(&dev->struct_mutex); + }
return 0; }
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Now that universal planes are in place we don't need this plane unref on failures.
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 | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bef37dc..2ef1836 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8362,8 +8362,7 @@ static bool cursor_size_ok(struct drm_device *dev, /* * intel_crtc_cursor_set_obj - Set cursor to specified GEM object * - * Note that the object's reference will be consumed if the update fails. If - * the update succeeds, the reference of the old object (if any) will be + * If the update succeeds, the reference of the old object (if any) will be * consumed. */ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, @@ -8395,8 +8394,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, stride = roundup_pow_of_two(width) * 4; if (obj->base.size < stride * height) { DRM_DEBUG_KMS("buffer is too small\n"); - ret = -ENOMEM; - goto fail; + return -ENOMEM; }
/* we only need to pin inside GTT if cursor is non-phy */ @@ -8485,8 +8483,6 @@ fail_unpin: i915_gem_object_unpin_from_display_plane(obj); fail_locked: mutex_unlock(&dev->struct_mutex); -fail: - drm_gem_object_unreference_unlocked(&obj->base); return ret; }
On Mon, Sep 22, 2014 at 07:23:09PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Now that universal planes are in place we don't need this plane unref on failures.
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 | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bef37dc..2ef1836 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8362,8 +8362,7 @@ static bool cursor_size_ok(struct drm_device *dev, /*
- intel_crtc_cursor_set_obj - Set cursor to specified GEM object
- Note that the object's reference will be consumed if the update fails. If
- the update succeeds, the reference of the old object (if any) will be
- If the update succeeds, the reference of the old object (if any) will be
- consumed.
The rest of this comment should be removed as well. With universal planes the fb holds the gem obj reference so we don't frob with gem obj references at all here.
With that fixed this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
*/ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, @@ -8395,8 +8394,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, stride = roundup_pow_of_two(width) * 4; if (obj->base.size < stride * height) { DRM_DEBUG_KMS("buffer is too small\n");
ret = -ENOMEM;goto fail;
return -ENOMEM;}
/* we only need to pin inside GTT if cursor is non-phy */
@@ -8485,8 +8483,6 @@ fail_unpin: i915_gem_object_unpin_from_display_plane(obj); fail_locked: mutex_unlock(&dev->struct_mutex); -fail:
- drm_gem_object_unreference_unlocked(&obj->base); return ret;
}
-- 1.9.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Move checks inside intel_crtc_cursor_set_obj() to intel_check_cursor_plane(), we only use they 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
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ef1836..3f37e93 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, 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, stride; + unsigned old_width; uint32_t addr; int ret;
@@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, goto finish; }
- /* Check for which cursor types we support */ - if (!cursor_size_ok(dev, width, height)) { - DRM_DEBUG("Cursor dimension not supported\n"); - return -EINVAL; - } - - stride = roundup_pow_of_two(width) * 4; - if (obj->base.size < stride * height) { - DRM_DEBUG_KMS("buffer is too small\n"); - return -ENOMEM; - } - /* 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;
- if (obj->tiling_mode) { - DRM_DEBUG_KMS("cursor cannot be tiled\n"); - ret = -EINVAL; - goto fail_locked; - } - /* * Global gtt pte registers are special registers which actually * forward writes to a chunk of system memory. Which means that @@ -11826,16 +11808,55 @@ intel_check_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_framebuffer *fb = state->fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; + struct drm_i915_gem_object *obj = intel_fb_obj(fb); + int crtc_w, crtc_h; + unsigned stride; + 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, true, true, &state->visible); + if (ret) + return ret; + + + /* if we want to turn off the cursor ignore width and height */ + 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); + if (!cursor_size_ok(dev, crtc_w, crtc_h)) { + DRM_DEBUG("Cursor dimension not supported\n"); + return -EINVAL; + } + + stride = roundup_pow_of_two(crtc_w) * 4; + if (obj->base.size < stride * crtc_h) { + DRM_DEBUG_KMS("buffer is too small\n"); + return -ENOMEM; + } + + /* 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) { + DRM_DEBUG_KMS("cursor cannot be tiled\n"); + ret = -EINVAL; + } + mutex_unlock(&dev->struct_mutex); + + return ret; }
static int
On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Move checks inside intel_crtc_cursor_set_obj() to intel_check_cursor_plane(), we only use they 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
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ef1836..3f37e93 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, 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, stride;
- unsigned old_width; uint32_t addr; int ret;
@@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, goto finish; }
/* Check for which cursor types we support */
if (!cursor_size_ok(dev, width, height)) {
DRM_DEBUG("Cursor dimension not supported\n");return -EINVAL;}
stride = roundup_pow_of_two(width) * 4;
if (obj->base.size < stride * height) {
DRM_DEBUG_KMS("buffer is too small\n");return -ENOMEM;}
/* 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;
if (obj->tiling_mode) {DRM_DEBUG_KMS("cursor cannot be tiled\n");ret = -EINVAL;goto fail_locked;}/*
- Global gtt pte registers are special registers which actually
- forward writes to a chunk of system memory. Which means that
@@ -11826,16 +11808,55 @@ intel_check_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_framebuffer *fb = state->fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip;
- struct drm_i915_gem_object *obj = intel_fb_obj(fb);
- int crtc_w, crtc_h;
- unsigned stride;
- 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, true, true, &state->visible);
- if (ret)
return ret;- /* if we want to turn off the cursor ignore width and height */
- if (!obj)
return 0;- if (fb == crtc->cursor->fb)
return 0;
Hmm. This check needs to be after the cursor/obj size checks. Otherwise we wouldn't reject invalid sized cursors when the fb didn't change. I suppose we could also just drop this check, but it can still save us from doing the tiling check since that can't have changed if the fb hasn't changed so maybe it's worth keeping. But either solution is fine by me.
- /* Check for which cursor types we support */
- crtc_w = drm_rect_width(&state->orig_dst);
- crtc_h = drm_rect_height(&state->orig_dst);
- if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
DRM_DEBUG("Cursor dimension not supported\n");return -EINVAL;- }
- stride = roundup_pow_of_two(crtc_w) * 4;
- if (obj->base.size < stride * crtc_h) {
DRM_DEBUG_KMS("buffer is too small\n");return -ENOMEM;- }
- /* 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) {
DRM_DEBUG_KMS("cursor cannot be tiled\n");ret = -EINVAL;- }
- mutex_unlock(&dev->struct_mutex);
- return ret;
'return 0' seems better here.
With these issues fixed: Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
}
static int
1.9.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2014-09-23 Ville Syrjälä ville.syrjala@linux.intel.com:
On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Move checks inside intel_crtc_cursor_set_obj() to intel_check_cursor_plane(), we only use they 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
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ef1836..3f37e93 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, 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, stride;
- unsigned old_width; uint32_t addr; int ret;
@@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, goto finish; }
/* Check for which cursor types we support */
if (!cursor_size_ok(dev, width, height)) {
DRM_DEBUG("Cursor dimension not supported\n");return -EINVAL;}
stride = roundup_pow_of_two(width) * 4;
if (obj->base.size < stride * height) {
DRM_DEBUG_KMS("buffer is too small\n");return -ENOMEM;}
/* 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;
if (obj->tiling_mode) {DRM_DEBUG_KMS("cursor cannot be tiled\n");ret = -EINVAL;goto fail_locked;}/*
- Global gtt pte registers are special registers which actually
- forward writes to a chunk of system memory. Which means that
@@ -11826,16 +11808,55 @@ intel_check_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_framebuffer *fb = state->fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip;
- struct drm_i915_gem_object *obj = intel_fb_obj(fb);
- int crtc_w, crtc_h;
- unsigned stride;
- 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, true, true, &state->visible);
- if (ret)
return ret;- /* if we want to turn off the cursor ignore width and height */
- if (!obj)
return 0;- if (fb == crtc->cursor->fb)
return 0;Hmm. This check needs to be after the cursor/obj size checks. Otherwise we wouldn't reject invalid sized cursors when the fb didn't change. I suppose we could also just drop this check, but it can still save us from doing the tiling check since that can't have changed if the fb hasn't changed so maybe it's worth keeping. But either solution is fine by me.
Only if this was a bug before this patch, if you look at the original intel_commit_cursor_plane() we do two things when the fbs are the same:
intel_crtc_update_cursor(); intel_frontbuffer_flip();
No checks are performed, that is why I put the fbs check before any other check to make the code as similar as possible to what it was before.
- /* Check for which cursor types we support */
- crtc_w = drm_rect_width(&state->orig_dst);
- crtc_h = drm_rect_height(&state->orig_dst);
- if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
DRM_DEBUG("Cursor dimension not supported\n");return -EINVAL;- }
- stride = roundup_pow_of_two(crtc_w) * 4;
- if (obj->base.size < stride * crtc_h) {
DRM_DEBUG_KMS("buffer is too small\n");return -ENOMEM;- }
- /* 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) {
DRM_DEBUG_KMS("cursor cannot be tiled\n");ret = -EINVAL;- }
- mutex_unlock(&dev->struct_mutex);
- return ret;
'return 0' seems better here.
ret can be -EINVAL, the code inside the mutex 3 lines above can set ret.
Gustavo
On Tue, Sep 23, 2014 at 12:41:56PM -0300, Gustavo Padovan wrote:
2014-09-23 Ville Syrjälä ville.syrjala@linux.intel.com:
On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Move checks inside intel_crtc_cursor_set_obj() to intel_check_cursor_plane(), we only use they 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
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ef1836..3f37e93 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, 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, stride;
- unsigned old_width; uint32_t addr; int ret;
@@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, goto finish; }
/* Check for which cursor types we support */
if (!cursor_size_ok(dev, width, height)) {
DRM_DEBUG("Cursor dimension not supported\n");return -EINVAL;}
stride = roundup_pow_of_two(width) * 4;
if (obj->base.size < stride * height) {
DRM_DEBUG_KMS("buffer is too small\n");return -ENOMEM;}
/* 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;
if (obj->tiling_mode) {DRM_DEBUG_KMS("cursor cannot be tiled\n");ret = -EINVAL;goto fail_locked;}/*
- Global gtt pte registers are special registers which actually
- forward writes to a chunk of system memory. Which means that
@@ -11826,16 +11808,55 @@ intel_check_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_framebuffer *fb = state->fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip;
- struct drm_i915_gem_object *obj = intel_fb_obj(fb);
- int crtc_w, crtc_h;
- unsigned stride;
- 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, true, true, &state->visible);
- if (ret)
return ret;- /* if we want to turn off the cursor ignore width and height */
- if (!obj)
return 0;- if (fb == crtc->cursor->fb)
return 0;Hmm. This check needs to be after the cursor/obj size checks. Otherwise we wouldn't reject invalid sized cursors when the fb didn't change. I suppose we could also just drop this check, but it can still save us from doing the tiling check since that can't have changed if the fb hasn't changed so maybe it's worth keeping. But either solution is fine by me.
Only if this was a bug before this patch, if you look at the original intel_commit_cursor_plane() we do two things when the fbs are the same:
intel_crtc_update_cursor(); intel_frontbuffer_flip();
No checks are performed, that is why I put the fbs check before any other check to make the code as similar as possible to what it was before.
I guess we already had the bug then. Not entirely unsurprising given the recent history of this code. Probably better to have the fix as a separate patch then to avoid mixing restructuring and functional changes in the same patch.
- /* Check for which cursor types we support */
- crtc_w = drm_rect_width(&state->orig_dst);
- crtc_h = drm_rect_height(&state->orig_dst);
- if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
DRM_DEBUG("Cursor dimension not supported\n");return -EINVAL;- }
- stride = roundup_pow_of_two(crtc_w) * 4;
- if (obj->base.size < stride * crtc_h) {
DRM_DEBUG_KMS("buffer is too small\n");return -ENOMEM;- }
- /* 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) {
DRM_DEBUG_KMS("cursor cannot be tiled\n");ret = -EINVAL;- }
- mutex_unlock(&dev->struct_mutex);
- return ret;
'return 0' seems better here.
ret can be -EINVAL, the code inside the mutex 3 lines above can set ret.
Oh, somehow I missed that one. You can leave the patch as is then.
Gustavo
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 | 227 ++++++++++++++++------------------- 1 file changed, 106 insertions(+), 121 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3f37e93..417b74d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8359,115 +8359,6 @@ static bool cursor_size_ok(struct drm_device *dev, return true; }
-/* - * intel_crtc_cursor_set_obj - Set cursor to specified GEM object - * - * If the update succeeds, the reference of the old object (if any) will be - * consumed. - */ -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) { @@ -11800,7 +11691,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 @@ -11864,26 +11756,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; + } + +finish: + if (intel_crtc->cursor_bo) { + if (!INTEL_INFO(dev)->cursor_needs_physical) + i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo); + }
- intel_frontbuffer_flip(crtc->dev, - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe)); + i915_gem_track_fb(intel_crtc->cursor_bo, obj, + INTEL_FRONTBUFFER_CURSOR(pipe)); + mutex_unlock(&dev->struct_mutex);
- return 0; + 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; + + 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)); + + 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
From: Daniel Stone daniels@collabora.com
Start the work of splitting the intel_crtc_page_flip() for later use by the atomic modesetting API.
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 417b74d..f9ab8aa 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9543,23 +9543,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 @@ -9582,6 +9570,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;
@@ -9725,6 +9734,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,
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; +} +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;
- 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 f9ab8aa..fd5251b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10056,9 +10056,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,
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 fd5251b..0f4cb20 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2760,74 +2760,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; @@ -9583,6 +9515,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; @@ -9724,7 +9658,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); @@ -11261,11 +11203,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);
/* * We need to make sure the primary plane is re-enabled if it
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Take out the pin_fb code so commit phase can't fail anymore.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0f4cb20..f7ca7ab 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11437,20 +11437,16 @@ intel_check_primary_plane(struct drm_plane *plane, }
static int -intel_commit_primary_plane(struct drm_plane *plane, - struct intel_plane_state *state) +intel_prepare_primary_plane(struct drm_plane *plane, + struct intel_plane_state *state) { struct drm_crtc *crtc = state->crtc; struct drm_framebuffer *fb = state->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 = 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); - struct drm_rect *src = &state->src; int ret;
intel_crtc_wait_for_pending_flips(crtc); @@ -11460,7 +11456,7 @@ intel_commit_primary_plane(struct drm_plane *plane, return -EBUSY; }
- if (plane->fb != fb) { + if (old_obj != obj) { mutex_lock(&dev->struct_mutex); ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); if (ret == 0) @@ -11473,6 +11469,25 @@ intel_commit_primary_plane(struct drm_plane *plane, } }
+ return 0; +} + +static void +intel_commit_primary_plane(struct drm_plane *plane, + struct intel_plane_state *state) +{ + struct drm_crtc *crtc = state->crtc; + struct drm_framebuffer *fb = state->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 = 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); + struct drm_rect *src = &state->src; + crtc->primary->fb = fb; crtc->x = src->x1; crtc->y = src->y1; @@ -11544,8 +11559,6 @@ intel_commit_primary_plane(struct drm_plane *plane, intel_unpin_fb_obj(old_obj); mutex_unlock(&dev->struct_mutex); } - - return 0; }
static int @@ -11586,6 +11599,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret) return ret;
+ ret = intel_prepare_primary_plane(plane, &state); + if (ret) + return ret; + intel_commit_primary_plane(plane, &state);
return 0;
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
take out pin_fb code so the commit phase can't fail anymore.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_sprite.c | 63 +++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 78044bb..e0c2fbc 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1011,34 +1011,18 @@ intel_check_sprite_plane(struct drm_plane *plane, }
static int -intel_commit_sprite_plane(struct drm_plane *plane, - struct intel_plane_state *state) +intel_prepare_sprite_plane(struct drm_plane *plane, + struct intel_plane_state *state) { struct drm_device *dev = plane->dev; struct drm_crtc *crtc = state->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_plane *intel_plane = to_intel_plane(plane); enum pipe pipe = intel_crtc->pipe; struct drm_framebuffer *fb = state->fb; - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); - struct drm_i915_gem_object *obj = intel_fb->obj; - struct drm_i915_gem_object *old_obj = intel_plane->obj; - int crtc_x, crtc_y; - unsigned int crtc_w, crtc_h; - uint32_t src_x, src_y, src_w, src_h; - struct drm_rect *dst = &state->dst; - const struct drm_rect *clip = &state->clip; - bool primary_enabled; + struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct drm_i915_gem_object *old_obj = intel_fb_obj(fb); int ret;
- /* - * If the sprite is completely covering the primary plane, - * we can disable the primary and save power. - */ - primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane); - WARN_ON(!primary_enabled && !state->visible && intel_crtc->active); - - if (old_obj != obj) { mutex_lock(&dev->struct_mutex);
@@ -1057,6 +1041,36 @@ intel_commit_sprite_plane(struct drm_plane *plane, return ret; }
+ return 0; +} + +static void +intel_commit_sprite_plane(struct drm_plane *plane, + struct intel_plane_state *state) +{ + struct drm_device *dev = plane->dev; + struct drm_crtc *crtc = state->crtc; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_plane *intel_plane = to_intel_plane(plane); + enum pipe pipe = intel_crtc->pipe; + struct drm_framebuffer *fb = state->fb; + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); + struct drm_i915_gem_object *obj = intel_fb->obj; + struct drm_i915_gem_object *old_obj = intel_plane->obj; + int crtc_x, crtc_y; + unsigned int crtc_w, crtc_h; + uint32_t src_x, src_y, src_w, src_h; + struct drm_rect *dst = &state->dst; + const struct drm_rect *clip = &state->clip; + bool primary_enabled; + + /* + * If the sprite is completely covering the primary plane, + * we can disable the primary and save power. + */ + primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane); + WARN_ON(!primary_enabled && !state->visible && intel_crtc->active); + 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); @@ -1117,8 +1131,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, intel_unpin_fb_obj(old_obj); mutex_unlock(&dev->struct_mutex); } - - return 0; }
static int @@ -1158,7 +1170,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret) return ret;
- return intel_commit_sprite_plane(plane, &state); + ret = intel_prepare_sprite_plane(plane, &state); + if (ret) + return ret; + + intel_commit_sprite_plane(plane, &state); + return 0; }
static int
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Use the macros makes the code cleaner and it also checks for a NULL fb.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_sprite.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index e0c2fbc..3778d19 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -851,8 +851,7 @@ intel_check_sprite_plane(struct drm_plane *plane, struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_framebuffer *fb = state->fb; - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); - struct drm_i915_gem_object *obj = intel_fb->obj; + struct drm_i915_gem_object *obj = intel_fb_obj(fb); int crtc_x, crtc_y; unsigned int crtc_w, crtc_h; uint32_t src_x, src_y, src_w, src_h; @@ -1054,9 +1053,8 @@ intel_commit_sprite_plane(struct drm_plane *plane, struct intel_plane *intel_plane = to_intel_plane(plane); enum pipe pipe = intel_crtc->pipe; struct drm_framebuffer *fb = state->fb; - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); - struct drm_i915_gem_object *obj = intel_fb->obj; - struct drm_i915_gem_object *old_obj = intel_plane->obj; + struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); int crtc_x, crtc_y; unsigned int crtc_w, crtc_h; uint32_t src_x, src_y, src_w, src_h;
On Mon, Sep 22, 2014 at 07:23:08PM -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)
Woah! Have you fixed the current regressions introduced by the earlier patches?
xrandr <output> --rotate inverted is broken as the set-property calls plane restore without the plane ever being configured, even though the CRTC is active.
A sketchy and completely broken patch: http://patchwork.freedesktop.org/patch/33855/ -Chris
On Mon, Sep 22, 2014 at 07:23:08PM -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
v5: take more Ville's comments:
- set primary_enabled to true and add BDW hack
- unify if (old_fb) and if (old_fb != fb)
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 | 142 +++++++++++++++++++++-------------- 1 file changed, 87 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 858011d..bef37dc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11578,12 +11578,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 @@ -11595,6 +11606,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);
@@ -11603,76 +11616,95 @@ 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) {
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);if (ret != 0) {DRM_DEBUG_KMS("pin & fence failed\n");return ret;}}
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);
intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
intel_plane->src_x = state->orig_src.x1;
intel_plane->src_y = state->orig_src.y1;
intel_plane->src_w = drm_rect_width(&state->orig_src);
intel_plane->src_h = drm_rect_height(&state->orig_src);
intel_plane->obj = obj;
if (intel_crtc->active) { /*
* Try to pin the new fb first so that we can bail out if we* fail.
* 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 (plane->fb != fb) {ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);if (ret) {mutex_unlock(&dev->struct_mutex);return ret;}
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);
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 (state->visible) {int was_enabled = intel_crtc->primary_enabled;
bool
/* FIXME: kill this fastboot hack */intel_update_pipe_size(intel_crtc);
if (plane->fb != fb)if (plane->fb)intel_unpin_fb_obj(old_obj);
intel_crtc->primary_enabled = true;
mutex_unlock(&dev->struct_mutex);
dev_priv->display.update_primary_plane(crtc, plane->fb,crtc->x, crtc->y);
- } else {
if (intel_crtc && intel_crtc->active &&intel_crtc->primary_enabled) {
if (IS_BROADWELL(dev) && was_enabled)
&& !was_enabled
and please bring over the comment from intel_enable_primary_hw_plane() as well so that people don't have to wonder what this vblank wait is doing here.
The rest of the patch looks good, so fix these and you get Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
intel_wait_for_vblank(dev, intel_crtc->pipe);} else { /*
* 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 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 (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);}
intel_disable_primary_hw_plane(plane, crtc);
ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);if (ret)return ret;if (!intel_crtc->primary_enabled)intel_enable_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);
- 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);
- intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
- intel_plane->src_x = state->orig_src.x1;
- intel_plane->src_y = state->orig_src.y1;
- intel_plane->src_w = drm_rect_width(&state->orig_src);
- intel_plane->src_h = drm_rect_height(&state->orig_src);
- intel_plane->obj = obj;
if (old_fb && old_fb != fb) {
if (intel_crtc->active)intel_wait_for_vblank(dev, intel_crtc->pipe);mutex_lock(&dev->struct_mutex);intel_unpin_fb_obj(old_obj);mutex_unlock(&dev->struct_mutex);}
return 0;
}
1.9.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Gustavo Padovan -
Gustavo Padovan -
Ville Syrjälä