* [PATCH 00/10] Nuke some legacy state.
@ 2015-09-10 14:07 Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update Maarten Lankhorst
` (9 more replies)
0 siblings, 10 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:07 UTC (permalink / raw)
To: intel-gfx
This series reduces the dependency on legacy state, and replaces it
with atomic state where possible.
Maarten Lankhorst (10):
drm/i915: Use atomic plane state in the primary plane update.
drm/i915: Use the plane state in intel_crtc_info.
drm/i915: Use the atomic state in intel_update_primary_planes.
drm/i915: Use atomic state when changing cursor visibility.
drm/i915: Remove legacy plane updates for cursor and sprite planes.
drm/i915: Update legacy primary state outside the commit hook.
drm/i915: Do not handle a null plane state.
drm/i915: Use crtc->state for duplication.
drm/i915: Kill off a user of update_state_fb.
drm/i915: Only run commit when crtc is active.
drivers/gpu/drm/i915/i915_debugfs.c | 8 +--
drivers/gpu/drm/i915/intel_atomic.c | 9 +---
drivers/gpu/drm/i915/intel_atomic_plane.c | 6 +--
drivers/gpu/drm/i915/intel_display.c | 82 ++++++++++++++-----------------
drivers/gpu/drm/i915/intel_sprite.c | 5 --
5 files changed, 45 insertions(+), 65 deletions(-)
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
@ 2015-09-10 14:07 ` Maarten Lankhorst
2015-09-14 13:27 ` Ville Syrjälä
2015-09-10 14:07 ` [PATCH 02/10] drm/i915: Use the plane state in intel_crtc_info Maarten Lankhorst
` (8 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:07 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 80a98bffd5ba..33200403a5db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13448,7 +13448,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
if (!crtc->state->active)
return;
- dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
+ dev_priv->display.update_primary_plane(crtc, fb,
+ state->src.x1 >> 16,
+ state->src.y1 >> 16);
}
static void
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 02/10] drm/i915: Use the plane state in intel_crtc_info.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update Maarten Lankhorst
@ 2015-09-10 14:07 ` Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes Maarten Lankhorst
` (7 subsequent siblings)
9 siblings, 0 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:07 UTC (permalink / raw)
To: intel-gfx
Legacy state might not be updated any more.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 41629faaf939..72ae3472ddbe 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2817,11 +2817,13 @@ static void intel_crtc_info(struct seq_file *m, struct intel_crtc *intel_crtc)
struct drm_device *dev = node->minor->dev;
struct drm_crtc *crtc = &intel_crtc->base;
struct intel_encoder *intel_encoder;
+ struct drm_plane_state *plane_state = crtc->primary->state;
+ struct drm_framebuffer *fb = plane_state->fb;
- if (crtc->primary->fb)
+ if (fb)
seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d\n",
- crtc->primary->fb->base.id, crtc->x, crtc->y,
- crtc->primary->fb->width, crtc->primary->fb->height);
+ fb->base.id, plane_state->src_x >> 16,
+ plane_state->src_y >> 16, fb->width, fb->height);
else
seq_puts(m, "\tprimary plane disabled\n");
for_each_encoder_on_crtc(dev, crtc, intel_encoder)
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 02/10] drm/i915: Use the plane state in intel_crtc_info Maarten Lankhorst
@ 2015-09-10 14:07 ` Maarten Lankhorst
2015-09-10 15:32 ` Daniel Vetter
2015-09-10 15:43 ` Ville Syrjälä
2015-09-10 14:07 ` [PATCH 04/10] drm/i915: Use atomic state when changing cursor visibility Maarten Lankhorst
` (6 subsequent siblings)
9 siblings, 2 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:07 UTC (permalink / raw)
To: intel-gfx
This function was still using the legacy state, convert it to atomic.
While we're at it, fix the FIXME too and disable the primary plane.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 33200403a5db..b68aa95c5460 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
static void intel_update_primary_planes(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_crtc *crtc;
for_each_crtc(dev, crtc) {
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_plane *plane = to_intel_plane(crtc->primary);
+ struct intel_plane_state *plane_state;
- drm_modeset_lock(&crtc->mutex, NULL);
- /*
- * FIXME: Once we have proper support for primary planes (and
- * disabling them without disabling the entire crtc) allow again
- * a NULL crtc->primary->fb.
- */
- if (intel_crtc->active && crtc->primary->fb)
- dev_priv->display.update_primary_plane(crtc,
- crtc->primary->fb,
- crtc->x,
- crtc->y);
- drm_modeset_unlock(&crtc->mutex);
+ drm_modeset_lock_crtc(crtc, &plane->base);
+
+ plane_state = to_intel_plane_state(plane->base.state);
+
+ if (plane_state->base.fb)
+ plane->commit_plane(&plane->base, plane_state);
+ else if (crtc->state->active)
+ plane->disable_plane(&plane->base, crtc);
+
+ drm_modeset_unlock_crtc(crtc);
}
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 04/10] drm/i915: Use atomic state when changing cursor visibility.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (2 preceding siblings ...)
2015-09-10 14:07 ` [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes Maarten Lankhorst
@ 2015-09-10 14:07 ` Maarten Lankhorst
2015-09-10 15:46 ` Ville Syrjälä
2015-09-10 14:08 ` [PATCH 05/10] drm/i915: Remove legacy plane updates for cursor and sprite planes Maarten Lankhorst
` (5 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:07 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b68aa95c5460..bad22cb828c0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9926,8 +9926,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc->pipe;
- int x = crtc->cursor_x;
- int y = crtc->cursor_y;
+ struct drm_plane_state *cursor_state = crtc->cursor->state;
+ int x = cursor_state->crtc_x;
+ int y = cursor_state->crtc_y;
u32 base = 0, pos = 0;
if (on)
@@ -9940,7 +9941,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
base = 0;
if (x < 0) {
- if (x + intel_crtc->base.cursor->state->crtc_w <= 0)
+ if (x + cursor_state->crtc_w <= 0)
base = 0;
pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
@@ -9949,7 +9950,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
pos |= x << CURSOR_X_SHIFT;
if (y < 0) {
- if (y + intel_crtc->base.cursor->state->crtc_h <= 0)
+ if (y + cursor_state->crtc_h <= 0)
base = 0;
pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
@@ -9965,8 +9966,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
/* ILK+ do this automagically */
if (HAS_GMCH_DISPLAY(dev) &&
crtc->cursor->state->rotation == BIT(DRM_ROTATE_180)) {
- base += (intel_crtc->base.cursor->state->crtc_h *
- intel_crtc->base.cursor->state->crtc_w - 1) * 4;
+ base += (cursor_state->crtc_h *
+ cursor_state->crtc_w - 1) * 4;
}
if (IS_845G(dev) || IS_I865G(dev))
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 05/10] drm/i915: Remove legacy plane updates for cursor and sprite planes.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (3 preceding siblings ...)
2015-09-10 14:07 ` [PATCH 04/10] drm/i915: Use atomic state when changing cursor visibility Maarten Lankhorst
@ 2015-09-10 14:08 ` Maarten Lankhorst
2015-09-10 15:38 ` Daniel Vetter
2015-09-10 14:08 ` [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook Maarten Lankhorst
` (4 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:08 UTC (permalink / raw)
To: intel-gfx
Unfortunately fbc still depends on legacy primary state, so
it can't be killed off completely yet.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 4 ----
drivers/gpu/drm/i915/intel_sprite.c | 2 --
2 files changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bad22cb828c0..87c5eba08454 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13662,10 +13662,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
crtc = crtc ? crtc : plane->crtc;
intel_crtc = to_intel_crtc(crtc);
- plane->fb = state->base.fb;
- crtc->cursor_x = state->base.crtc_x;
- crtc->cursor_y = state->base.crtc_y;
-
if (intel_crtc->cursor_bo == obj)
goto update;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ca7e26430e66..9553859ca151 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -927,8 +927,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
crtc = crtc ? crtc : plane->crtc;
- plane->fb = fb;
-
if (!crtc->state->active)
return;
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (4 preceding siblings ...)
2015-09-10 14:08 ` [PATCH 05/10] drm/i915: Remove legacy plane updates for cursor and sprite planes Maarten Lankhorst
@ 2015-09-10 14:08 ` Maarten Lankhorst
2015-09-10 15:41 ` Daniel Vetter
2015-09-10 14:08 ` [PATCH 07/10] drm/i915: Do not handle a null plane state Maarten Lankhorst
` (3 subsequent siblings)
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:08 UTC (permalink / raw)
To: intel-gfx
This should allow not running plane commit when the crtc is off.
While the atomic helpers update those, crtc->x/y is only updated
during modesets, and primary plane is updated after this function
returns.
Unfortunately non-atomic watermarks and fbc still depend on this
state inside i915, so it has to be kept in sync.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 87c5eba08454..b809ee2a8678 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
crtc->hwmode = crtc->state->adjusted_mode;
else
crtc->hwmode.crtc_clock = 0;
+
+ if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
+ struct drm_plane_state *plane_state = crtc->primary->state;
+
+ crtc->primary->fb = plane_state->fb;
+ crtc->x = plane_state->src_x >> 16;
+ crtc->y = plane_state->src_y >> 16;
+ }
}
}
@@ -13434,15 +13442,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
struct drm_framebuffer *fb = state->base.fb;
struct drm_device *dev = plane->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_crtc *intel_crtc;
- struct drm_rect *src = &state->src;
crtc = crtc ? crtc : plane->crtc;
- intel_crtc = to_intel_crtc(crtc);
-
- plane->fb = fb;
- crtc->x = src->x1 >> 16;
- crtc->y = src->y1 >> 16;
if (!crtc->state->active)
return;
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 07/10] drm/i915: Do not handle a null plane state.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (5 preceding siblings ...)
2015-09-10 14:08 ` [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook Maarten Lankhorst
@ 2015-09-10 14:08 ` Maarten Lankhorst
2015-09-10 14:08 ` [PATCH 08/10] drm/i915: Use crtc->state for duplication Maarten Lankhorst
` (2 subsequent siblings)
9 siblings, 0 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:08 UTC (permalink / raw)
To: intel-gfx
With the conversion to atomic this cannot happen any more.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_atomic_plane.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index f1ab8e4b9c11..a11980696595 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -76,11 +76,7 @@ intel_plane_duplicate_state(struct drm_plane *plane)
struct drm_plane_state *state;
struct intel_plane_state *intel_state;
- if (WARN_ON(!plane->state))
- intel_state = intel_create_plane_state(plane);
- else
- intel_state = kmemdup(plane->state, sizeof(*intel_state),
- GFP_KERNEL);
+ intel_state = kmemdup(plane->state, sizeof(*intel_state), GFP_KERNEL);
if (!intel_state)
return NULL;
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 08/10] drm/i915: Use crtc->state for duplication.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (6 preceding siblings ...)
2015-09-10 14:08 ` [PATCH 07/10] drm/i915: Do not handle a null plane state Maarten Lankhorst
@ 2015-09-10 14:08 ` Maarten Lankhorst
2015-09-10 15:46 ` Daniel Vetter
2015-09-10 14:08 ` [PATCH 09/10] drm/i915: Kill off a user of update_state_fb Maarten Lankhorst
2015-09-10 14:08 ` [PATCH 10/10] drm/i915: Only run commit when crtc is active Maarten Lankhorst
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:08 UTC (permalink / raw)
To: intel-gfx
In async mode crtc->config can be updated after the locks are released,
resulting in the wrong state being duplicated.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_atomic.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 8287b81287a0..9d5cc89ebfcb 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -85,20 +85,13 @@ intel_connector_atomic_get_property(struct drm_connector *connector,
struct drm_crtc_state *
intel_crtc_duplicate_state(struct drm_crtc *crtc)
{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_crtc_state *crtc_state;
- if (WARN_ON(!intel_crtc->config))
- crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
- else
- crtc_state = kmemdup(intel_crtc->config,
- sizeof(*intel_crtc->config), GFP_KERNEL);
-
+ crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
if (!crtc_state)
return NULL;
__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->base);
- crtc_state->base.crtc = crtc;
crtc_state->update_pipe = false;
return &crtc_state->base;
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 09/10] drm/i915: Kill off a user of update_state_fb.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (7 preceding siblings ...)
2015-09-10 14:08 ` [PATCH 08/10] drm/i915: Use crtc->state for duplication Maarten Lankhorst
@ 2015-09-10 14:08 ` Maarten Lankhorst
2015-09-10 15:50 ` Daniel Vetter
2015-09-10 14:08 ` [PATCH 10/10] drm/i915: Only run commit when crtc is active Maarten Lankhorst
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:08 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b809ee2a8678..f41ca558ba3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15353,9 +15353,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
DRM_ERROR("failed to pin boot fb on pipe %d\n",
to_intel_crtc(c)->pipe);
drm_framebuffer_unreference(c->primary->fb);
- c->primary->fb = NULL;
+ drm_framebuffer_unreference(c->primary->state->fb);
+ c->primary->fb = c->primary->state->fb = NULL;
c->primary->crtc = c->primary->state->crtc = NULL;
- update_state_fb(c->primary);
c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
}
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 10/10] drm/i915: Only run commit when crtc is active.
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
` (8 preceding siblings ...)
2015-09-10 14:08 ` [PATCH 09/10] drm/i915: Kill off a user of update_state_fb Maarten Lankhorst
@ 2015-09-10 14:08 ` Maarten Lankhorst
2015-09-10 15:51 ` Daniel Vetter
9 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 14:08 UTC (permalink / raw)
To: intel-gfx
The crtc->active guards are no longer needed now that all state
updates are outside the commit.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 16 ++++++----------
drivers/gpu/drm/i915/intel_sprite.c | 3 ---
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f41ca558ba3b..dc696ec5f228 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13135,7 +13135,9 @@ static int intel_atomic_commit(struct drm_device *dev,
if (!modeset)
intel_pre_plane_update(intel_crtc);
- drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+ if (crtc->state->active && (crtc->state->planes_changed ||
+ to_intel_crtc_state(crtc->state)->update_pipe))
+ drm_atomic_helper_commit_planes_on_crtc(crtc_state);
if (put_domains)
modeset_put_power_domains(dev_priv, put_domains);
@@ -13445,9 +13447,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
crtc = crtc ? crtc : plane->crtc;
- if (!crtc->state->active)
- return;
-
dev_priv->display.update_primary_plane(crtc, fb,
state->src.x1 >> 16,
state->src.y1 >> 16);
@@ -13476,8 +13475,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
intel_update_watermarks(crtc);
/* Perform vblank evasion around commit operation */
- if (crtc->state->active)
- intel_pipe_update_start(intel_crtc);
+ intel_pipe_update_start(intel_crtc);
if (modeset)
return;
@@ -13493,8 +13491,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
{
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- if (crtc->state->active)
- intel_pipe_update_end(intel_crtc);
+ intel_pipe_update_end(intel_crtc);
}
/**
@@ -13677,8 +13674,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
intel_crtc->cursor_bo = obj;
update:
- if (crtc->state->active)
- intel_crtc_update_cursor(crtc, state->visible);
+ intel_crtc_update_cursor(crtc, state->visible);
}
static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9553859ca151..9c13d8156135 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -927,9 +927,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
crtc = crtc ? crtc : plane->crtc;
- if (!crtc->state->active)
- return;
-
if (state->visible) {
intel_plane->update_plane(plane, crtc, fb,
state->dst.x1, state->dst.y1,
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
2015-09-10 14:07 ` [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes Maarten Lankhorst
@ 2015-09-10 15:32 ` Daniel Vetter
2015-09-10 15:43 ` Ville Syrjälä
1 sibling, 0 replies; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:32 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> This function was still using the legacy state, convert it to atomic.
> While we're at it, fix the FIXME too and disable the primary plane.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 33200403a5db..b68aa95c5460 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
>
> static void intel_update_primary_planes(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
>
> for_each_crtc(dev, crtc) {
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_plane *plane = to_intel_plane(crtc->primary);
> + struct intel_plane_state *plane_state;
>
> - drm_modeset_lock(&crtc->mutex, NULL);
> - /*
> - * FIXME: Once we have proper support for primary planes (and
> - * disabling them without disabling the entire crtc) allow again
> - * a NULL crtc->primary->fb.
> - */
We probably should have a new FIXME here to ditch this all once we use
atomic for legacy page_flips (since the only reason we have this is when
CS flips are lost in the reset).
-Daniel
> - if (intel_crtc->active && crtc->primary->fb)
> - dev_priv->display.update_primary_plane(crtc,
> - crtc->primary->fb,
> - crtc->x,
> - crtc->y);
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_lock_crtc(crtc, &plane->base);
> +
> + plane_state = to_intel_plane_state(plane->base.state);
> +
> + if (plane_state->base.fb)
> + plane->commit_plane(&plane->base, plane_state);
> + else if (crtc->state->active)
> + plane->disable_plane(&plane->base, crtc);
> +
> + drm_modeset_unlock_crtc(crtc);
> }
> }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 05/10] drm/i915: Remove legacy plane updates for cursor and sprite planes.
2015-09-10 14:08 ` [PATCH 05/10] drm/i915: Remove legacy plane updates for cursor and sprite planes Maarten Lankhorst
@ 2015-09-10 15:38 ` Daniel Vetter
0 siblings, 0 replies; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:38 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:08:00PM +0200, Maarten Lankhorst wrote:
> Unfortunately fbc still depends on legacy primary state, so
> it can't be killed off completely yet.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Wont it be enough that the helpers will update these legacy states for us?
And I didn't spot anything else really using them ...
What I did spot though is update_state_fb - the core should take care of
that now too for us since
commit 3cb43cc0b408c4672ba94fe28406a90a94b1edfe
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Tue Jul 7 08:43:03 2015 +0200
drm: Update plane->fb also for page_flip
Can you please have a testcase on top to remove this one too?
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 ----
> drivers/gpu/drm/i915/intel_sprite.c | 2 --
> 2 files changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bad22cb828c0..87c5eba08454 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13662,10 +13662,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> crtc = crtc ? crtc : plane->crtc;
> intel_crtc = to_intel_crtc(crtc);
>
> - plane->fb = state->base.fb;
> - crtc->cursor_x = state->base.crtc_x;
> - crtc->cursor_y = state->base.crtc_y;
> -
> if (intel_crtc->cursor_bo == obj)
> goto update;
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ca7e26430e66..9553859ca151 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -927,8 +927,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>
> crtc = crtc ? crtc : plane->crtc;
>
> - plane->fb = fb;
> -
> if (!crtc->state->active)
> return;
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-10 14:08 ` [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook Maarten Lankhorst
@ 2015-09-10 15:41 ` Daniel Vetter
2015-09-14 9:52 ` Maarten Lankhorst
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:41 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote:
> This should allow not running plane commit when the crtc is off.
> While the atomic helpers update those, crtc->x/y is only updated
> during modesets, and primary plane is updated after this function
> returns.
>
> Unfortunately non-atomic watermarks and fbc still depend on this
> state inside i915, so it has to be kept in sync.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 87c5eba08454..b809ee2a8678 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
> crtc->hwmode = crtc->state->adjusted_mode;
> else
> crtc->hwmode.crtc_clock = 0;
> +
> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
> + struct drm_plane_state *plane_state = crtc->primary->state;
> +
> + crtc->primary->fb = plane_state->fb;
> + crtc->x = plane_state->src_x >> 16;
> + crtc->y = plane_state->src_y >> 16;
> + }
drm_atomic_helper_update_legacy_modeset_state should do that for us, not?
Atm we have an if (any_ms) check for it, but that seems to just be a bug
really.
-Daniel
> }
> }
>
> @@ -13434,15 +13442,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
> struct drm_framebuffer *fb = state->base.fb;
> struct drm_device *dev = plane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc;
> - struct drm_rect *src = &state->src;
>
> crtc = crtc ? crtc : plane->crtc;
> - intel_crtc = to_intel_crtc(crtc);
> -
> - plane->fb = fb;
> - crtc->x = src->x1 >> 16;
> - crtc->y = src->y1 >> 16;
>
> if (!crtc->state->active)
> return;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
2015-09-10 14:07 ` [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes Maarten Lankhorst
2015-09-10 15:32 ` Daniel Vetter
@ 2015-09-10 15:43 ` Ville Syrjälä
2015-09-10 16:31 ` Daniel Vetter
1 sibling, 1 reply; 37+ messages in thread
From: Ville Syrjälä @ 2015-09-10 15:43 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> This function was still using the legacy state, convert it to atomic.
> While we're at it, fix the FIXME too and disable the primary plane.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 33200403a5db..b68aa95c5460 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
>
> static void intel_update_primary_planes(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
>
> for_each_crtc(dev, crtc) {
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_plane *plane = to_intel_plane(crtc->primary);
> + struct intel_plane_state *plane_state;
>
> - drm_modeset_lock(&crtc->mutex, NULL);
> - /*
> - * FIXME: Once we have proper support for primary planes (and
> - * disabling them without disabling the entire crtc) allow again
> - * a NULL crtc->primary->fb.
> - */
> - if (intel_crtc->active && crtc->primary->fb)
> - dev_priv->display.update_primary_plane(crtc,
> - crtc->primary->fb,
> - crtc->x,
> - crtc->y);
> - drm_modeset_unlock(&crtc->mutex);
> + drm_modeset_lock_crtc(crtc, &plane->base);
> +
> + plane_state = to_intel_plane_state(plane->base.state);
> +
> + if (plane_state->base.fb)
> + plane->commit_plane(&plane->base, plane_state);
> + else if (crtc->state->active)
> + plane->disable_plane(&plane->base, crtc);
That doesn't make sense. There's no way to disable a plane with a page
flip, so there's simply nothing to do here if the fb is NULL. If we can
trust the plane state to be sane we should just check 'visible' here and
commit the plane in that case.
> +
> + drm_modeset_unlock_crtc(crtc);
> }
> }
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 04/10] drm/i915: Use atomic state when changing cursor visibility.
2015-09-10 14:07 ` [PATCH 04/10] drm/i915: Use atomic state when changing cursor visibility Maarten Lankhorst
@ 2015-09-10 15:46 ` Ville Syrjälä
0 siblings, 0 replies; 37+ messages in thread
From: Ville Syrjälä @ 2015-09-10 15:46 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:07:59PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b68aa95c5460..bad22cb828c0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9926,8 +9926,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> - int x = crtc->cursor_x;
> - int y = crtc->cursor_y;
> + struct drm_plane_state *cursor_state = crtc->cursor->state;
> + int x = cursor_state->crtc_x;
> + int y = cursor_state->crtc_y;
> u32 base = 0, pos = 0;
>
> if (on)
> @@ -9940,7 +9941,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> base = 0;
>
> if (x < 0) {
> - if (x + intel_crtc->base.cursor->state->crtc_w <= 0)
> + if (x + cursor_state->crtc_w <= 0)
> base = 0;
>
> pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT;
> @@ -9949,7 +9950,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> pos |= x << CURSOR_X_SHIFT;
>
> if (y < 0) {
> - if (y + intel_crtc->base.cursor->state->crtc_h <= 0)
> + if (y + cursor_state->crtc_h <= 0)
> base = 0;
This whole stuff should be ripped out, and instead we should make sure
the plane state is sane (ie. 'visible' is set when the cursor must be
visible, and not set when the cursor must not be visible).
>
> pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT;
> @@ -9965,8 +9966,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> /* ILK+ do this automagically */
> if (HAS_GMCH_DISPLAY(dev) &&
> crtc->cursor->state->rotation == BIT(DRM_ROTATE_180)) {
> - base += (intel_crtc->base.cursor->state->crtc_h *
> - intel_crtc->base.cursor->state->crtc_w - 1) * 4;
> + base += (cursor_state->crtc_h *
> + cursor_state->crtc_w - 1) * 4;
> }
>
> if (IS_845G(dev) || IS_I865G(dev))
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 08/10] drm/i915: Use crtc->state for duplication.
2015-09-10 14:08 ` [PATCH 08/10] drm/i915: Use crtc->state for duplication Maarten Lankhorst
@ 2015-09-10 15:46 ` Daniel Vetter
2015-09-10 15:49 ` Daniel Vetter
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:46 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:08:03PM +0200, Maarten Lankhorst wrote:
> In async mode crtc->config can be updated after the locks are released,
> resulting in the wrong state being duplicated.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 8287b81287a0..9d5cc89ebfcb 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -85,20 +85,13 @@ intel_connector_atomic_get_property(struct drm_connector *connector,
> struct drm_crtc_state *
> intel_crtc_duplicate_state(struct drm_crtc *crtc)
> {
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *crtc_state;
>
> - if (WARN_ON(!intel_crtc->config))
> - crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> - else
> - crtc_state = kmemdup(intel_crtc->config,
> - sizeof(*intel_crtc->config), GFP_KERNEL);
> -
> + crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
> if (!crtc_state)
> return NULL;
>
> __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->base);
> - crtc_state->base.crtc = crtc;
This is a bugfix for Ander's patch in 4.3. Please split it out and submit
just that part to Jani. I've dropped this part from the patch I merged to
dinq.
-Daniel
> crtc_state->update_pipe = false;
>
> return &crtc_state->base;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 08/10] drm/i915: Use crtc->state for duplication.
2015-09-10 15:46 ` Daniel Vetter
@ 2015-09-10 15:49 ` Daniel Vetter
0 siblings, 0 replies; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:49 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 05:46:12PM +0200, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 04:08:03PM +0200, Maarten Lankhorst wrote:
> > In async mode crtc->config can be updated after the locks are released,
> > resulting in the wrong state being duplicated.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_atomic.c | 9 +--------
> > 1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > index 8287b81287a0..9d5cc89ebfcb 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -85,20 +85,13 @@ intel_connector_atomic_get_property(struct drm_connector *connector,
> > struct drm_crtc_state *
> > intel_crtc_duplicate_state(struct drm_crtc *crtc)
> > {
> > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > struct intel_crtc_state *crtc_state;
> >
> > - if (WARN_ON(!intel_crtc->config))
> > - crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> > - else
> > - crtc_state = kmemdup(intel_crtc->config,
> > - sizeof(*intel_crtc->config), GFP_KERNEL);
> > -
> > + crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
> > if (!crtc_state)
> > return NULL;
> >
> > __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->base);
> > - crtc_state->base.crtc = crtc;
>
> This is a bugfix for Ander's patch in 4.3. Please split it out and submit
> just that part to Jani. I've dropped this part from the patch I merged to
> dinq.
Argh got confused, that line is in dinq too. I added a commit reference to
your patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 09/10] drm/i915: Kill off a user of update_state_fb.
2015-09-10 14:08 ` [PATCH 09/10] drm/i915: Kill off a user of update_state_fb Maarten Lankhorst
@ 2015-09-10 15:50 ` Daniel Vetter
2015-09-14 9:56 ` Maarten Lankhorst
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:50 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:08:04PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Not sure this is worth it ... I'll punt for now. But if you add a bit of
justification that we could get rid of update_state_fb entirely it would
look better ;-)
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b809ee2a8678..f41ca558ba3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15353,9 +15353,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
> DRM_ERROR("failed to pin boot fb on pipe %d\n",
> to_intel_crtc(c)->pipe);
> drm_framebuffer_unreference(c->primary->fb);
> - c->primary->fb = NULL;
> + drm_framebuffer_unreference(c->primary->state->fb);
> + c->primary->fb = c->primary->state->fb = NULL;
> c->primary->crtc = c->primary->state->crtc = NULL;
> - update_state_fb(c->primary);
> c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
> }
> }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 10/10] drm/i915: Only run commit when crtc is active.
2015-09-10 14:08 ` [PATCH 10/10] drm/i915: Only run commit when crtc is active Maarten Lankhorst
@ 2015-09-10 15:51 ` Daniel Vetter
2015-09-14 10:02 ` Maarten Lankhorst
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:51 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:08:05PM +0200, Maarten Lankhorst wrote:
> The crtc->active guards are no longer needed now that all state
> updates are outside the commit.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
This looks actually complicated, so I'll punt. Merged most of the other
patches from your series, thanks.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 16 ++++++----------
> drivers/gpu/drm/i915/intel_sprite.c | 3 ---
> 2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f41ca558ba3b..dc696ec5f228 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13135,7 +13135,9 @@ static int intel_atomic_commit(struct drm_device *dev,
> if (!modeset)
> intel_pre_plane_update(intel_crtc);
>
> - drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> + if (crtc->state->active && (crtc->state->planes_changed ||
> + to_intel_crtc_state(crtc->state)->update_pipe))
> + drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>
> if (put_domains)
> modeset_put_power_domains(dev_priv, put_domains);
> @@ -13445,9 +13447,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>
> crtc = crtc ? crtc : plane->crtc;
>
> - if (!crtc->state->active)
> - return;
> -
> dev_priv->display.update_primary_plane(crtc, fb,
> state->src.x1 >> 16,
> state->src.y1 >> 16);
> @@ -13476,8 +13475,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> intel_update_watermarks(crtc);
>
> /* Perform vblank evasion around commit operation */
> - if (crtc->state->active)
> - intel_pipe_update_start(intel_crtc);
> + intel_pipe_update_start(intel_crtc);
>
> if (modeset)
> return;
> @@ -13493,8 +13491,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> - if (crtc->state->active)
> - intel_pipe_update_end(intel_crtc);
> + intel_pipe_update_end(intel_crtc);
> }
>
> /**
> @@ -13677,8 +13674,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> intel_crtc->cursor_bo = obj;
>
> update:
> - if (crtc->state->active)
> - intel_crtc_update_cursor(crtc, state->visible);
> + intel_crtc_update_cursor(crtc, state->visible);
> }
>
> static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9553859ca151..9c13d8156135 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -927,9 +927,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>
> crtc = crtc ? crtc : plane->crtc;
>
> - if (!crtc->state->active)
> - return;
> -
> if (state->visible) {
> intel_plane->update_plane(plane, crtc, fb,
> state->dst.x1, state->dst.y1,
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
2015-09-10 15:43 ` Ville Syrjälä
@ 2015-09-10 16:31 ` Daniel Vetter
2015-09-10 16:34 ` Ville Syrjälä
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-10 16:31 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> > This function was still using the legacy state, convert it to atomic.
> > While we're at it, fix the FIXME too and disable the primary plane.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> > 1 file changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 33200403a5db..b68aa95c5460 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> >
> > static void intel_update_primary_planes(struct drm_device *dev)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_crtc *crtc;
> >
> > for_each_crtc(dev, crtc) {
> > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > + struct intel_plane *plane = to_intel_plane(crtc->primary);
> > + struct intel_plane_state *plane_state;
> >
> > - drm_modeset_lock(&crtc->mutex, NULL);
> > - /*
> > - * FIXME: Once we have proper support for primary planes (and
> > - * disabling them without disabling the entire crtc) allow again
> > - * a NULL crtc->primary->fb.
> > - */
> > - if (intel_crtc->active && crtc->primary->fb)
> > - dev_priv->display.update_primary_plane(crtc,
> > - crtc->primary->fb,
> > - crtc->x,
> > - crtc->y);
> > - drm_modeset_unlock(&crtc->mutex);
> > + drm_modeset_lock_crtc(crtc, &plane->base);
> > +
> > + plane_state = to_intel_plane_state(plane->base.state);
> > +
> > + if (plane_state->base.fb)
> > + plane->commit_plane(&plane->base, plane_state);
> > + else if (crtc->state->active)
> > + plane->disable_plane(&plane->base, crtc);
>
> That doesn't make sense. There's no way to disable a plane with a page
> flip, so there's simply nothing to do here if the fb is NULL. If we can
> trust the plane state to be sane we should just check 'visible' here and
> commit the plane in that case.
Right missed that and squash up a fixup that Ville acked on irc. Also
added the FIXME comment too.
-Daniel
>
> > +
> > + drm_modeset_unlock_crtc(crtc);
> > }
> > }
> >
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
2015-09-10 16:31 ` Daniel Vetter
@ 2015-09-10 16:34 ` Ville Syrjälä
2015-09-14 9:02 ` Daniel Vetter
0 siblings, 1 reply; 37+ messages in thread
From: Ville Syrjälä @ 2015-09-10 16:34 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> > > This function was still using the legacy state, convert it to atomic.
> > > While we're at it, fix the FIXME too and disable the primary plane.
> > >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> > > 1 file changed, 12 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 33200403a5db..b68aa95c5460 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> > >
> > > static void intel_update_primary_planes(struct drm_device *dev)
> > > {
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct drm_crtc *crtc;
> > >
> > > for_each_crtc(dev, crtc) {
> > > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > + struct intel_plane *plane = to_intel_plane(crtc->primary);
> > > + struct intel_plane_state *plane_state;
> > >
> > > - drm_modeset_lock(&crtc->mutex, NULL);
> > > - /*
> > > - * FIXME: Once we have proper support for primary planes (and
> > > - * disabling them without disabling the entire crtc) allow again
> > > - * a NULL crtc->primary->fb.
> > > - */
> > > - if (intel_crtc->active && crtc->primary->fb)
> > > - dev_priv->display.update_primary_plane(crtc,
> > > - crtc->primary->fb,
> > > - crtc->x,
> > > - crtc->y);
> > > - drm_modeset_unlock(&crtc->mutex);
> > > + drm_modeset_lock_crtc(crtc, &plane->base);
> > > +
> > > + plane_state = to_intel_plane_state(plane->base.state);
> > > +
> > > + if (plane_state->base.fb)
> > > + plane->commit_plane(&plane->base, plane_state);
> > > + else if (crtc->state->active)
> > > + plane->disable_plane(&plane->base, crtc);
> >
> > That doesn't make sense. There's no way to disable a plane with a page
> > flip, so there's simply nothing to do here if the fb is NULL. If we can
> > trust the plane state to be sane we should just check 'visible' here and
> > commit the plane in that case.
>
> Right missed that and squash up a fixup that Ville acked on irc. Also
> added the FIXME comment too.
Actually I take it back. We just lost the ->active check entirely so now
we're commiting a plane on a disabled pipe potentially.
> -Daniel
>
> >
> > > +
> > > + drm_modeset_unlock_crtc(crtc);
> > > }
> > > }
> > >
> > > --
> > > 2.1.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
2015-09-10 16:34 ` Ville Syrjälä
@ 2015-09-14 9:02 ` Daniel Vetter
2015-09-14 9:41 ` [fixup PATCH] drm/i915: Only commit active planes when updating planes during reset Maarten Lankhorst
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-14 9:02 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 07:34:05PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> > > > This function was still using the legacy state, convert it to atomic.
> > > > While we're at it, fix the FIXME too and disable the primary plane.
> > > >
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> > > > 1 file changed, 12 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 33200403a5db..b68aa95c5460 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> > > >
> > > > static void intel_update_primary_planes(struct drm_device *dev)
> > > > {
> > > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > > struct drm_crtc *crtc;
> > > >
> > > > for_each_crtc(dev, crtc) {
> > > > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > + struct intel_plane *plane = to_intel_plane(crtc->primary);
> > > > + struct intel_plane_state *plane_state;
> > > >
> > > > - drm_modeset_lock(&crtc->mutex, NULL);
> > > > - /*
> > > > - * FIXME: Once we have proper support for primary planes (and
> > > > - * disabling them without disabling the entire crtc) allow again
> > > > - * a NULL crtc->primary->fb.
> > > > - */
> > > > - if (intel_crtc->active && crtc->primary->fb)
> > > > - dev_priv->display.update_primary_plane(crtc,
> > > > - crtc->primary->fb,
> > > > - crtc->x,
> > > > - crtc->y);
> > > > - drm_modeset_unlock(&crtc->mutex);
> > > > + drm_modeset_lock_crtc(crtc, &plane->base);
> > > > +
> > > > + plane_state = to_intel_plane_state(plane->base.state);
> > > > +
> > > > + if (plane_state->base.fb)
> > > > + plane->commit_plane(&plane->base, plane_state);
> > > > + else if (crtc->state->active)
> > > > + plane->disable_plane(&plane->base, crtc);
> > >
> > > That doesn't make sense. There's no way to disable a plane with a page
> > > flip, so there's simply nothing to do here if the fb is NULL. If we can
> > > trust the plane state to be sane we should just check 'visible' here and
> > > commit the plane in that case.
> >
> > Right missed that and squash up a fixup that Ville acked on irc. Also
> > added the FIXME comment too.
>
> Actually I take it back. We just lost the ->active check entirely so now
> we're commiting a plane on a disabled pipe potentially.
Blergh and I tagged the pile already :( Maarten, can you pls send out the
fixup as a separate patch asap?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* [fixup PATCH] drm/i915: Only commit active planes when updating planes during reset.
2015-09-14 9:02 ` Daniel Vetter
@ 2015-09-14 9:41 ` Maarten Lankhorst
2015-09-14 13:12 ` Daniel Vetter
0 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-14 9:41 UTC (permalink / raw)
To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx
In a future commit commit_plane will no longer check if the crtc is active.
To prevent issues with legacy page flips the check should be performed inside
update_primary_planes.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
----
Hey,
Op 14-09-15 om 11:02 schreef Daniel Vetter:
> On Thu, Sep 10, 2015 at 07:34:05PM +0300, Ville Syrjälä wrote:
>> On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote:
>>> On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
>>>> On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
>>>>> This function was still using the legacy state, convert it to atomic.
>>>>> While we're at it, fix the FIXME too and disable the primary plane.
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
>>>>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 33200403a5db..b68aa95c5460 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
>>>>>
>>>>> static void intel_update_primary_planes(struct drm_device *dev)
>>>>> {
>>>>> - struct drm_i915_private *dev_priv = dev->dev_private;
>>>>> struct drm_crtc *crtc;
>>>>>
>>>>> for_each_crtc(dev, crtc) {
>>>>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>>> + struct intel_plane *plane = to_intel_plane(crtc->primary);
>>>>> + struct intel_plane_state *plane_state;
>>>>>
>>>>> - drm_modeset_lock(&crtc->mutex, NULL);
>>>>> - /*
>>>>> - * FIXME: Once we have proper support for primary planes (and
>>>>> - * disabling them without disabling the entire crtc) allow again
>>>>> - * a NULL crtc->primary->fb.
>>>>> - */
>>>>> - if (intel_crtc->active && crtc->primary->fb)
>>>>> - dev_priv->display.update_primary_plane(crtc,
>>>>> - crtc->primary->fb,
>>>>> - crtc->x,
>>>>> - crtc->y);
>>>>> - drm_modeset_unlock(&crtc->mutex);
>>>>> + drm_modeset_lock_crtc(crtc, &plane->base);
>>>>> +
>>>>> + plane_state = to_intel_plane_state(plane->base.state);
>>>>> +
>>>>> + if (plane_state->base.fb)
>>>>> + plane->commit_plane(&plane->base, plane_state);
>>>>> + else if (crtc->state->active)
>>>>> + plane->disable_plane(&plane->base, crtc);
>>>> That doesn't make sense. There's no way to disable a plane with a page
>>>> flip, so there's simply nothing to do here if the fb is NULL. If we can
>>>> trust the plane state to be sane we should just check 'visible' here and
>>>> commit the plane in that case.
>>> Right missed that and squash up a fixup that Ville acked on irc. Also
>>> added the FIXME comment too.
>> Actually I take it back. We just lost the ->active check entirely so now
>> we're commiting a plane on a disabled pipe potentially.
> Blergh and I tagged the pile already :( Maarten, can you pls send out the
> fixup as a separate patch asap?
>
This is not currently an issue because "drm/i915: Only run commit when crtc is active.' is not applied.
Thanks for catching though!
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 452e8f77151d..47d9c0f8b2fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3145,10 +3145,9 @@ static void intel_update_primary_planes(struct drm_device *dev)
struct intel_plane_state *plane_state;
drm_modeset_lock_crtc(crtc, &plane->base);
-
plane_state = to_intel_plane_state(plane->base.state);
- if (plane_state->base.fb)
+ if (crtc->state->active && plane_state->base.fb)
plane->commit_plane(&plane->base, plane_state);
drm_modeset_unlock_crtc(crtc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-10 15:41 ` Daniel Vetter
@ 2015-09-14 9:52 ` Maarten Lankhorst
2015-09-14 13:13 ` Daniel Vetter
0 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-14 9:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 10-09-15 om 17:41 schreef Daniel Vetter:
> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote:
>> This should allow not running plane commit when the crtc is off.
>> While the atomic helpers update those, crtc->x/y is only updated
>> during modesets, and primary plane is updated after this function
>> returns.
>>
>> Unfortunately non-atomic watermarks and fbc still depend on this
>> state inside i915, so it has to be kept in sync.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 87c5eba08454..b809ee2a8678 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>> crtc->hwmode = crtc->state->adjusted_mode;
>> else
>> crtc->hwmode.crtc_clock = 0;
>> +
>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
>> + struct drm_plane_state *plane_state = crtc->primary->state;
>> +
>> + crtc->primary->fb = plane_state->fb;
>> + crtc->x = plane_state->src_x >> 16;
>> + crtc->y = plane_state->src_y >> 16;
>> + }
> drm_atomic_helper_update_legacy_modeset_state should do that for us, not?
> Atm we have an if (any_ms) check for it, but that seems to just be a bug
> really.
That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit
after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state.
There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 09/10] drm/i915: Kill off a user of update_state_fb.
2015-09-10 15:50 ` Daniel Vetter
@ 2015-09-14 9:56 ` Maarten Lankhorst
0 siblings, 0 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-14 9:56 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 10-09-15 om 17:50 schreef Daniel Vetter:
> On Thu, Sep 10, 2015 at 04:08:04PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Not sure this is worth it ... I'll punt for now. But if you add a bit of
> justification that we could get rid of update_state_fb entirely it would
> look better ;-)
>
When the page flip handler is converted to atomic this function will finally have no more use. I'm a patient man and can hold off this patch a bit longer. ;-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 10/10] drm/i915: Only run commit when crtc is active.
2015-09-10 15:51 ` Daniel Vetter
@ 2015-09-14 10:02 ` Maarten Lankhorst
0 siblings, 0 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-14 10:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 10-09-15 om 17:51 schreef Daniel Vetter:
> On Thu, Sep 10, 2015 at 04:08:05PM +0200, Maarten Lankhorst wrote:
>> The crtc->active guards are no longer needed now that all state
>> updates are outside the commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> This looks actually complicated, so I'll punt. Merged most of the other
> patches from your series, thanks.
It's very easy, but needs previous patches to be applied, the fixup patch for intel_update_primaries and the update primary plane state outside commit_primary_plane patch.
With all state updates in commit gone, only hw state is still updated.
Each commit_plane has a line with if (!crtc->state->active) return; eg its a noop when crtc is inactive. Same for begin/finish crtc_commit.
Hence it could be changed to
if (crtc->state->active)
drm_atomic_helper_commit_planes_on_crtc(crtc_state);
When planes_changed = false, there is no plane state for the current crtc. The other function of begin_crtc_commit is updating pipe config for a fastset.
If neither are required, there's no hw state to update, so commit_planes_on_crtc would do vblank evasion for no reason. It can be skipped.
Maybe it's easier to understand if I split this up in 2 patches. One to make the crtc->state->active changes, another to add the planes_changed || update_pipe changes.
> -Daniel
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 16 ++++++----------
>> drivers/gpu/drm/i915/intel_sprite.c | 3 ---
>> 2 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f41ca558ba3b..dc696ec5f228 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13135,7 +13135,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>> if (!modeset)
>> intel_pre_plane_update(intel_crtc);
>>
>> - drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>> + if (crtc->state->active && (crtc->state->planes_changed ||
>> + to_intel_crtc_state(crtc->state)->update_pipe))
>> + drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>>
>> if (put_domains)
>> modeset_put_power_domains(dev_priv, put_domains);
>> @@ -13445,9 +13447,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>>
>> crtc = crtc ? crtc : plane->crtc;
>>
>> - if (!crtc->state->active)
>> - return;
>> -
>> dev_priv->display.update_primary_plane(crtc, fb,
>> state->src.x1 >> 16,
>> state->src.y1 >> 16);
>> @@ -13476,8 +13475,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>> intel_update_watermarks(crtc);
>>
>> /* Perform vblank evasion around commit operation */
>> - if (crtc->state->active)
>> - intel_pipe_update_start(intel_crtc);
>> + intel_pipe_update_start(intel_crtc);
>>
>> if (modeset)
>> return;
>> @@ -13493,8 +13491,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>> {
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>
>> - if (crtc->state->active)
>> - intel_pipe_update_end(intel_crtc);
>> + intel_pipe_update_end(intel_crtc);
>> }
>>
>> /**
>> @@ -13677,8 +13674,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>> intel_crtc->cursor_bo = obj;
>>
>> update:
>> - if (crtc->state->active)
>> - intel_crtc_update_cursor(crtc, state->visible);
>> + intel_crtc_update_cursor(crtc, state->visible);
>> }
>>
>> static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 9553859ca151..9c13d8156135 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -927,9 +927,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>>
>> crtc = crtc ? crtc : plane->crtc;
>>
>> - if (!crtc->state->active)
>> - return;
>> -
>> if (state->visible) {
>> intel_plane->update_plane(plane, crtc, fb,
>> state->dst.x1, state->dst.y1,
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [fixup PATCH] drm/i915: Only commit active planes when updating planes during reset.
2015-09-14 9:41 ` [fixup PATCH] drm/i915: Only commit active planes when updating planes during reset Maarten Lankhorst
@ 2015-09-14 13:12 ` Daniel Vetter
0 siblings, 0 replies; 37+ messages in thread
From: Daniel Vetter @ 2015-09-14 13:12 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Mon, Sep 14, 2015 at 11:41:51AM +0200, Maarten Lankhorst wrote:
> In a future commit commit_plane will no longer check if the crtc is active.
> To prevent issues with legacy page flips the check should be performed inside
> update_primary_planes.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ----
>
> Hey,
>
> Op 14-09-15 om 11:02 schreef Daniel Vetter:
> > On Thu, Sep 10, 2015 at 07:34:05PM +0300, Ville Syrjälä wrote:
> >> On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote:
> >>> On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> >>>> On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> >>>>> This function was still using the legacy state, convert it to atomic.
> >>>>> While we're at it, fix the FIXME too and disable the primary plane.
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> ---
> >>>>> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> >>>>> 1 file changed, 12 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 33200403a5db..b68aa95c5460 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> >>>>>
> >>>>> static void intel_update_primary_planes(struct drm_device *dev)
> >>>>> {
> >>>>> - struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>> struct drm_crtc *crtc;
> >>>>>
> >>>>> for_each_crtc(dev, crtc) {
> >>>>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>>> + struct intel_plane *plane = to_intel_plane(crtc->primary);
> >>>>> + struct intel_plane_state *plane_state;
> >>>>>
> >>>>> - drm_modeset_lock(&crtc->mutex, NULL);
> >>>>> - /*
> >>>>> - * FIXME: Once we have proper support for primary planes (and
> >>>>> - * disabling them without disabling the entire crtc) allow again
> >>>>> - * a NULL crtc->primary->fb.
> >>>>> - */
> >>>>> - if (intel_crtc->active && crtc->primary->fb)
> >>>>> - dev_priv->display.update_primary_plane(crtc,
> >>>>> - crtc->primary->fb,
> >>>>> - crtc->x,
> >>>>> - crtc->y);
> >>>>> - drm_modeset_unlock(&crtc->mutex);
> >>>>> + drm_modeset_lock_crtc(crtc, &plane->base);
> >>>>> +
> >>>>> + plane_state = to_intel_plane_state(plane->base.state);
> >>>>> +
> >>>>> + if (plane_state->base.fb)
> >>>>> + plane->commit_plane(&plane->base, plane_state);
> >>>>> + else if (crtc->state->active)
> >>>>> + plane->disable_plane(&plane->base, crtc);
> >>>> That doesn't make sense. There's no way to disable a plane with a page
> >>>> flip, so there's simply nothing to do here if the fb is NULL. If we can
> >>>> trust the plane state to be sane we should just check 'visible' here and
> >>>> commit the plane in that case.
> >>> Right missed that and squash up a fixup that Ville acked on irc. Also
> >>> added the FIXME comment too.
> >> Actually I take it back. We just lost the ->active check entirely so now
> >> we're commiting a plane on a disabled pipe potentially.
> > Blergh and I tagged the pile already :( Maarten, can you pls send out the
> > fixup as a separate patch asap?
> >
> This is not currently an issue because "drm/i915: Only run commit when crtc is active.' is not applied.
>
> Thanks for catching though!
I Meant a full patch (including commit citation and sob and all taht)
since I already tagged it and can't squash in any more.
-Daniel
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 452e8f77151d..47d9c0f8b2fb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3145,10 +3145,9 @@ static void intel_update_primary_planes(struct drm_device *dev)
> struct intel_plane_state *plane_state;
>
> drm_modeset_lock_crtc(crtc, &plane->base);
> -
> plane_state = to_intel_plane_state(plane->base.state);
>
> - if (plane_state->base.fb)
> + if (crtc->state->active && plane_state->base.fb)
> plane->commit_plane(&plane->base, plane_state);
>
> drm_modeset_unlock_crtc(crtc);
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-14 9:52 ` Maarten Lankhorst
@ 2015-09-14 13:13 ` Daniel Vetter
2015-09-14 13:16 ` Maarten Lankhorst
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-14 13:13 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote:
> Op 10-09-15 om 17:41 schreef Daniel Vetter:
> > On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote:
> >> This should allow not running plane commit when the crtc is off.
> >> While the atomic helpers update those, crtc->x/y is only updated
> >> during modesets, and primary plane is updated after this function
> >> returns.
> >>
> >> Unfortunately non-atomic watermarks and fbc still depend on this
> >> state inside i915, so it has to be kept in sync.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> >> 1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 87c5eba08454..b809ee2a8678 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
> >> crtc->hwmode = crtc->state->adjusted_mode;
> >> else
> >> crtc->hwmode.crtc_clock = 0;
> >> +
> >> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
> >> + struct drm_plane_state *plane_state = crtc->primary->state;
> >> +
> >> + crtc->primary->fb = plane_state->fb;
> >> + crtc->x = plane_state->src_x >> 16;
> >> + crtc->y = plane_state->src_y >> 16;
> >> + }
> > drm_atomic_helper_update_legacy_modeset_state should do that for us, not?
> > Atm we have an if (any_ms) check for it, but that seems to just be a bug
> > really.
> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit
> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state.
>
> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc.
Hm, I tried looking really hard but didn't find anything that cared about
plane->fb. Can you please point me at them?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-14 13:13 ` Daniel Vetter
@ 2015-09-14 13:16 ` Maarten Lankhorst
2015-09-14 14:26 ` Daniel Vetter
0 siblings, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-14 13:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 14-09-15 om 15:13 schreef Daniel Vetter:
> On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote:
>> Op 10-09-15 om 17:41 schreef Daniel Vetter:
>>> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote:
>>>> This should allow not running plane commit when the crtc is off.
>>>> While the atomic helpers update those, crtc->x/y is only updated
>>>> during modesets, and primary plane is updated after this function
>>>> returns.
>>>>
>>>> Unfortunately non-atomic watermarks and fbc still depend on this
>>>> state inside i915, so it has to be kept in sync.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>>>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 87c5eba08454..b809ee2a8678 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>>>> crtc->hwmode = crtc->state->adjusted_mode;
>>>> else
>>>> crtc->hwmode.crtc_clock = 0;
>>>> +
>>>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
>>>> + struct drm_plane_state *plane_state = crtc->primary->state;
>>>> +
>>>> + crtc->primary->fb = plane_state->fb;
>>>> + crtc->x = plane_state->src_x >> 16;
>>>> + crtc->y = plane_state->src_y >> 16;
>>>> + }
>>> drm_atomic_helper_update_legacy_modeset_state should do that for us, not?
>>> Atm we have an if (any_ms) check for it, but that seems to just be a bug
>>> really.
>> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit
>> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state.
>>
>> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc.
> Hm, I tried looking really hard but didn't find anything that cared about
> plane->fb. Can you please point me at them?
>
Look for primary->fb. :-) Also crtc->y is used in __intel_fbc_update.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update.
2015-09-10 14:07 ` [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update Maarten Lankhorst
@ 2015-09-14 13:27 ` Ville Syrjälä
2015-09-14 13:40 ` Maarten Lankhorst
2015-09-14 14:23 ` Daniel Vetter
0 siblings, 2 replies; 37+ messages in thread
From: Ville Syrjälä @ 2015-09-14 13:27 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 04:07:56PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 80a98bffd5ba..33200403a5db 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13448,7 +13448,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
> if (!crtc->state->active)
> return;
>
> - dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> + dev_priv->display.update_primary_plane(crtc, fb,
> + state->src.x1 >> 16,
> + state->src.y1 >> 16);
> }
You know I posted a pile of patches last spring to start cleaning up
this mess. Maybe someone wants to rescue the patches and avoid some
duplicate work?
http://lists.freedesktop.org/archives/intel-gfx/2015-March/061715.html
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update.
2015-09-14 13:27 ` Ville Syrjälä
@ 2015-09-14 13:40 ` Maarten Lankhorst
2015-09-14 13:54 ` Ville Syrjälä
2015-09-14 14:23 ` Daniel Vetter
1 sibling, 1 reply; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-14 13:40 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 14-09-15 om 15:27 schreef Ville Syrjälä:
> On Thu, Sep 10, 2015 at 04:07:56PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 80a98bffd5ba..33200403a5db 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13448,7 +13448,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
>> if (!crtc->state->active)
>> return;
>>
>> - dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
>> + dev_priv->display.update_primary_plane(crtc, fb,
>> + state->src.x1 >> 16,
>> + state->src.y1 >> 16);
>> }
> You know I posted a pile of patches last spring to start cleaning up
> this mess. Maybe someone wants to rescue the patches and avoid some
> duplicate work?
>
> http://lists.freedesktop.org/archives/intel-gfx/2015-March/061715.html
>
Yeah I've seen it, watermarks will have to be solved another way.
I've been considering passing to the primary plane state instead. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update.
2015-09-14 13:40 ` Maarten Lankhorst
@ 2015-09-14 13:54 ` Ville Syrjälä
0 siblings, 0 replies; 37+ messages in thread
From: Ville Syrjälä @ 2015-09-14 13:54 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Mon, Sep 14, 2015 at 03:40:47PM +0200, Maarten Lankhorst wrote:
> Op 14-09-15 om 15:27 schreef Ville Syrjälä:
> > On Thu, Sep 10, 2015 at 04:07:56PM +0200, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 80a98bffd5ba..33200403a5db 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -13448,7 +13448,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >> if (!crtc->state->active)
> >> return;
> >>
> >> - dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> >> + dev_priv->display.update_primary_plane(crtc, fb,
> >> + state->src.x1 >> 16,
> >> + state->src.y1 >> 16);
> >> }
> > You know I posted a pile of patches last spring to start cleaning up
> > this mess. Maybe someone wants to rescue the patches and avoid some
> > duplicate work?
> >
> > http://lists.freedesktop.org/archives/intel-gfx/2015-March/061715.html
> >
> Yeah I've seen it, watermarks will have to be solved another way.
> I've been considering passing to the primary plane state instead. :-)
I wasn't referring to watermarks. I meant the series started to make the
primary plane code sane, but it got stuck in some crap limbo again even
though the relevant patches were reviewed. Someone should take them, and
thus make the primary plane code suck a bit less. And then it should be
fairly easy to continue the effort to unify the primary plane code to
look like the sprite plane code, and eventually unify it all. The
current situation is just a weird mess.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update.
2015-09-14 13:27 ` Ville Syrjälä
2015-09-14 13:40 ` Maarten Lankhorst
@ 2015-09-14 14:23 ` Daniel Vetter
2015-09-14 14:31 ` Ville Syrjälä
1 sibling, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-14 14:23 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Sep 14, 2015 at 04:27:23PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 04:07:56PM +0200, Maarten Lankhorst wrote:
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 80a98bffd5ba..33200403a5db 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13448,7 +13448,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
> > if (!crtc->state->active)
> > return;
> >
> > - dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> > + dev_priv->display.update_primary_plane(crtc, fb,
> > + state->src.x1 >> 16,
> > + state->src.y1 >> 16);
> > }
>
> You know I posted a pile of patches last spring to start cleaning up
> this mess. Maybe someone wants to rescue the patches and avoid some
> duplicate work?
>
> http://lists.freedesktop.org/archives/intel-gfx/2015-March/061715.html
I double-checked that series again and I pulled in the first 3 patches
(which had an r-b), but for patches 4+ there's been some inconclusive
discussion afaict from a quick look. Did I miss a resend somewhere?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-14 13:16 ` Maarten Lankhorst
@ 2015-09-14 14:26 ` Daniel Vetter
2015-09-23 14:08 ` Maarten Lankhorst
0 siblings, 1 reply; 37+ messages in thread
From: Daniel Vetter @ 2015-09-14 14:26 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Mon, Sep 14, 2015 at 03:16:20PM +0200, Maarten Lankhorst wrote:
> Op 14-09-15 om 15:13 schreef Daniel Vetter:
> > On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote:
> >> Op 10-09-15 om 17:41 schreef Daniel Vetter:
> >>> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote:
> >>>> This should allow not running plane commit when the crtc is off.
> >>>> While the atomic helpers update those, crtc->x/y is only updated
> >>>> during modesets, and primary plane is updated after this function
> >>>> returns.
> >>>>
> >>>> Unfortunately non-atomic watermarks and fbc still depend on this
> >>>> state inside i915, so it has to be kept in sync.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> >>>> 1 file changed, 8 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index 87c5eba08454..b809ee2a8678 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
> >>>> crtc->hwmode = crtc->state->adjusted_mode;
> >>>> else
> >>>> crtc->hwmode.crtc_clock = 0;
> >>>> +
> >>>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
> >>>> + struct drm_plane_state *plane_state = crtc->primary->state;
> >>>> +
> >>>> + crtc->primary->fb = plane_state->fb;
> >>>> + crtc->x = plane_state->src_x >> 16;
> >>>> + crtc->y = plane_state->src_y >> 16;
> >>>> + }
> >>> drm_atomic_helper_update_legacy_modeset_state should do that for us, not?
> >>> Atm we have an if (any_ms) check for it, but that seems to just be a bug
> >>> really.
> >> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit
> >> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state.
> >>
> >> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc.
> > Hm, I tried looking really hard but didn't find anything that cared about
> > plane->fb. Can you please point me at them?
> >
> Look for primary->fb. :-) Also crtc->y is used in __intel_fbc_update.
Well I did hunt for that but only found usage in crtc_disable functions
(where we need the old value anyway) and flip code (which updates it
itself). I did indeed somehow miss the fbc code. Oh well. Comment might be
good for that.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update.
2015-09-14 14:23 ` Daniel Vetter
@ 2015-09-14 14:31 ` Ville Syrjälä
0 siblings, 0 replies; 37+ messages in thread
From: Ville Syrjälä @ 2015-09-14 14:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, Sep 14, 2015 at 04:23:35PM +0200, Daniel Vetter wrote:
> On Mon, Sep 14, 2015 at 04:27:23PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 10, 2015 at 04:07:56PM +0200, Maarten Lankhorst wrote:
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 80a98bffd5ba..33200403a5db 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13448,7 +13448,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
> > > if (!crtc->state->active)
> > > return;
> > >
> > > - dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> > > + dev_priv->display.update_primary_plane(crtc, fb,
> > > + state->src.x1 >> 16,
> > > + state->src.y1 >> 16);
> > > }
> >
> > You know I posted a pile of patches last spring to start cleaning up
> > this mess. Maybe someone wants to rescue the patches and avoid some
> > duplicate work?
> >
> > http://lists.freedesktop.org/archives/intel-gfx/2015-March/061715.html
>
> I double-checked that series again and I pulled in the first 3 patches
> (which had an r-b), but for patches 4+ there's been some inconclusive
> discussion afaict from a quick look. Did I miss a resend somewhere?
Nah. I think the patches cleaning up the primary plane junk had an r-b,
but I guess those were patches 5-6. But given how bad atomic/plane state
was at the time, I'm sure those couldn't have been applied safely w/o
patch 4. One might hope things have gotten better since then, but I
can't be sure.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook.
2015-09-14 14:26 ` Daniel Vetter
@ 2015-09-23 14:08 ` Maarten Lankhorst
0 siblings, 0 replies; 37+ messages in thread
From: Maarten Lankhorst @ 2015-09-23 14:08 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Op 14-09-15 om 16:26 schreef Daniel Vetter:
> On Mon, Sep 14, 2015 at 03:16:20PM +0200, Maarten Lankhorst wrote:
>> Op 14-09-15 om 15:13 schreef Daniel Vetter:
>>> On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote:
>>>> Op 10-09-15 om 17:41 schreef Daniel Vetter:
>>>>> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote:
>>>>>> This should allow not running plane commit when the crtc is off.
>>>>>> While the atomic helpers update those, crtc->x/y is only updated
>>>>>> during modesets, and primary plane is updated after this function
>>>>>> returns.
>>>>>>
>>>>>> Unfortunately non-atomic watermarks and fbc still depend on this
>>>>>> state inside i915, so it has to be kept in sync.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>>>>>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index 87c5eba08454..b809ee2a8678 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>>>>>> crtc->hwmode = crtc->state->adjusted_mode;
>>>>>> else
>>>>>> crtc->hwmode.crtc_clock = 0;
>>>>>> +
>>>>>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
>>>>>> + struct drm_plane_state *plane_state = crtc->primary->state;
>>>>>> +
>>>>>> + crtc->primary->fb = plane_state->fb;
>>>>>> + crtc->x = plane_state->src_x >> 16;
>>>>>> + crtc->y = plane_state->src_y >> 16;
>>>>>> + }
>>>>> drm_atomic_helper_update_legacy_modeset_state should do that for us, not?
>>>>> Atm we have an if (any_ms) check for it, but that seems to just be a bug
>>>>> really.
>>>> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit
>>>> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state.
>>>>
>>>> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc.
>>> Hm, I tried looking really hard but didn't find anything that cared about
>>> plane->fb. Can you please point me at them?
>>>
>> Look for primary->fb. :-) Also crtc->y is used in __intel_fbc_update.
> Well I did hunt for that but only found usage in crtc_disable functions
> (where we need the old value anyway) and flip code (which updates it
> itself). I did indeed somehow miss the fbc code. Oh well. Comment might be
> good for that.
> -Daniel
Ok, I will resend the remaining patches from this series and this one with the comment added.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2015-09-23 14:08 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-10 14:07 [PATCH 00/10] Nuke some legacy state Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 01/10] drm/i915: Use atomic plane state in the primary plane update Maarten Lankhorst
2015-09-14 13:27 ` Ville Syrjälä
2015-09-14 13:40 ` Maarten Lankhorst
2015-09-14 13:54 ` Ville Syrjälä
2015-09-14 14:23 ` Daniel Vetter
2015-09-14 14:31 ` Ville Syrjälä
2015-09-10 14:07 ` [PATCH 02/10] drm/i915: Use the plane state in intel_crtc_info Maarten Lankhorst
2015-09-10 14:07 ` [PATCH 03/10] drm/i915: Use the atomic state in intel_update_primary_planes Maarten Lankhorst
2015-09-10 15:32 ` Daniel Vetter
2015-09-10 15:43 ` Ville Syrjälä
2015-09-10 16:31 ` Daniel Vetter
2015-09-10 16:34 ` Ville Syrjälä
2015-09-14 9:02 ` Daniel Vetter
2015-09-14 9:41 ` [fixup PATCH] drm/i915: Only commit active planes when updating planes during reset Maarten Lankhorst
2015-09-14 13:12 ` Daniel Vetter
2015-09-10 14:07 ` [PATCH 04/10] drm/i915: Use atomic state when changing cursor visibility Maarten Lankhorst
2015-09-10 15:46 ` Ville Syrjälä
2015-09-10 14:08 ` [PATCH 05/10] drm/i915: Remove legacy plane updates for cursor and sprite planes Maarten Lankhorst
2015-09-10 15:38 ` Daniel Vetter
2015-09-10 14:08 ` [PATCH 06/10] drm/i915: Update legacy primary state outside the commit hook Maarten Lankhorst
2015-09-10 15:41 ` Daniel Vetter
2015-09-14 9:52 ` Maarten Lankhorst
2015-09-14 13:13 ` Daniel Vetter
2015-09-14 13:16 ` Maarten Lankhorst
2015-09-14 14:26 ` Daniel Vetter
2015-09-23 14:08 ` Maarten Lankhorst
2015-09-10 14:08 ` [PATCH 07/10] drm/i915: Do not handle a null plane state Maarten Lankhorst
2015-09-10 14:08 ` [PATCH 08/10] drm/i915: Use crtc->state for duplication Maarten Lankhorst
2015-09-10 15:46 ` Daniel Vetter
2015-09-10 15:49 ` Daniel Vetter
2015-09-10 14:08 ` [PATCH 09/10] drm/i915: Kill off a user of update_state_fb Maarten Lankhorst
2015-09-10 15:50 ` Daniel Vetter
2015-09-14 9:56 ` Maarten Lankhorst
2015-09-10 14:08 ` [PATCH 10/10] drm/i915: Only run commit when crtc is active Maarten Lankhorst
2015-09-10 15:51 ` Daniel Vetter
2015-09-14 10:02 ` Maarten Lankhorst
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.