* [PATCH 2/5] drm/i915: move checks of intel_crtc_cursor_set_obj() out
2014-09-18 19:43 [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Gustavo Padovan
@ 2014-09-18 19:43 ` Gustavo Padovan
2014-09-19 12:11 ` Ville Syrjälä
2014-09-18 19:43 ` [PATCH 3/5] drm/i915: remove intel_crtc_cursor_set_obj() Gustavo Padovan
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Gustavo Padovan @ 2014-09-18 19:43 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
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.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
1 file changed, 44 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fd9b70..a68befb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8377,7 +8377,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;
@@ -8389,30 +8389,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");
- ret = -ENOMEM;
- goto fail;
- }
-
/* 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
@@ -8488,7 +8469,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;
}
@@ -12039,16 +12019,58 @@ 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;
+
+ crtc_w = drm_rect_width(&state->orig_dst);
+ crtc_h = drm_rect_height(&state->orig_dst);
+
+ /* 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 */
+ 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");
+ ret = -ENOMEM;
+ goto fail;
+ }
+
+ /* 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);
+
+fail:
+ drm_gem_object_unreference_unlocked(&obj->base);
+ return ret;
}
static int
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/5] drm/i915: move checks of intel_crtc_cursor_set_obj() out
2014-09-18 19:43 ` [PATCH 2/5] drm/i915: move checks of intel_crtc_cursor_set_obj() out Gustavo Padovan
@ 2014-09-19 12:11 ` Ville Syrjälä
0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2014-09-19 12:11 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Thu, Sep 18, 2014 at 04:43:13PM -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.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
> 1 file changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1fd9b70..a68befb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8377,7 +8377,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;
>
> @@ -8389,30 +8389,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");
> - ret = -ENOMEM;
> - goto fail;
> - }
> -
> /* 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;
> - }
Hmm. I was going to say this check needs to remain here due to
struct_mutex getting dropped between check() and here. But it looks
like these days obj->framebuffer_references should protect us from
anyone changing the tiling mode while the object is wrapped in
framebuffer. So seems moving it should still work fine.
> -
> /*
> * Global gtt pte registers are special registers which actually
> * forward writes to a chunk of system memory. Which means that
> @@ -8488,7 +8469,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);
That unref looks like a leftover from the days before universal
planes. Should be just removed AFAICS and the stale comments about
reference consumption should be removed. Please send a separate patch
for this stuff.
> return ret;
> }
> @@ -12039,16 +12019,58 @@ 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;
> +
> + crtc_w = drm_rect_width(&state->orig_dst);
> + crtc_h = drm_rect_height(&state->orig_dst);
Could move these a bit later since they're not needed immediately.
> +
> + /* 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 */
> + 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");
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + /* 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);
> +
> +fail:
> + drm_gem_object_unreference_unlocked(&obj->base);
and this bug should not get copied here.
> + return ret;
> }
>
> static int
> --
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/5] drm/i915: remove intel_crtc_cursor_set_obj()
2014-09-18 19:43 [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Gustavo Padovan
2014-09-18 19:43 ` [PATCH 2/5] drm/i915: move checks of intel_crtc_cursor_set_obj() out Gustavo Padovan
@ 2014-09-18 19:43 ` Gustavo Padovan
2014-09-18 19:43 ` [PATCH 4/5] drm/i915: split intel_crtc_page_flip() into check() and commit() Gustavo Padovan
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Gustavo Padovan @ 2014-09-18 19:43 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
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 | 229 ++++++++++++++++-------------------
1 file changed, 106 insertions(+), 123 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a68befb..ad86fa3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8362,117 +8362,6 @@ static bool cursor_size_ok(struct drm_device *dev,
return true;
}
-/*
- * 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
- * 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);
- drm_gem_object_unreference_unlocked(&obj->base);
- return ret;
-}
-
static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
u16 *blue, uint32_t start, uint32_t size)
{
@@ -12011,7 +11900,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
@@ -12078,26 +11968,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
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/5] drm/i915: split intel_crtc_page_flip() into check() and commit()
2014-09-18 19:43 [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Gustavo Padovan
2014-09-18 19:43 ` [PATCH 2/5] drm/i915: move checks of intel_crtc_cursor_set_obj() out Gustavo Padovan
2014-09-18 19:43 ` [PATCH 3/5] drm/i915: remove intel_crtc_cursor_set_obj() Gustavo Padovan
@ 2014-09-18 19:43 ` Gustavo Padovan
2014-09-18 19:43 ` [PATCH 5/5] drm/i915: remove intel_pipe_set_base() Gustavo Padovan
2014-09-19 11:27 ` [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Ville Syrjälä
4 siblings, 0 replies; 10+ messages in thread
From: Gustavo Padovan @ 2014-09-18 19:43 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Stone, dri-devel
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 ad86fa3..6c61c8f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9757,23 +9757,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
@@ -9796,6 +9784,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;
@@ -9939,6 +9948,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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5/5] drm/i915: remove intel_pipe_set_base()
2014-09-18 19:43 [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Gustavo Padovan
` (2 preceding siblings ...)
2014-09-18 19:43 ` [PATCH 4/5] drm/i915: split intel_crtc_page_flip() into check() and commit() Gustavo Padovan
@ 2014-09-18 19:43 ` Gustavo Padovan
2014-09-19 13:51 ` Ville Syrjälä
2014-09-19 11:27 ` [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Ville Syrjälä
4 siblings, 1 reply; 10+ messages in thread
From: Gustavo Padovan @ 2014-09-18 19:43 UTC (permalink / raw)
To: intel-gfx; +Cc: Gustavo Padovan, dri-devel
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().
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 79 ++++--------------------------------
1 file changed, 8 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6c61c8f..2477587 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2763,74 +2763,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;
@@ -9797,6 +9729,7 @@ 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;
enum pipe pipe = intel_crtc->pipe;
struct intel_unpin_work *work;
struct intel_engine_cs *ring;
@@ -9938,7 +9871,9 @@ 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,
+ 0, 0, 0, 0,
+ crtc->x, crtc->y, 0, 0);
if (ret == 0 && event) {
spin_lock_irq(&dev->event_lock);
drm_send_vblank_event(dev, pipe, event);
@@ -11475,11 +11410,13 @@ 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;
intel_crtc_wait_for_pending_flips(set->crtc);
- ret = intel_pipe_set_base(set->crtc,
- set->x, set->y, set->fb);
+ ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
+ 0, 0, 0, 0,
+ set->x, set->y, 0, 0);
/*
* We need to make sure the primary plane is re-enabled if it
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 5/5] drm/i915: remove intel_pipe_set_base()
2014-09-18 19:43 ` [PATCH 5/5] drm/i915: remove intel_pipe_set_base() Gustavo Padovan
@ 2014-09-19 13:51 ` Ville Syrjälä
2014-09-19 21:31 ` Gustavo Padovan
0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2014-09-19 13:51 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Thu, Sep 18, 2014 at 04:43:16PM -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().
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 79 ++++--------------------------------
> 1 file changed, 8 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6c61c8f..2477587 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2763,74 +2763,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;
> @@ -9797,6 +9729,7 @@ 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;
> enum pipe pipe = intel_crtc->pipe;
> struct intel_unpin_work *work;
> struct intel_engine_cs *ring;
> @@ -9938,7 +9871,9 @@ 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,
> + 0, 0, 0, 0,
> + crtc->x, crtc->y, 0, 0);
I think we want page flips to not change the current plane setup so
these should be:
intel_plane->crtc_x/y/w/h
intel_plane->src_x/y/w/h
> if (ret == 0 && event) {
> spin_lock_irq(&dev->event_lock);
> drm_send_vblank_event(dev, pipe, event);
> @@ -11475,11 +11410,13 @@ 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;
>
> intel_crtc_wait_for_pending_flips(set->crtc);
>
> - ret = intel_pipe_set_base(set->crtc,
> - set->x, set->y, set->fb);
> + ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> + 0, 0, 0, 0,
> + set->x, set->y, 0, 0);
And these should really be:
0, 0, hdisplay, vdisplay,
set->x << 16, set->y << 16, hdisplay << 16, vdisplay << 16,
Oh, one extra complication here is the stereo modes, so I think we'll be
needing to borrow the stereo doubling trick from intel_modeset_pipe_config()
to adjust hdisplay/vdisplay when appropriate.
And we'll be needing something also for the full modeset path
so that intel_plane->crtc_* and intel_plane->src_* get initialized
properly. Perhaps it's enough to rip out the fb pin/unpin stuff
from __intel_set_mode() and just replace it with a call to
->update_plane().
>
> /*
> * We need to make sure the primary plane is re-enabled if it
> --
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 5/5] drm/i915: remove intel_pipe_set_base()
2014-09-19 13:51 ` Ville Syrjälä
@ 2014-09-19 21:31 ` Gustavo Padovan
2014-09-22 10:58 ` Ville Syrjälä
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo Padovan @ 2014-09-19 21:31 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Gustavo Padovan, dri-devel
2014-09-19 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Sep 18, 2014 at 04:43:16PM -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().
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 79 ++++--------------------------------
> > 1 file changed, 8 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 6c61c8f..2477587 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2763,74 +2763,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;
> > @@ -9797,6 +9729,7 @@ 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;
> > enum pipe pipe = intel_crtc->pipe;
> > struct intel_unpin_work *work;
> > struct intel_engine_cs *ring;
> > @@ -9938,7 +9871,9 @@ 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,
> > + 0, 0, 0, 0,
> > + crtc->x, crtc->y, 0, 0);
>
> I think we want page flips to not change the current plane setup so
> these should be:
> intel_plane->crtc_x/y/w/h
> intel_plane->src_x/y/w/h
Okay.
>
> > if (ret == 0 && event) {
> > spin_lock_irq(&dev->event_lock);
> > drm_send_vblank_event(dev, pipe, event);
> > @@ -11475,11 +11410,13 @@ 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;
> >
> > intel_crtc_wait_for_pending_flips(set->crtc);
> >
> > - ret = intel_pipe_set_base(set->crtc,
> > - set->x, set->y, set->fb);
> > + ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> > + 0, 0, 0, 0,
> > + set->x, set->y, 0, 0);
>
> And these should really be:
> 0, 0, hdisplay, vdisplay,
> set->x << 16, set->y << 16, hdisplay << 16, vdisplay << 16,
>
> Oh, one extra complication here is the stereo modes, so I think we'll be
> needing to borrow the stereo doubling trick from intel_modeset_pipe_config()
> to adjust hdisplay/vdisplay when appropriate.
I see, so just calling intel_modeset_pipe_config() and getting
pipe_config's pipe_src_w and pipe_src_h will fix the problem?
>
> And we'll be needing something also for the full modeset path
> so that intel_plane->crtc_* and intel_plane->src_* get initialized
> properly. Perhaps it's enough to rip out the fb pin/unpin stuff
> from __intel_set_mode() and just replace it with a call to
> ->update_plane().
I see. Is it okay to update the primary planes from there?
Gustavo
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 5/5] drm/i915: remove intel_pipe_set_base()
2014-09-19 21:31 ` Gustavo Padovan
@ 2014-09-22 10:58 ` Ville Syrjälä
0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2014-09-22 10:58 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Fri, Sep 19, 2014 at 06:31:25PM -0300, Gustavo Padovan wrote:
> 2014-09-19 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>
> > On Thu, Sep 18, 2014 at 04:43:16PM -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().
> > >
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 79 ++++--------------------------------
> > > 1 file changed, 8 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 6c61c8f..2477587 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2763,74 +2763,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;
> > > @@ -9797,6 +9729,7 @@ 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;
> > > enum pipe pipe = intel_crtc->pipe;
> > > struct intel_unpin_work *work;
> > > struct intel_engine_cs *ring;
> > > @@ -9938,7 +9871,9 @@ 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,
> > > + 0, 0, 0, 0,
> > > + crtc->x, crtc->y, 0, 0);
> >
> > I think we want page flips to not change the current plane setup so
> > these should be:
> > intel_plane->crtc_x/y/w/h
> > intel_plane->src_x/y/w/h
>
> Okay.
>
> >
> > > if (ret == 0 && event) {
> > > spin_lock_irq(&dev->event_lock);
> > > drm_send_vblank_event(dev, pipe, event);
> > > @@ -11475,11 +11410,13 @@ 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;
> > >
> > > intel_crtc_wait_for_pending_flips(set->crtc);
> > >
> > > - ret = intel_pipe_set_base(set->crtc,
> > > - set->x, set->y, set->fb);
> > > + ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> > > + 0, 0, 0, 0,
> > > + set->x, set->y, 0, 0);
> >
> > And these should really be:
> > 0, 0, hdisplay, vdisplay,
> > set->x << 16, set->y << 16, hdisplay << 16, vdisplay << 16,
> >
> > Oh, one extra complication here is the stereo modes, so I think we'll be
> > needing to borrow the stereo doubling trick from intel_modeset_pipe_config()
> > to adjust hdisplay/vdisplay when appropriate.
>
> I see, so just calling intel_modeset_pipe_config() and getting
> pipe_config's pipe_src_w and pipe_src_h will fix the problem?
No, we shouldn't call intel_modeset_pipe_config() here. You just need to
extract the code to compute pipe_src_w/h when dealing with a doubled
stereo mode. We also use the same trick in drm_crtc_check_viewport().
That's three instances of the same thing basically so maybe we should put
that bit of code into a small helper function and just call that from all
three places.
>
> >
> > And we'll be needing something also for the full modeset path
> > so that intel_plane->crtc_* and intel_plane->src_* get initialized
> > properly. Perhaps it's enough to rip out the fb pin/unpin stuff
> > from __intel_set_mode() and just replace it with a call to
> > ->update_plane().
>
> I see. Is it okay to update the primary planes from there?
At that point the pipe should be disabled, so in theory the
.update_plane() should just end up doing pin+unpin+update the
saved plane coords. But I might be missing something subtle here.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes
2014-09-18 19:43 [PATCH 1/5] drm/i915: Merge of visible and !visible paths for primary planes Gustavo Padovan
` (3 preceding siblings ...)
2014-09-18 19:43 ` [PATCH 5/5] drm/i915: remove intel_pipe_set_base() Gustavo Padovan
@ 2014-09-19 11:27 ` Ville Syrjälä
4 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2014-09-19 11:27 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: intel-gfx, Gustavo Padovan, dri-devel
On Thu, Sep 18, 2014 at 04:43:12PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> Fold intel_pipe_set_base() in the update primary plane path merging
> pieces of code that are common to both paths.
>
> Basically the the pin/unpin procedures are the same for both paths
> and some checks can also be shared (some of the were moved to the
> check() stage)
>
> v2: take Ville's comments:
> - remove unnecessary plane check
> - move mutex lock to inside the conditional
> - make the pin fail message a debug one
> - add a fixme for the fastboot hack
> - call intel_frontbuffer_flip() after FBC update
>
> v3: take more Ville's comments:
> - fold update code under if (intel_crtc->active), and do the
> visible/!visible split inside.
> - check ret inside the same conditional we assign it
>
> v4: don't use intel_enable_primary_hw_plane(), the primary_enabled
> check inside will break page flips
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 141 +++++++++++++++++++++--------------
> 1 file changed, 84 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5b05ddb..1fd9b70 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11792,12 +11792,23 @@ intel_check_primary_plane(struct drm_plane *plane,
> struct drm_rect *dest = &state->dst;
> struct drm_rect *src = &state->src;
> const struct drm_rect *clip = &state->clip;
> + int ret;
>
> - return drm_plane_helper_check_update(plane, crtc, fb,
> + ret = drm_plane_helper_check_update(plane, crtc, fb,
> src, dest, clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, true, &state->visible);
> + if (ret)
> + return ret;
> +
> + /* no fb bound */
> + if (state->visible && !fb) {
> + DRM_ERROR("No FB bound\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> static int
> @@ -11809,6 +11820,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + enum pipe pipe = intel_crtc->pipe;
> + struct drm_framebuffer *old_fb = plane->fb;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -11817,67 +11830,28 @@ intel_commit_primary_plane(struct drm_plane *plane,
>
> intel_crtc_wait_for_pending_flips(crtc);
>
> - /*
> - * If clipping results in a non-visible primary plane, we'll disable
> - * the primary plane. Note that this is a bit different than what
> - * happens if userspace explicitly disables the plane by passing fb=0
> - * because plane->fb still gets set and pinned.
> - */
> - if (!state->visible) {
> - mutex_lock(&dev->struct_mutex);
> -
> - /*
> - * Try to pin the new fb first so that we can bail out if we
> - * fail.
> - */
> - if (plane->fb != fb) {
> - ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> - if (ret) {
> - mutex_unlock(&dev->struct_mutex);
> - return ret;
> - }
> - }
> -
> - i915_gem_track_fb(old_obj, obj,
> - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -
> - if (intel_crtc->primary_enabled)
> - intel_disable_primary_hw_plane(plane, crtc);
> -
> -
> - if (plane->fb != fb)
> - if (plane->fb)
> - intel_unpin_fb_obj(old_obj);
> + if (intel_crtc_has_pending_flip(crtc)) {
> + DRM_ERROR("pipe is still busy with an old pageflip\n");
> + return -EBUSY;
> + }
>
> + if (plane->fb != fb) {
> + mutex_lock(&dev->struct_mutex);
> + ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> + if (ret == 0)
> + i915_gem_track_fb(old_obj, obj,
> + INTEL_FRONTBUFFER_PRIMARY(pipe));
> mutex_unlock(&dev->struct_mutex);
> -
> - } else {
> - if (intel_crtc && intel_crtc->active &&
> - intel_crtc->primary_enabled) {
> - /*
> - * FBC does not work on some platforms for rotated
> - * planes, so disable it when rotation is not 0 and
> - * update it when rotation is set back to 0.
> - *
> - * FIXME: This is redundant with the fbc update done in
> - * the primary plane enable function except that that
> - * one is done too late. We eventually need to unify
> - * this.
> - */
> - if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> - dev_priv->fbc.plane == intel_crtc->plane &&
> - intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> - intel_disable_fbc(dev);
> - }
> - }
> - ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
> - if (ret)
> + if (ret != 0) {
> + DRM_DEBUG_KMS("pin & fence failed\n");
> return ret;
> -
> - if (!intel_crtc->primary_enabled)
> - intel_enable_primary_hw_plane(plane, crtc);
> + }
> }
>
> + crtc->primary->fb = fb;
> + crtc->x = src->x1;
> + crtc->y = src->y1;
> +
> intel_plane->crtc_x = state->orig_dst.x1;
> intel_plane->crtc_y = state->orig_dst.y1;
> intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> @@ -11888,6 +11862,59 @@ intel_commit_primary_plane(struct drm_plane *plane,
> intel_plane->src_h = drm_rect_height(&state->orig_src);
> intel_plane->obj = obj;
>
> + if (intel_crtc->active) {
> + /*
> + * FBC does not work on some platforms for rotated
> + * planes, so disable it when rotation is not 0 and
> + * update it when rotation is set back to 0.
> + *
> + * FIXME: This is redundant with the fbc update done in
> + * the primary plane enable function except that that
> + * one is done too late. We eventually need to unify
> + * this.
> + */
> + if (intel_crtc->primary_enabled &&
> + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> + dev_priv->fbc.plane == intel_crtc->plane &&
> + intel_plane->rotation != BIT(DRM_ROTATE_0)) {
> + intel_disable_fbc(dev);
> + }
> +
> + if (state->visible) {
> + /* FIXME: kill this fastboot hack */
> + intel_update_pipe_size(intel_crtc);
> +
Need to set primary_enabled=true here.
> + dev_priv->display.update_primary_plane(crtc, plane->fb,
> + crtc->x, crtc->y);
And you need to duplicate the BDW wait_for_vblank hack from
intel_enable_primary_hw_plane() here. But that wait should only be
done if the plane was previosuly disabled.
> + } else {
> + /*
> + * If clipping results in a non-visible primary plane,
> + * we'll disable the primary plane. Note that this is
> + * a bit different than what happens if userspace
> + * explicitly disables the plane by passing fb=0
> + * because plane->fb still gets set and pinned.
> + */
> + intel_disable_primary_hw_plane(plane, crtc);
> + }
> +
> + intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> +
> + mutex_lock(&dev->struct_mutex);
> + intel_update_fbc(dev);
> + mutex_unlock(&dev->struct_mutex);
> + }
> +
> + if (old_fb) {
Still could unify the old_fb!=fb checks with this one.
if (old_fb && old_fb != fb) {
> + if (intel_crtc->active && old_fb != fb)
> + intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> + if (old_fb != fb) {
> + mutex_lock(&dev->struct_mutex);
> + intel_unpin_fb_obj(old_obj);
> + mutex_unlock(&dev->struct_mutex);
> + }
> + }
> +
> return 0;
> }
>
> --
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 10+ messages in thread