* [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes.
@ 2017-09-04 10:48 Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2 Maarten Lankhorst
` (9 more replies)
0 siblings, 10 replies; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
New iteration with all feedback hopefully addressed.
Maarten Lankhorst (6):
drm/i915: Always wait for flip_done, v2.
drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
drm/atomic: Return commit in drm_crtc_commit_get for better annotation
drm/atomic: Fix freeing connector/plane state too early by tracking
commits, v3.
drm/atomic: Make async plane update checks work as intended, v2.
drivers/gpu/drm/drm_atomic.c | 11 +-
drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++++++++------------
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 109 +++----------
include/drm/drm_atomic.h | 19 ++-
include/drm/drm_connector.h | 7 +
include/drm/drm_crtc.h | 23 ++-
include/drm/drm_plane.h | 8 +
8 files changed, 275 insertions(+), 198 deletions(-)
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2.
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
@ 2017-09-04 10:48 ` Maarten Lankhorst
2017-09-07 9:49 ` Daniel Vetter
2017-09-04 10:48 ` [PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3 Maarten Lankhorst
` (8 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
The next commit removes the wait for flip_done in in
drm_atomic_helper_commit_cleanup_done, but we need it for the tests
to pass. Instead of using complicated vblank tracking which ends
up being ignored anyway, call the correct atomic helper. :)
Changes since v1:
- Always call drm_atomic_helper_wait_for_flip_done,
even for legacy cursor updates. (danvet)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 84 +++---------------------------------
2 files changed, 8 insertions(+), 79 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 38bd08f2539b..79533ba6f387 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
struct drm_atomic_state *old_state);
void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
struct drm_atomic_state *old_state);
- void (*update_crtcs)(struct drm_atomic_state *state,
- unsigned int *crtc_vblank_mask);
+ void (*update_crtcs)(struct drm_atomic_state *state);
void (*audio_codec_enable)(struct drm_connector *connector,
struct intel_encoder *encoder,
const struct drm_display_mode *adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 216cd9e0e08f..a6cf1c20c712 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
return dev->driver->get_vblank_counter(dev, crtc->pipe);
}
-static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
- struct drm_i915_private *dev_priv,
- unsigned crtc_mask)
-{
- unsigned last_vblank_count[I915_MAX_PIPES];
- enum pipe pipe;
- int ret;
-
- if (!crtc_mask)
- return;
-
- for_each_pipe(dev_priv, pipe) {
- struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
- pipe);
-
- if (!((1 << pipe) & crtc_mask))
- continue;
-
- ret = drm_crtc_vblank_get(&crtc->base);
- if (WARN_ON(ret != 0)) {
- crtc_mask &= ~(1 << pipe);
- continue;
- }
-
- last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
- }
-
- for_each_pipe(dev_priv, pipe) {
- struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
- pipe);
- long lret;
-
- if (!((1 << pipe) & crtc_mask))
- continue;
-
- lret = wait_event_timeout(dev->vblank[pipe].queue,
- last_vblank_count[pipe] !=
- drm_crtc_vblank_count(&crtc->base),
- msecs_to_jiffies(50));
-
- WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
-
- drm_crtc_vblank_put(&crtc->base);
- }
-}
-
-static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
-{
- /* fb updated, need to unpin old fb */
- if (crtc_state->fb_changed)
- return true;
-
- /* wm changes, need vblank before final wm's */
- if (crtc_state->update_wm_post)
- return true;
-
- if (crtc_state->wm.need_postvbl_update)
- return true;
-
- return false;
-}
-
static void intel_update_crtc(struct drm_crtc *crtc,
struct drm_atomic_state *state,
struct drm_crtc_state *old_crtc_state,
- struct drm_crtc_state *new_crtc_state,
- unsigned int *crtc_vblank_mask)
+ struct drm_crtc_state *new_crtc_state)
{
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
}
drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
-
- if (needs_vblank_wait(pipe_config))
- *crtc_vblank_mask |= drm_crtc_mask(crtc);
}
-static void intel_update_crtcs(struct drm_atomic_state *state,
- unsigned int *crtc_vblank_mask)
+static void intel_update_crtcs(struct drm_atomic_state *state)
{
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
@@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
continue;
intel_update_crtc(crtc, state, old_crtc_state,
- new_crtc_state, crtc_vblank_mask);
+ new_crtc_state);
}
}
-static void skl_update_crtcs(struct drm_atomic_state *state,
- unsigned int *crtc_vblank_mask)
+static void skl_update_crtcs(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
@@ -12306,7 +12238,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
vbl_wait = true;
intel_update_crtc(crtc, state, old_crtc_state,
- new_crtc_state, crtc_vblank_mask);
+ new_crtc_state);
if (vbl_wait)
intel_wait_for_vblank(dev_priv, pipe);
@@ -12368,7 +12300,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
struct intel_crtc_state *intel_cstate;
bool hw_check = intel_state->modeset;
u64 put_domains[I915_MAX_PIPES] = {};
- unsigned crtc_vblank_mask = 0;
int i;
intel_atomic_commit_fence_wait(intel_state);
@@ -12458,7 +12389,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
}
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
- dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
+ dev_priv->display.update_crtcs(state);
/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
* already, but still need the state for the delayed optimization. To
@@ -12469,8 +12400,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
* - switch over to the vblank wait helper in the core after that since
* we don't need out special handling any more.
*/
- if (!state->legacy_cursor_update)
- intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
+ drm_atomic_helper_wait_for_flip_done(dev, state);
/*
* Now that the vblank has passed, we can go ahead and program the
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2 Maarten Lankhorst
@ 2017-09-04 10:48 ` Maarten Lankhorst
2017-09-04 15:04 ` [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4 Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 3/6] drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2 Maarten Lankhorst
` (7 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)
The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.
Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
-Remove drm_atomic_helper_async_check hunk. (pinchartl)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/drm_atomic.c | 7 ----
drivers/gpu/drm/drm_atomic_helper.c | 64 ++++++++++++-------------------------
include/drm/drm_atomic.h | 1 -
include/drm/drm_crtc.h | 23 ++++++++++---
4 files changed, 40 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
crtc->funcs->atomic_destroy_state(crtc,
state->crtcs[i].state);
- if (state->crtcs[i].commit) {
- kfree(state->crtcs[i].commit->event);
- state->crtcs[i].commit->event = NULL;
- drm_crtc_commit_put(state->crtcs[i].commit);
- }
-
- state->crtcs[i].commit = NULL;
state->crtcs[i].ptr = NULL;
state->crtcs[i].state = NULL;
}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..9001fc1023b1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
struct drm_atomic_state *old_state)
{
- struct drm_crtc_state *unused;
+ struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
int i;
- for_each_new_crtc_in_state(old_state, crtc, unused, i) {
- struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+ for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+ struct drm_crtc_commit *commit = new_crtc_state->commit;
int ret;
if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
kref_init(&commit->ref);
commit->crtc = crtc;
- state->crtcs[i].commit = commit;
+ new_crtc_state->commit = commit;
ret = stall_checks(crtc, nonblock);
if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
- struct drm_crtc_commit *commit;
- int i = 0;
-
- list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
- /* skip the first entry, that's the current commit */
- if (i == 1)
- return commit;
- i++;
- }
-
- return NULL;
-}
-
/**
* drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
* @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- spin_lock(&crtc->commit_lock);
- commit = preceeding_commit(crtc);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
+ for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (!commit)
continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
if (ret == 0)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
crtc->base.id, crtc->name);
-
- drm_crtc_commit_put(commit);
}
}
EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1857,7 +1835,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
int i;
for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- commit = old_state->crtcs[i].commit;
+ commit = new_crtc_state->commit;
if (!commit)
continue;
@@ -1888,7 +1866,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
long ret;
for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- commit = old_state->crtcs[i].commit;
+ commit = new_crtc_state->commit;
if (WARN_ON(!commit))
continue;
@@ -2294,20 +2272,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
struct drm_private_state *old_obj_state, *new_obj_state;
if (stall) {
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
- spin_lock(&crtc->commit_lock);
- commit = list_first_entry_or_null(&crtc->commit_list,
- struct drm_crtc_commit, commit_entry);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
+ for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (!commit)
continue;
ret = wait_for_completion_interruptible(&commit->hw_done);
- drm_crtc_commit_put(commit);
-
if (ret)
return ret;
}
@@ -2332,13 +2303,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
state->crtcs[i].state = old_crtc_state;
crtc->state = new_crtc_state;
- if (state->crtcs[i].commit) {
+ if (new_crtc_state->commit) {
spin_lock(&crtc->commit_lock);
- list_add(&state->crtcs[i].commit->commit_entry,
+ list_add(&new_crtc_state->commit->commit_entry,
&crtc->commit_list);
spin_unlock(&crtc->commit_lock);
- state->crtcs[i].commit->event = NULL;
+ new_crtc_state->commit->event = NULL;
}
}
@@ -3186,6 +3157,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
state->connectors_changed = false;
state->color_mgmt_changed = false;
state->zpos_changed = false;
+ state->commit = NULL;
state->event = NULL;
state->pageflip_flags = 0;
}
@@ -3224,6 +3196,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
*/
void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
{
+ if (state->commit) {
+ kfree(state->commit->event);
+ state->commit->event = NULL;
+ drm_crtc_commit_put(state->commit);
+ }
+
drm_property_blob_put(state->mode_blob);
drm_property_blob_put(state->degamma_lut);
drm_property_blob_put(state->ctm);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f73b663c1f76..285fbc4ec360 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,7 +144,6 @@ struct __drm_planes_state {
struct __drm_crtcs_state {
struct drm_crtc *ptr;
struct drm_crtc_state *state, *old_state, *new_state;
- struct drm_crtc_commit *commit;
s32 __user *out_fence_ptr;
unsigned last_vblank_count;
};
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a642020e306..1a01ff4ea023 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -253,6 +253,15 @@ struct drm_crtc_state {
*/
struct drm_pending_vblank_event *event;
+ /**
+ * @commit:
+ *
+ * This tracks how the commit for this update proceeds through the
+ * various phases. This is never cleared, except when we destroy the
+ * state, so that subsequent commits can synchronize with previous ones.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_atomic_state *state;
};
@@ -808,10 +817,16 @@ struct drm_crtc {
* @commit_list:
*
* List of &drm_crtc_commit structures tracking pending commits.
- * Protected by @commit_lock. This list doesn't hold its own full
- * reference, but burrows it from the ongoing commit. Commit entries
- * must be removed from this list once the commit is fully completed,
- * but before it's correspoding &drm_atomic_state gets destroyed.
+ * Protected by @commit_lock. This list holds its own full reference,
+ * as does the ongoing commit.
+ *
+ * "Note that the commit for a state change is also tracked in
+ * &drm_crtc_state.commit. For accessing the immediately preceeding
+ * commit in an atomic update it is recommended to just use that
+ * pointer in the old CRTC state, since accessing that doesn't need
+ * any locking or list-walking. @commit_list should only be used to
+ * stall for framebuffer cleanup that's signalled through
+ * &drm_crtc_commit.cleanup_done."
*/
struct list_head commit_list;
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 3/6] drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2 Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3 Maarten Lankhorst
@ 2017-09-04 10:48 ` Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation Maarten Lankhorst
` (6 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
When commit synchronization through drm_crtc_commit was first
introduced, we tried to solve the problem of the flip_done
needing a reference count by blocking in cleanup_done.
This has been changed by commit 24835e442f28 ("drm: reference count
event->completion") which made the waits here no longer needed.
However, even after this commit we still needed the wait because
otherwise we cannot wait for the flip_done because this item might
have been removed from the list.
Changed since v1:
- Make mention of cleanup_done completing before flip_done.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/drm_atomic_helper.c | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9001fc1023b1..1f5cdafb050e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1863,7 +1863,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
struct drm_crtc_state *new_crtc_state;
struct drm_crtc_commit *commit;
int i;
- long ret;
for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
commit = new_crtc_state->commit;
@@ -1873,22 +1872,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
complete_all(&commit->cleanup_done);
WARN_ON(!try_wait_for_completion(&commit->hw_done));
- /* commit_list borrows our reference, need to remove before we
- * clean up our drm_atomic_state. But only after it actually
- * completed, otherwise subsequent commits won't stall properly. */
- if (try_wait_for_completion(&commit->flip_done))
- goto del_commit;
-
- /* We must wait for the vblank event to signal our completion
- * before releasing our reference, since the vblank work does
- * not hold a reference of its own. */
- ret = wait_for_completion_timeout(&commit->flip_done,
- 10*HZ);
- if (ret == 0)
- DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
- crtc->base.id, crtc->name);
-
-del_commit:
spin_lock(&crtc->commit_lock);
list_del(&commit->commit_entry);
spin_unlock(&crtc->commit_lock);
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (2 preceding siblings ...)
2017-09-04 10:48 ` [PATCH v2 3/6] drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2 Maarten Lankhorst
@ 2017-09-04 10:48 ` Maarten Lankhorst
2017-09-07 9:59 ` Daniel Vetter
2017-09-04 10:48 ` [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3 Maarten Lankhorst
` (5 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
This will allow code to do x->commit = drm_crtc_commit_get(commit),
making it clearer where references are used.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/drm_atomic_helper.c | 3 +--
include/drm/drm_atomic.h | 6 +++++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1f5cdafb050e..04629d883114 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1633,8 +1633,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
return -EBUSY;
}
} else if (i == 1) {
- stall_commit = commit;
- drm_crtc_commit_get(stall_commit);
+ stall_commit = drm_crtc_commit_get(commit);
break;
}
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 285fbc4ec360..a80a8dadef00 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -251,10 +251,14 @@ void __drm_crtc_commit_free(struct kref *kref);
* @commit: CRTC commit
*
* Increases the reference of @commit.
+ *
+ * Returns:
+ * The pointer to @commit, with reference increased.
*/
-static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
+static inline struct drm_crtc_commit *drm_crtc_commit_get(struct drm_crtc_commit *commit)
{
kref_get(&commit->ref);
+ return commit;
}
/**
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (3 preceding siblings ...)
2017-09-04 10:48 ` [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation Maarten Lankhorst
@ 2017-09-04 10:48 ` Maarten Lankhorst
2017-09-07 10:05 ` Daniel Vetter
2017-09-04 10:48 ` [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2 Maarten Lankhorst
` (4 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Laurent Pinchart
Currently we neatly track the crtc state, but forget to look at
plane/connector state.
When doing a nonblocking modeset, immediately followed by a setprop
before the modeset completes, the setprop will see the modesets new
state as the old state and free it.
This has to be solved by waiting for hw_done on the connector, even
if it's not assigned to a crtc. When a connector is unbound we take
the last crtc commit, and when it stays unbound we create a new
fake crtc commit for that gets signaled on hw_done for all the
planes/connectors.
We wait for it the same way as we do for crtc's, which will make
sure we never run into a use-after-free situation.
Changes since v1:
- Only create a single disable commit. (danvet)
- Fix leak in intel_legacy_cursor_update.
Changes since v2:
- Make reference counting in drm_atomic_helper_setup_commit
more obvious. (pinchartl)
- Call cleanup_done for fake commit. (danvet)
- Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
- Add comment to drm_atomic_helper_swap_state. (pinchartl)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/gpu/drm/drm_atomic.c | 4 +
drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_display.c | 2 +
include/drm/drm_atomic.h | 12 +++
include/drm/drm_connector.h | 7 ++
include/drm/drm_plane.h | 7 ++
6 files changed, 198 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2cce48f203e0..75f5f74de9bf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
}
state->num_private_objs = 0;
+ if (state->fake_commit) {
+ drm_crtc_commit_put(state->fake_commit);
+ state->fake_commit = NULL;
+ }
}
EXPORT_SYMBOL(drm_atomic_state_default_clear);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 04629d883114..c81d46927a74 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
drm_crtc_commit_put(commit);
}
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
+{
+ init_completion(&commit->flip_done);
+ init_completion(&commit->hw_done);
+ init_completion(&commit->cleanup_done);
+ INIT_LIST_HEAD(&commit->commit_entry);
+ kref_init(&commit->ref);
+ commit->crtc = crtc;
+}
+
+static struct drm_crtc_commit *
+crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+{
+ if (crtc) {
+ struct drm_crtc_state *new_crtc_state;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+ return new_crtc_state->commit;
+ }
+
+ if (!state->fake_commit) {
+ state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
+ if (!state->fake_commit)
+ return NULL;
+
+ init_commit(state->fake_commit, NULL);
+ }
+
+ return state->fake_commit;
+}
+
/**
* drm_atomic_helper_setup_commit - setup possibly nonblocking commit
* @state: new modeset state to be committed
@@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
{
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+ struct drm_connector *conn;
+ struct drm_connector_state *old_conn_state, *new_conn_state;
+ struct drm_plane *plane;
+ struct drm_plane_state *old_plane_state, *new_plane_state;
struct drm_crtc_commit *commit;
int i, ret;
@@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
if (!commit)
return -ENOMEM;
- init_completion(&commit->flip_done);
- init_completion(&commit->hw_done);
- init_completion(&commit->cleanup_done);
- INIT_LIST_HEAD(&commit->commit_entry);
- kref_init(&commit->ref);
- commit->crtc = crtc;
+ init_commit(commit, crtc);
new_crtc_state->commit = commit;
@@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
drm_crtc_commit_get(commit);
}
+ for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
+ /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+ if (new_conn_state->crtc)
+ continue;
+
+ /* Userspace is not allowed to get ahead of the previous
+ * commit with nonblocking ones. */
+ if (nonblock && old_conn_state->commit &&
+ !try_wait_for_completion(&old_conn_state->commit->flip_done))
+ return -EBUSY;
+
+ commit = crtc_or_fake_commit(state, old_conn_state->crtc);
+ if (!commit)
+ return -ENOMEM;
+
+ new_conn_state->commit = drm_crtc_commit_get(commit);
+ }
+
+ for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+ /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+ if (new_plane_state->crtc)
+ continue;
+
+ /* Userspace is not allowed to get ahead of the previous
+ * commit with nonblocking ones. */
+ if (nonblock && old_plane_state->commit &&
+ !try_wait_for_completion(&old_plane_state->commit->flip_done))
+ return -EBUSY;
+
+ commit = crtc_or_fake_commit(state, old_plane_state->crtc);
+ if (!commit)
+ return -ENOMEM;
+
+ new_plane_state->commit = drm_crtc_commit_get(commit);
+ }
+
return 0;
}
EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
@@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state;
+ struct drm_plane *plane;
+ struct drm_plane_state *old_plane_state;
+ struct drm_connector *conn;
+ struct drm_connector_state *old_conn_state;
struct drm_crtc_commit *commit;
int i;
long ret;
@@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
crtc->base.id, crtc->name);
}
+
+ for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
+ commit = old_conn_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_timeout(&commit->hw_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
+ conn->base.id, conn->name);
+
+ /* Currently no support for overwriting flips, hence
+ * stall for previous one to execute completely. */
+ ret = wait_for_completion_timeout(&commit->flip_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
+ conn->base.id, conn->name);
+ }
+
+ for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+ commit = old_plane_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_timeout(&commit->hw_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
+ plane->base.id, plane->name);
+
+ /* Currently no support for overwriting flips, hence
+ * stall for previous one to execute completely. */
+ ret = wait_for_completion_timeout(&commit->flip_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
+ plane->base.id, plane->name);
+ }
}
EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
WARN_ON(new_crtc_state->event);
complete_all(&commit->hw_done);
}
+
+ if (old_state->fake_commit) {
+ complete_all(&old_state->fake_commit->hw_done);
+ complete_all(&old_state->fake_commit->flip_done);
+ }
}
EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
@@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
list_del(&commit->commit_entry);
spin_unlock(&crtc->commit_lock);
}
+
+ if (old_state->fake_commit)
+ complete_all(&old_state->fake_commit->cleanup_done);
}
EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
@@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
struct drm_private_state *old_obj_state, *new_obj_state;
if (stall) {
+ /*
+ * We have to stall for hw_done here before
+ * drm_atomic_helper_wait_for_dependencies() because flip
+ * depth > 1 is not yet supported by all drivers. As long as
+ * obj->state is directly dereferenced anywhere in the drivers
+ * atomic_commit_tail function, then it's unsafe to swap state
+ * before drm_atomic_helper_commit_hw_done() is called.
+ */
+
for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
commit = old_crtc_state->commit;
@@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
if (ret)
return ret;
}
+
+ for_each_old_connector_in_state(state, connector, old_conn_state, i) {
+ commit = old_conn_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_interruptible(&commit->hw_done);
+ if (ret)
+ return ret;
+ }
+
+ for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+ commit = old_plane_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_interruptible(&commit->hw_done);
+ if (ret)
+ return ret;
+ }
}
for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
@@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
drm_framebuffer_get(state->fb);
state->fence = NULL;
+ state->commit = NULL;
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
if (state->fence)
dma_fence_put(state->fence);
+
+ if (state->commit)
+ drm_crtc_commit_put(state->commit);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
@@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
memcpy(state, connector->state, sizeof(*state));
if (state->crtc)
drm_connector_get(connector);
+ state->commit = NULL;
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
@@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
{
if (state->crtc)
drm_connector_put(state->connector);
+
+ if (state->commit)
+ drm_crtc_commit_put(state->commit);
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a6cf1c20c712..7abbc761a635 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
/* Swap plane state */
new_plane_state->fence = old_plane_state->fence;
+ new_plane_state->commit = old_plane_state->commit;
*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
new_plane_state->fence = NULL;
+ new_plane_state->commit = NULL;
new_plane_state->fb = old_fb;
to_intel_plane_state(new_plane_state)->vma = NULL;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index a80a8dadef00..07a71daa3582 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -236,6 +236,18 @@ struct drm_atomic_state {
struct drm_modeset_acquire_ctx *acquire_ctx;
/**
+ * @fake_commit:
+ *
+ * Used for signaling unbound planes/connectors.
+ * When a connector or plane is not bound to any CRTC, it's still important
+ * to preserve linearity to prevent the atomic states from being freed to early.
+ *
+ * This commit (if set) is not bound to any crtc, but will be completed when
+ * drm_atomic_helper_commit_hw_done() is called.
+ */
+ struct drm_crtc_commit *fake_commit;
+
+ /**
* @commit_work:
*
* Work item which can be used by the driver or helpers to execute the
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ea8da401c93c..8837649d16e8 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -347,6 +347,13 @@ struct drm_connector_state {
struct drm_atomic_state *state;
+ /**
+ * @commit: Tracks the pending commit to prevent use-after-free conditions.
+ *
+ * Is only set when @crtc is NULL.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_tv_connector_state tv;
/**
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 73f90f9d057f..7d96116fd4c4 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -123,6 +123,13 @@ struct drm_plane_state {
*/
bool visible;
+ /**
+ * @commit: Tracks the pending commit to prevent use-after-free conditions.
+ *
+ * Is only set when @crtc is NULL.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_atomic_state *state;
};
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2.
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (4 preceding siblings ...)
2017-09-04 10:48 ` [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3 Maarten Lankhorst
@ 2017-09-04 10:48 ` Maarten Lankhorst
[not found] ` <20170904104838.23822-7-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-09-04 11:11 ` ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2) Patchwork
` (3 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:48 UTC (permalink / raw)
To: dri-devel; +Cc: Gustavo Padovan, intel-gfx
By always keeping track of the last commit in plane_state, we know
whether there is an active update on the plane or not. With that
information we can reject the fast update, and force the slowpath
to be used as was originally intended.
We cannot use plane_state->crtc->state here, because this only mentions
the most recent commit for the crtc, but not the planes that were part
of it. We specifically care about what the last commit involving this
plane is, which can only be tracked with a pointer in the plane state.
Changes since v1:
- Clean up the whole function here, instead of partially earlier.
- Add mention in the commit message why we need commit in plane_state.
- Swap plane->state in intel_legacy_cursor_update, instead of
reassigning all variables. With this commit We know that the cursor
is not part of any active commits so this hack can be removed.
Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
---
drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++--------------------------
drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
include/drm/drm_plane.h | 5 ++--
3 files changed, 35 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c81d46927a74..a2cd432d8b2d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
{
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
- struct drm_crtc_commit *commit;
- struct drm_plane *__plane, *plane = NULL;
- struct drm_plane_state *__plane_state, *plane_state = NULL;
+ struct drm_plane *plane;
+ struct drm_plane_state *old_plane_state, *new_plane_state;
const struct drm_plane_helper_funcs *funcs;
- int i, j, n_planes = 0;
+ int i, n_planes = 0;
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
if (drm_atomic_crtc_needs_modeset(crtc_state))
return -EINVAL;
}
- for_each_new_plane_in_state(state, __plane, __plane_state, i) {
+ for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
n_planes++;
- plane = __plane;
- plane_state = __plane_state;
- }
/* FIXME: we support only single plane updates for now */
- if (!plane || n_planes != 1)
+ if (n_planes != 1)
return -EINVAL;
- if (!plane_state->crtc)
+ if (!new_plane_state->crtc)
return -EINVAL;
funcs = plane->helper_private;
if (!funcs->atomic_async_update)
return -EINVAL;
- if (plane_state->fence)
+ if (new_plane_state->fence)
return -EINVAL;
/*
@@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
* the plane. This prevents our async update's changes from getting
* overridden by a previous synchronous update's state.
*/
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
- if (plane->crtc != crtc)
- continue;
-
- spin_lock(&crtc->commit_lock);
- commit = list_first_entry_or_null(&crtc->commit_list,
- struct drm_crtc_commit,
- commit_entry);
- if (!commit) {
- spin_unlock(&crtc->commit_lock);
- continue;
- }
- spin_unlock(&crtc->commit_lock);
-
- if (!crtc->state->state)
- continue;
+ if (old_plane_state->commit &&
+ !try_wait_for_completion(&old_plane_state->commit->hw_done))
+ return -EBUSY;
- for_each_plane_in_state(crtc->state->state, __plane,
- __plane_state, j) {
- if (__plane == plane)
- return -EINVAL;
- }
- }
-
- return funcs->atomic_async_check(plane, plane_state);
+ return funcs->atomic_async_check(plane, new_plane_state);
}
EXPORT_SYMBOL(drm_atomic_helper_async_check);
@@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
}
for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
- /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
- if (new_plane_state->crtc)
- continue;
+ /*
+ * Unlike connectors, always track planes explicitly for
+ * async pageflip support.
+ */
/* Userspace is not allowed to get ahead of the previous
* commit with nonblocking ones. */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7abbc761a635..0add029d95f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13064,6 +13064,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
goto slow;
old_plane_state = plane->state;
+ /*
+ * Don't do an async update if there is an outstanding commit modifying
+ * the plane. This prevents our async update's changes from getting
+ * overridden by a previous synchronous update's state.
+ */
+ if (old_plane_state->commit &&
+ !try_wait_for_completion(&old_plane_state->commit->hw_done))
+ goto slow;
/*
* If any parameters change that may affect watermarks,
@@ -13125,19 +13133,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
}
old_fb = old_plane_state->fb;
- old_vma = to_intel_plane_state(old_plane_state)->vma;
i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
intel_plane->frontbuffer_bit);
/* Swap plane state */
- new_plane_state->fence = old_plane_state->fence;
- new_plane_state->commit = old_plane_state->commit;
- *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
- new_plane_state->fence = NULL;
- new_plane_state->commit = NULL;
- new_plane_state->fb = old_fb;
- to_intel_plane_state(new_plane_state)->vma = NULL;
+ plane->state = new_plane_state;
if (plane->state->visible) {
trace_intel_update_plane(plane, to_intel_crtc(crtc));
@@ -13149,13 +13150,19 @@ intel_legacy_cursor_update(struct drm_plane *plane,
intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
}
- if (old_vma)
+ old_vma = to_intel_plane_state(old_plane_state)->vma;
+ if (old_vma) {
+ to_intel_plane_state(old_plane_state)->vma = NULL;
intel_unpin_fb_vma(old_vma);
+ }
out_unlock:
mutex_unlock(&dev_priv->drm.struct_mutex);
out_free:
- intel_plane_destroy_state(plane, new_plane_state);
+ if (ret)
+ intel_plane_destroy_state(plane, new_plane_state);
+ else
+ intel_plane_destroy_state(plane, old_plane_state);
return ret;
slow:
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 7d96116fd4c4..feb9941d0cdb 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -124,9 +124,10 @@ struct drm_plane_state {
bool visible;
/**
- * @commit: Tracks the pending commit to prevent use-after-free conditions.
+ * @commit: Tracks the pending commit to prevent use-after-free conditions,
+ * and for async plane updates.
*
- * Is only set when @crtc is NULL.
+ * May be NULL.
*/
struct drm_crtc_commit *commit;
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (5 preceding siblings ...)
2017-09-04 10:48 ` [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2 Maarten Lankhorst
@ 2017-09-04 11:11 ` Patchwork
2017-09-04 12:45 ` ✗ Fi.CI.IGT: failure " Patchwork
` (2 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-09-04 11:11 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
URL : https://patchwork.freedesktop.org/series/29538/
State : success
== Summary ==
Series 29538v2 drm/atomic: Fix use-after-free with unbound connectors/planes.
https://patchwork.freedesktop.org/api/1.0/series/29538/revisions/2/mbox/
Test kms_flip:
Subgroup basic-flip-vs-modeset:
pass -> SKIP (fi-skl-x1585l) fdo#101781
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fi-bdw-5557u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:453s
fi-bdw-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:439s
fi-blb-e6850 total:288 pass:224 dwarn:1 dfail:0 fail:0 skip:63 time:361s
fi-bsw-n3050 total:288 pass:243 dwarn:0 dfail:0 fail:0 skip:45 time:554s
fi-bwr-2160 total:288 pass:184 dwarn:0 dfail:0 fail:0 skip:104 time:255s
fi-bxt-j4205 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:519s
fi-byt-j1900 total:288 pass:254 dwarn:1 dfail:0 fail:0 skip:33 time:523s
fi-byt-n2820 total:288 pass:250 dwarn:1 dfail:0 fail:0 skip:37 time:518s
fi-cfl-s total:285 pass:250 dwarn:1 dfail:0 fail:0 skip:33
fi-elk-e7500 total:288 pass:230 dwarn:0 dfail:0 fail:0 skip:58 time:438s
fi-glk-2a total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:612s
fi-hsw-4770 total:288 pass:263 dwarn:0 dfail:0 fail:0 skip:25 time:444s
fi-hsw-4770r total:288 pass:263 dwarn:0 dfail:0 fail:0 skip:25 time:423s
fi-ilk-650 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:427s
fi-ivb-3520m total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:507s
fi-ivb-3770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:472s
fi-kbl-7500u total:288 pass:264 dwarn:1 dfail:0 fail:0 skip:23 time:520s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:596s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:598s
fi-pnv-d510 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:528s
fi-skl-6260u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:472s
fi-skl-6700k total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:534s
fi-skl-6770hq total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:518s
fi-skl-gvtdvm total:288 pass:266 dwarn:0 dfail:0 fail:0 skip:22 time:444s
fi-skl-x1585l total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:483s
fi-snb-2520m total:288 pass:251 dwarn:0 dfail:0 fail:0 skip:37 time:548s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:2 skip:38 time:403s
a53eac614143e349be9ca1ab6c4ac799f8e1f0f9 drm-tip: 2017y-09m-04d-09h-11m-21s UTC integration manifest
a5bb7aa90443 drm/atomic: Make async plane update checks work as intended, v2.
01321b66e3ad drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
0e520aabc9b9 drm/atomic: Return commit in drm_crtc_commit_get for better annotation
eea1321f34ec drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
0c1f91d8a2ba drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
f65cba7fa70a drm/i915: Always wait for flip_done, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5574/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (6 preceding siblings ...)
2017-09-04 11:11 ` ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2) Patchwork
@ 2017-09-04 12:45 ` Patchwork
2017-09-04 15:23 ` ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3) Patchwork
2017-09-04 16:21 ` ✗ Fi.CI.IGT: failure " Patchwork
9 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-09-04 12:45 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
URL : https://patchwork.freedesktop.org/series/29538/
State : failure
== Summary ==
Test kms_setmode:
Subgroup basic:
pass -> FAIL (shard-hsw) fdo#99912
Test kms_cursor_legacy:
Subgroup long-nonblocking-modeset-vs-cursor-atomic:
pass -> INCOMPLETE (shard-hsw)
Test perf:
Subgroup blocking:
pass -> FAIL (shard-hsw) fdo#102252
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
skip -> PASS (shard-hsw)
Subgroup fbc-1p-offscren-pri-indfb-draw-mmap-wc:
skip -> PASS (shard-hsw)
Subgroup fbc-rgb565-draw-mmap-gtt:
pass -> FAIL (shard-hsw)
Test kms_pipe_crc_basic:
Subgroup nonblocking-crc-pipe-C-frame-sequence:
pass -> FAIL (shard-hsw)
Test kms_busy:
Subgroup extended-modeset-hang-oldfb-with-reset-render-C:
pass -> INCOMPLETE (shard-hsw)
Test kms_atomic_transition:
Subgroup plane-use-after-nonblocking-unbind-fencing:
fail -> PASS (shard-hsw) fdo#101847 +1
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847
shard-hsw total:2118 pass:1153 dwarn:0 dfail:0 fail:17 skip:946 time:8895s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5574/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.
2017-09-04 10:48 ` [PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3 Maarten Lankhorst
@ 2017-09-04 15:04 ` Maarten Lankhorst
2017-09-04 18:01 ` kbuild test robot
2017-09-05 6:25 ` Daniel Vetter
0 siblings, 2 replies; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 15:04 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)
The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.
Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
- Remove drm_atomic_helper_async_check hunk. (pinchartl)
Changes since v3:
- Fix use-after-free in drm_atomic_helper_commit_cleanup_done().
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/drm_atomic.c | 7 ----
drivers/gpu/drm/drm_atomic_helper.c | 82 ++++++++++++++++---------------------
include/drm/drm_atomic.h | 1 -
include/drm/drm_crtc.h | 23 +++++++++--
4 files changed, 54 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
crtc->funcs->atomic_destroy_state(crtc,
state->crtcs[i].state);
- if (state->crtcs[i].commit) {
- kfree(state->crtcs[i].commit->event);
- state->crtcs[i].commit->event = NULL;
- drm_crtc_commit_put(state->crtcs[i].commit);
- }
-
- state->crtcs[i].commit = NULL;
state->crtcs[i].ptr = NULL;
state->crtcs[i].state = NULL;
}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..80c138cbde9a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
struct drm_atomic_state *old_state)
{
- struct drm_crtc_state *unused;
+ struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
int i;
- for_each_new_crtc_in_state(old_state, crtc, unused, i) {
- struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+ for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+ struct drm_crtc_commit *commit = new_crtc_state->commit;
int ret;
if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
kref_init(&commit->ref);
commit->crtc = crtc;
- state->crtcs[i].commit = commit;
+ new_crtc_state->commit = commit;
ret = stall_checks(crtc, nonblock);
if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
- struct drm_crtc_commit *commit;
- int i = 0;
-
- list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
- /* skip the first entry, that's the current commit */
- if (i == 1)
- return commit;
- i++;
- }
-
- return NULL;
-}
-
/**
* drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
* @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- spin_lock(&crtc->commit_lock);
- commit = preceeding_commit(crtc);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
+ for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (!commit)
continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
if (ret == 0)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
crtc->base.id, crtc->name);
-
- drm_crtc_commit_put(commit);
}
}
EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1852,15 +1830,25 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state, *new_crtc_state;
struct drm_crtc_commit *commit;
int i;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- commit = old_state->crtcs[i].commit;
+ for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+ commit = new_crtc_state->commit;
if (!commit)
continue;
+ /*
+ * copy new_crtc_state->commit to old_crtc_state->commit,
+ * it's unsafe to touch new_crtc_state after hw_done,
+ * but we still need to do so in cleanup_done().
+ */
+ if (old_crtc_state->commit)
+ drm_crtc_commit_put(old_crtc_state->commit);
+
+ old_crtc_state->commit = drm_crtc_commit_get(commit);
+
/* backend must have consumed any event by now */
WARN_ON(new_crtc_state->event);
complete_all(&commit->hw_done);
@@ -1882,13 +1870,13 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- commit = old_state->crtcs[i].commit;
+ for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (WARN_ON(!commit))
continue;
@@ -2294,20 +2282,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
struct drm_private_state *old_obj_state, *new_obj_state;
if (stall) {
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
- spin_lock(&crtc->commit_lock);
- commit = list_first_entry_or_null(&crtc->commit_list,
- struct drm_crtc_commit, commit_entry);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
+ for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (!commit)
continue;
ret = wait_for_completion_interruptible(&commit->hw_done);
- drm_crtc_commit_put(commit);
-
if (ret)
return ret;
}
@@ -2332,13 +2313,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
state->crtcs[i].state = old_crtc_state;
crtc->state = new_crtc_state;
- if (state->crtcs[i].commit) {
+ if (new_crtc_state->commit) {
spin_lock(&crtc->commit_lock);
- list_add(&state->crtcs[i].commit->commit_entry,
+ list_add(&new_crtc_state->commit->commit_entry,
&crtc->commit_list);
spin_unlock(&crtc->commit_lock);
- state->crtcs[i].commit->event = NULL;
+ new_crtc_state->commit->event = NULL;
}
}
@@ -3186,6 +3167,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
state->connectors_changed = false;
state->color_mgmt_changed = false;
state->zpos_changed = false;
+ state->commit = NULL;
state->event = NULL;
state->pageflip_flags = 0;
}
@@ -3224,6 +3206,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
*/
void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
{
+ if (state->commit) {
+ kfree(state->commit->event);
+ state->commit->event = NULL;
+ drm_crtc_commit_put(state->commit);
+ }
+
drm_property_blob_put(state->mode_blob);
drm_property_blob_put(state->degamma_lut);
drm_property_blob_put(state->ctm);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f73b663c1f76..285fbc4ec360 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,7 +144,6 @@ struct __drm_planes_state {
struct __drm_crtcs_state {
struct drm_crtc *ptr;
struct drm_crtc_state *state, *old_state, *new_state;
- struct drm_crtc_commit *commit;
s32 __user *out_fence_ptr;
unsigned last_vblank_count;
};
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a642020e306..1a01ff4ea023 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -253,6 +253,15 @@ struct drm_crtc_state {
*/
struct drm_pending_vblank_event *event;
+ /**
+ * @commit:
+ *
+ * This tracks how the commit for this update proceeds through the
+ * various phases. This is never cleared, except when we destroy the
+ * state, so that subsequent commits can synchronize with previous ones.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_atomic_state *state;
};
@@ -808,10 +817,16 @@ struct drm_crtc {
* @commit_list:
*
* List of &drm_crtc_commit structures tracking pending commits.
- * Protected by @commit_lock. This list doesn't hold its own full
- * reference, but burrows it from the ongoing commit. Commit entries
- * must be removed from this list once the commit is fully completed,
- * but before it's correspoding &drm_atomic_state gets destroyed.
+ * Protected by @commit_lock. This list holds its own full reference,
+ * as does the ongoing commit.
+ *
+ * "Note that the commit for a state change is also tracked in
+ * &drm_crtc_state.commit. For accessing the immediately preceeding
+ * commit in an atomic update it is recommended to just use that
+ * pointer in the old CRTC state, since accessing that doesn't need
+ * any locking or list-walking. @commit_list should only be used to
+ * stall for framebuffer cleanup that's signalled through
+ * &drm_crtc_commit.cleanup_done."
*/
struct list_head commit_list;
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (7 preceding siblings ...)
2017-09-04 12:45 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2017-09-04 15:23 ` Patchwork
2017-09-04 16:21 ` ✗ Fi.CI.IGT: failure " Patchwork
9 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-09-04 15:23 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
URL : https://patchwork.freedesktop.org/series/29538/
State : success
== Summary ==
Series 29538v3 drm/atomic: Fix use-after-free with unbound connectors/planes.
https://patchwork.freedesktop.org/api/1.0/series/29538/revisions/3/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail -> PASS (fi-snb-2600) fdo#100007
Test kms_flip:
Subgroup basic-flip-vs-modeset:
skip -> PASS (fi-skl-x1585l) fdo#101781
Test drv_module_reload:
Subgroup basic-reload:
dmesg-warn -> INCOMPLETE (fi-cfl-s) k.org#196765
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
k.org#196765 https://bugzilla.kernel.org/show_bug.cgi?id=196765
fi-bdw-5557u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:458s
fi-bdw-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:444s
fi-blb-e6850 total:288 pass:224 dwarn:1 dfail:0 fail:0 skip:63 time:358s
fi-bsw-n3050 total:288 pass:243 dwarn:0 dfail:0 fail:0 skip:45 time:563s
fi-bwr-2160 total:288 pass:184 dwarn:0 dfail:0 fail:0 skip:104 time:253s
fi-bxt-j4205 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:517s
fi-byt-j1900 total:288 pass:254 dwarn:1 dfail:0 fail:0 skip:33 time:526s
fi-byt-n2820 total:288 pass:250 dwarn:1 dfail:0 fail:0 skip:37 time:514s
fi-cfl-s total:285 pass:250 dwarn:1 dfail:0 fail:0 skip:33
fi-elk-e7500 total:288 pass:230 dwarn:0 dfail:0 fail:0 skip:58 time:440s
fi-glk-2a total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:612s
fi-hsw-4770 total:288 pass:263 dwarn:0 dfail:0 fail:0 skip:25 time:444s
fi-hsw-4770r total:288 pass:263 dwarn:0 dfail:0 fail:0 skip:25 time:426s
fi-ilk-650 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:424s
fi-ivb-3520m total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:506s
fi-ivb-3770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:482s
fi-kbl-7500u total:288 pass:264 dwarn:1 dfail:0 fail:0 skip:23 time:510s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:595s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:596s
fi-pnv-d510 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:537s
fi-skl-6260u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:473s
fi-skl-6700k total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:538s
fi-skl-6770hq total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:513s
fi-skl-gvtdvm total:288 pass:266 dwarn:0 dfail:0 fail:0 skip:22 time:446s
fi-skl-x1585l total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:507s
fi-snb-2520m total:288 pass:251 dwarn:0 dfail:0 fail:0 skip:37 time:551s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:2 skip:38 time:406s
59fbfaa48e1837b7a692df34ce6696eb4035e7a8 drm-tip: 2017y-09m-04d-11h-39m-37s UTC integration manifest
b307bc7d0cf6 drm/atomic: Make async plane update checks work as intended, v2.
e23ef2074a02 drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
08156193021b drm/atomic: Return commit in drm_crtc_commit_get for better annotation
5d5cf3601e90 drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
e283a7f56851 drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.
cdfa52e3441f drm/i915: Always wait for flip_done, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5575/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
` (8 preceding siblings ...)
2017-09-04 15:23 ` ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3) Patchwork
@ 2017-09-04 16:21 ` Patchwork
9 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-09-04 16:21 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
URL : https://patchwork.freedesktop.org/series/29538/
State : failure
== Summary ==
Test kms_atomic_transition:
Subgroup plane-use-after-nonblocking-unbind-fencing:
fail -> PASS (shard-hsw) fdo#101847 +1
Test gem_sync:
Subgroup basic-many-each:
pass -> FAIL (shard-hsw)
Test perf:
Subgroup blocking:
pass -> FAIL (shard-hsw) fdo#102252
Test kms_setmode:
Subgroup basic:
fail -> PASS (shard-hsw) fdo#99912
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (shard-hsw)
fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
shard-hsw total:2265 pass:1232 dwarn:2 dfail:0 fail:15 skip:1016 time:9556s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5575/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.
2017-09-04 15:04 ` [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4 Maarten Lankhorst
@ 2017-09-04 18:01 ` kbuild test robot
2017-09-05 6:25 ` Daniel Vetter
1 sibling, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2017-09-04 18:01 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, kbuild-all, dri-devel
[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]
Hi Maarten,
[auto build test ERROR on drm/drm-next]
[also build test ERROR on next-20170904]
[cannot apply to v4.13]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-atomic-Move-drm_crtc_commit-to-drm_crtc_state-v4/20170905-011023
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x016-201736 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu//drm/drm_atomic_helper.c: In function 'drm_atomic_helper_commit_hw_done':
>> drivers/gpu//drm/drm_atomic_helper.c:1850:26: error: void value not ignored as it ought to be
old_crtc_state->commit = drm_crtc_commit_get(commit);
^
vim +1850 drivers/gpu//drm/drm_atomic_helper.c
1814
1815 /**
1816 * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
1817 * @old_state: atomic state object with old state structures
1818 *
1819 * This function is used to signal completion of the hardware commit step. After
1820 * this step the driver is not allowed to read or change any permanent software
1821 * or hardware modeset state. The only exception is state protected by other
1822 * means than &drm_modeset_lock locks.
1823 *
1824 * Drivers should try to postpone any expensive or delayed cleanup work after
1825 * this function is called.
1826 *
1827 * This is part of the atomic helper support for nonblocking commits, see
1828 * drm_atomic_helper_setup_commit() for an overview.
1829 */
1830 void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
1831 {
1832 struct drm_crtc *crtc;
1833 struct drm_crtc_state *old_crtc_state, *new_crtc_state;
1834 struct drm_crtc_commit *commit;
1835 int i;
1836
1837 for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
1838 commit = new_crtc_state->commit;
1839 if (!commit)
1840 continue;
1841
1842 /*
1843 * copy new_crtc_state->commit to old_crtc_state->commit,
1844 * it's unsafe to touch new_crtc_state after hw_done,
1845 * but we still need to do so in cleanup_done().
1846 */
1847 if (old_crtc_state->commit)
1848 drm_crtc_commit_put(old_crtc_state->commit);
1849
> 1850 old_crtc_state->commit = drm_crtc_commit_get(commit);
1851
1852 /* backend must have consumed any event by now */
1853 WARN_ON(new_crtc_state->event);
1854 complete_all(&commit->hw_done);
1855 }
1856 }
1857 EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
1858
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30752 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.
2017-09-04 15:04 ` [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4 Maarten Lankhorst
2017-09-04 18:01 ` kbuild test robot
@ 2017-09-05 6:25 ` Daniel Vetter
1 sibling, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2017-09-05 6:25 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel
On Mon, Sep 04, 2017 at 05:04:56PM +0200, Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortuantely we already have a place to track those. Move it to
> drm_crtc_state where it belongs. :)
>
> The per-crtc commit_list is kept for places where we have to look
> deeper than the current or previous commit for checking whether to stall
> on unpin. This is used in drm_atomic_helper_setup_commit and
> intel_has_pending_fb_unpin.
>
> Changes since v1:
> - Update kerneldoc for drm_crtc.commit_list. (danvet)
> Changes since v2:
> - Remove drm_atomic_helper_async_check hunk. (pinchartl)
> Changes since v3:
> - Fix use-after-free in drm_atomic_helper_commit_cleanup_done().
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Might be good to drop this, or at least annoate that the r-b is for v3,
just for next time around. But I looked at the patch again, r-b: me still
holds I think. But then I missed the bug in v4 ...
-Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 7 ----
> drivers/gpu/drm/drm_atomic_helper.c | 82 ++++++++++++++++---------------------
> include/drm/drm_atomic.h | 1 -
> include/drm/drm_crtc.h | 23 +++++++++--
> 4 files changed, 54 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..2cce48f203e0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> crtc->funcs->atomic_destroy_state(crtc,
> state->crtcs[i].state);
>
> - if (state->crtcs[i].commit) {
> - kfree(state->crtcs[i].commit->event);
> - state->crtcs[i].commit->event = NULL;
> - drm_crtc_commit_put(state->crtcs[i].commit);
> - }
> -
> - state->crtcs[i].commit = NULL;
> state->crtcs[i].ptr = NULL;
> state->crtcs[i].state = NULL;
> }
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4e53aae9a1fb..80c138cbde9a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
> void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
> struct drm_atomic_state *old_state)
> {
> - struct drm_crtc_state *unused;
> + struct drm_crtc_state *new_crtc_state;
> struct drm_crtc *crtc;
> int i;
>
> - for_each_new_crtc_in_state(old_state, crtc, unused, i) {
> - struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
> + for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> + struct drm_crtc_commit *commit = new_crtc_state->commit;
> int ret;
>
> if (!commit)
> @@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> kref_init(&commit->ref);
> commit->crtc = crtc;
>
> - state->crtcs[i].commit = commit;
> + new_crtc_state->commit = commit;
>
> ret = stall_checks(crtc, nonblock);
> if (ret)
> @@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> }
> EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>
> -
> -static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> -{
> - struct drm_crtc_commit *commit;
> - int i = 0;
> -
> - list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> - /* skip the first entry, that's the current commit */
> - if (i == 1)
> - return commit;
> - i++;
> - }
> -
> - return NULL;
> -}
> -
> /**
> * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
> * @old_state: atomic state object with old state structures
> @@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> - struct drm_crtc_state *new_crtc_state;
> + struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_commit *commit;
> int i;
> long ret;
>
> - for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> - spin_lock(&crtc->commit_lock);
> - commit = preceeding_commit(crtc);
> - if (commit)
> - drm_crtc_commit_get(commit);
> - spin_unlock(&crtc->commit_lock);
> + for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> + commit = old_crtc_state->commit;
>
> if (!commit)
> continue;
> @@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> if (ret == 0)
> DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
> crtc->base.id, crtc->name);
> -
> - drm_crtc_commit_put(commit);
> }
> }
> EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> @@ -1852,15 +1830,25 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> - struct drm_crtc_state *new_crtc_state;
> + struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> struct drm_crtc_commit *commit;
> int i;
>
> - for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> - commit = old_state->crtcs[i].commit;
> + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> + commit = new_crtc_state->commit;
> if (!commit)
> continue;
>
> + /*
> + * copy new_crtc_state->commit to old_crtc_state->commit,
> + * it's unsafe to touch new_crtc_state after hw_done,
> + * but we still need to do so in cleanup_done().
> + */
> + if (old_crtc_state->commit)
> + drm_crtc_commit_put(old_crtc_state->commit);
> +
> + old_crtc_state->commit = drm_crtc_commit_get(commit);
> +
> /* backend must have consumed any event by now */
> WARN_ON(new_crtc_state->event);
> complete_all(&commit->hw_done);
> @@ -1882,13 +1870,13 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
> void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> - struct drm_crtc_state *new_crtc_state;
> + struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_commit *commit;
> int i;
> long ret;
>
> - for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> - commit = old_state->crtcs[i].commit;
> + for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> + commit = old_crtc_state->commit;
> if (WARN_ON(!commit))
> continue;
>
> @@ -2294,20 +2282,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> struct drm_private_state *old_obj_state, *new_obj_state;
>
> if (stall) {
> - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> - spin_lock(&crtc->commit_lock);
> - commit = list_first_entry_or_null(&crtc->commit_list,
> - struct drm_crtc_commit, commit_entry);
> - if (commit)
> - drm_crtc_commit_get(commit);
> - spin_unlock(&crtc->commit_lock);
> + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> + commit = old_crtc_state->commit;
>
> if (!commit)
> continue;
>
> ret = wait_for_completion_interruptible(&commit->hw_done);
> - drm_crtc_commit_put(commit);
> -
> if (ret)
> return ret;
> }
> @@ -2332,13 +2313,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> state->crtcs[i].state = old_crtc_state;
> crtc->state = new_crtc_state;
>
> - if (state->crtcs[i].commit) {
> + if (new_crtc_state->commit) {
> spin_lock(&crtc->commit_lock);
> - list_add(&state->crtcs[i].commit->commit_entry,
> + list_add(&new_crtc_state->commit->commit_entry,
> &crtc->commit_list);
> spin_unlock(&crtc->commit_lock);
>
> - state->crtcs[i].commit->event = NULL;
> + new_crtc_state->commit->event = NULL;
> }
> }
>
> @@ -3186,6 +3167,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> state->connectors_changed = false;
> state->color_mgmt_changed = false;
> state->zpos_changed = false;
> + state->commit = NULL;
> state->event = NULL;
> state->pageflip_flags = 0;
> }
> @@ -3224,6 +3206,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
> */
> void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
> {
> + if (state->commit) {
> + kfree(state->commit->event);
> + state->commit->event = NULL;
> + drm_crtc_commit_put(state->commit);
> + }
> +
> drm_property_blob_put(state->mode_blob);
> drm_property_blob_put(state->degamma_lut);
> drm_property_blob_put(state->ctm);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index f73b663c1f76..285fbc4ec360 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,7 +144,6 @@ struct __drm_planes_state {
> struct __drm_crtcs_state {
> struct drm_crtc *ptr;
> struct drm_crtc_state *state, *old_state, *new_state;
> - struct drm_crtc_commit *commit;
> s32 __user *out_fence_ptr;
> unsigned last_vblank_count;
> };
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1a642020e306..1a01ff4ea023 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -253,6 +253,15 @@ struct drm_crtc_state {
> */
> struct drm_pending_vblank_event *event;
>
> + /**
> + * @commit:
> + *
> + * This tracks how the commit for this update proceeds through the
> + * various phases. This is never cleared, except when we destroy the
> + * state, so that subsequent commits can synchronize with previous ones.
> + */
> + struct drm_crtc_commit *commit;
> +
> struct drm_atomic_state *state;
> };
>
> @@ -808,10 +817,16 @@ struct drm_crtc {
> * @commit_list:
> *
> * List of &drm_crtc_commit structures tracking pending commits.
> - * Protected by @commit_lock. This list doesn't hold its own full
> - * reference, but burrows it from the ongoing commit. Commit entries
> - * must be removed from this list once the commit is fully completed,
> - * but before it's correspoding &drm_atomic_state gets destroyed.
> + * Protected by @commit_lock. This list holds its own full reference,
> + * as does the ongoing commit.
> + *
> + * "Note that the commit for a state change is also tracked in
> + * &drm_crtc_state.commit. For accessing the immediately preceeding
> + * commit in an atomic update it is recommended to just use that
> + * pointer in the old CRTC state, since accessing that doesn't need
> + * any locking or list-walking. @commit_list should only be used to
> + * stall for framebuffer cleanup that's signalled through
> + * &drm_crtc_commit.cleanup_done."
> */
> struct list_head commit_list;
>
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2.
2017-09-04 10:48 ` [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2 Maarten Lankhorst
@ 2017-09-07 9:49 ` Daniel Vetter
2017-09-08 9:08 ` Maarten Lankhorst
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2017-09-07 9:49 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel
On Mon, Sep 04, 2017 at 12:48:33PM +0200, Maarten Lankhorst wrote:
> The next commit removes the wait for flip_done in in
> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
> to pass. Instead of using complicated vblank tracking which ends
> up being ignored anyway, call the correct atomic helper. :)
>
> Changes since v1:
> - Always call drm_atomic_helper_wait_for_flip_done,
> even for legacy cursor updates. (danvet)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_display.c | 84 +++---------------------------------
> 2 files changed, 8 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 38bd08f2539b..79533ba6f387 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
> struct drm_atomic_state *old_state);
> void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
> struct drm_atomic_state *old_state);
> - void (*update_crtcs)(struct drm_atomic_state *state,
> - unsigned int *crtc_vblank_mask);
> + void (*update_crtcs)(struct drm_atomic_state *state);
> void (*audio_codec_enable)(struct drm_connector *connector,
> struct intel_encoder *encoder,
> const struct drm_display_mode *adjusted_mode);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 216cd9e0e08f..a6cf1c20c712 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> return dev->driver->get_vblank_counter(dev, crtc->pipe);
> }
>
> -static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
> - struct drm_i915_private *dev_priv,
> - unsigned crtc_mask)
> -{
> - unsigned last_vblank_count[I915_MAX_PIPES];
> - enum pipe pipe;
> - int ret;
> -
> - if (!crtc_mask)
> - return;
> -
> - for_each_pipe(dev_priv, pipe) {
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> - pipe);
> -
> - if (!((1 << pipe) & crtc_mask))
> - continue;
> -
> - ret = drm_crtc_vblank_get(&crtc->base);
> - if (WARN_ON(ret != 0)) {
> - crtc_mask &= ~(1 << pipe);
> - continue;
> - }
> -
> - last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
> - }
> -
> - for_each_pipe(dev_priv, pipe) {
> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> - pipe);
> - long lret;
> -
> - if (!((1 << pipe) & crtc_mask))
> - continue;
> -
> - lret = wait_event_timeout(dev->vblank[pipe].queue,
> - last_vblank_count[pipe] !=
> - drm_crtc_vblank_count(&crtc->base),
> - msecs_to_jiffies(50));
> -
> - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
> -
> - drm_crtc_vblank_put(&crtc->base);
> - }
> -}
> -
> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
> -{
> - /* fb updated, need to unpin old fb */
> - if (crtc_state->fb_changed)
> - return true;
> -
> - /* wm changes, need vblank before final wm's */
> - if (crtc_state->update_wm_post)
> - return true;
> -
> - if (crtc_state->wm.need_postvbl_update)
> - return true;
> -
> - return false;
> -}
> -
> static void intel_update_crtc(struct drm_crtc *crtc,
> struct drm_atomic_state *state,
> struct drm_crtc_state *old_crtc_state,
> - struct drm_crtc_state *new_crtc_state,
> - unsigned int *crtc_vblank_mask)
> + struct drm_crtc_state *new_crtc_state)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> }
>
> drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
> -
> - if (needs_vblank_wait(pipe_config))
> - *crtc_vblank_mask |= drm_crtc_mask(crtc);
> }
>
> -static void intel_update_crtcs(struct drm_atomic_state *state,
> - unsigned int *crtc_vblank_mask)
> +static void intel_update_crtcs(struct drm_atomic_state *state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
> continue;
>
> intel_update_crtc(crtc, state, old_crtc_state,
> - new_crtc_state, crtc_vblank_mask);
> + new_crtc_state);
> }
> }
>
> -static void skl_update_crtcs(struct drm_atomic_state *state,
> - unsigned int *crtc_vblank_mask)
> +static void skl_update_crtcs(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> @@ -12306,7 +12238,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> vbl_wait = true;
>
> intel_update_crtc(crtc, state, old_crtc_state,
> - new_crtc_state, crtc_vblank_mask);
> + new_crtc_state);
>
> if (vbl_wait)
> intel_wait_for_vblank(dev_priv, pipe);
> @@ -12368,7 +12300,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> struct intel_crtc_state *intel_cstate;
> bool hw_check = intel_state->modeset;
> u64 put_domains[I915_MAX_PIPES] = {};
> - unsigned crtc_vblank_mask = 0;
> int i;
>
> intel_atomic_commit_fence_wait(intel_state);
> @@ -12458,7 +12389,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> }
>
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> - dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
> + dev_priv->display.update_crtcs(state);
>
> /* FIXME: We should call drm_atomic_helper_commit_hw_done() here
> * already, but still need the state for the delayed optimization. To
> @@ -12469,8 +12400,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> * - switch over to the vblank wait helper in the core after that since
> * we don't need out special handling any more.
> */
> - if (!state->legacy_cursor_update)
> - intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
> + drm_atomic_helper_wait_for_flip_done(dev, state);
Superflous if dropped, I'm happy.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> /*
> * Now that the vblank has passed, we can go ahead and program the
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation
2017-09-04 10:48 ` [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation Maarten Lankhorst
@ 2017-09-07 9:59 ` Daniel Vetter
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2017-09-07 9:59 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, dri-devel
On Mon, Sep 04, 2017 at 12:48:36PM +0200, Maarten Lankhorst wrote:
> This will allow code to do x->commit = drm_crtc_commit_get(commit),
> making it clearer where references are used.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 3 +--
> include/drm/drm_atomic.h | 6 +++++-
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1f5cdafb050e..04629d883114 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1633,8 +1633,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
> return -EBUSY;
> }
> } else if (i == 1) {
> - stall_commit = commit;
> - drm_crtc_commit_get(stall_commit);
> + stall_commit = drm_crtc_commit_get(commit);
> break;
> }
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 285fbc4ec360..a80a8dadef00 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -251,10 +251,14 @@ void __drm_crtc_commit_free(struct kref *kref);
> * @commit: CRTC commit
> *
> * Increases the reference of @commit.
> + *
> + * Returns:
> + * The pointer to @commit, with reference increased.
> */
> -static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
> +static inline struct drm_crtc_commit *drm_crtc_commit_get(struct drm_crtc_commit *commit)
> {
> kref_get(&commit->ref);
> + return commit;
> }
>
> /**
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
2017-09-04 10:48 ` [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3 Maarten Lankhorst
@ 2017-09-07 10:05 ` Daniel Vetter
2017-09-07 11:08 ` Maarten Lankhorst
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2017-09-07 10:05 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx, Laurent Pinchart, dri-devel
On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote:
> Currently we neatly track the crtc state, but forget to look at
> plane/connector state.
>
> When doing a nonblocking modeset, immediately followed by a setprop
> before the modeset completes, the setprop will see the modesets new
> state as the old state and free it.
>
> This has to be solved by waiting for hw_done on the connector, even
> if it's not assigned to a crtc. When a connector is unbound we take
> the last crtc commit, and when it stays unbound we create a new
> fake crtc commit for that gets signaled on hw_done for all the
> planes/connectors.
>
> We wait for it the same way as we do for crtc's, which will make
> sure we never run into a use-after-free situation.
>
> Changes since v1:
> - Only create a single disable commit. (danvet)
> - Fix leak in intel_legacy_cursor_update.
> Changes since v2:
> - Make reference counting in drm_atomic_helper_setup_commit
> more obvious. (pinchartl)
> - Call cleanup_done for fake commit. (danvet)
> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
> - Add comment to drm_atomic_helper_swap_state. (pinchartl)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 4 +
> drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_display.c | 2 +
> include/drm/drm_atomic.h | 12 +++
> include/drm/drm_connector.h | 7 ++
> include/drm/drm_plane.h | 7 ++
> 6 files changed, 198 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2cce48f203e0..75f5f74de9bf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> }
> state->num_private_objs = 0;
>
> + if (state->fake_commit) {
> + drm_crtc_commit_put(state->fake_commit);
> + state->fake_commit = NULL;
> + }
> }
> EXPORT_SYMBOL(drm_atomic_state_default_clear);
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 04629d883114..c81d46927a74 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
> drm_crtc_commit_put(commit);
> }
>
> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
> +{
> + init_completion(&commit->flip_done);
> + init_completion(&commit->hw_done);
> + init_completion(&commit->cleanup_done);
> + INIT_LIST_HEAD(&commit->commit_entry);
> + kref_init(&commit->ref);
> + commit->crtc = crtc;
> +}
> +
> +static struct drm_crtc_commit *
> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
Bikeshed: Would be nice if this function directly increases the refcount,
instead of imposing this on all callers. Would need a rename too like
crtc_or_fake_commit_get().
But since this bug is randomly killing our hsw CI and causing lots of
noise better to push as-is and polish later on.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +{
> + if (crtc) {
> + struct drm_crtc_state *new_crtc_state;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> + return new_crtc_state->commit;
> + }
> +
> + if (!state->fake_commit) {
> + state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
> + if (!state->fake_commit)
> + return NULL;
> +
> + init_commit(state->fake_commit, NULL);
> + }
> +
> + return state->fake_commit;
> +}
> +
> /**
> * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
> * @state: new modeset state to be committed
> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *old_conn_state, *new_conn_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
> struct drm_crtc_commit *commit;
> int i, ret;
>
> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> if (!commit)
> return -ENOMEM;
>
> - init_completion(&commit->flip_done);
> - init_completion(&commit->hw_done);
> - init_completion(&commit->cleanup_done);
> - INIT_LIST_HEAD(&commit->commit_entry);
> - kref_init(&commit->ref);
> - commit->crtc = crtc;
> + init_commit(commit, crtc);
>
> new_crtc_state->commit = commit;
>
> @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> drm_crtc_commit_get(commit);
> }
>
> + for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
> + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> + if (new_conn_state->crtc)
> + continue;
> +
> + /* Userspace is not allowed to get ahead of the previous
> + * commit with nonblocking ones. */
> + if (nonblock && old_conn_state->commit &&
> + !try_wait_for_completion(&old_conn_state->commit->flip_done))
> + return -EBUSY;
> +
> + commit = crtc_or_fake_commit(state, old_conn_state->crtc);
> + if (!commit)
> + return -ENOMEM;
> +
> + new_conn_state->commit = drm_crtc_commit_get(commit);
> + }
> +
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> + if (new_plane_state->crtc)
> + continue;
> +
> + /* Userspace is not allowed to get ahead of the previous
> + * commit with nonblocking ones. */
> + if (nonblock && old_plane_state->commit &&
> + !try_wait_for_completion(&old_plane_state->commit->flip_done))
> + return -EBUSY;
> +
> + commit = crtc_or_fake_commit(state, old_plane_state->crtc);
> + if (!commit)
> + return -ENOMEM;
> +
> + new_plane_state->commit = drm_crtc_commit_get(commit);
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *old_conn_state;
> struct drm_crtc_commit *commit;
> int i;
> long ret;
> @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
> crtc->base.id, crtc->name);
> }
> +
> + for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
> + commit = old_conn_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_timeout(&commit->hw_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
> + conn->base.id, conn->name);
> +
> + /* Currently no support for overwriting flips, hence
> + * stall for previous one to execute completely. */
> + ret = wait_for_completion_timeout(&commit->flip_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
> + conn->base.id, conn->name);
> + }
> +
> + for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
> + commit = old_plane_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_timeout(&commit->hw_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
> + plane->base.id, plane->name);
> +
> + /* Currently no support for overwriting flips, hence
> + * stall for previous one to execute completely. */
> + ret = wait_for_completion_timeout(&commit->flip_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
> + plane->base.id, plane->name);
> + }
> }
> EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>
> @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
> WARN_ON(new_crtc_state->event);
> complete_all(&commit->hw_done);
> }
> +
> + if (old_state->fake_commit) {
> + complete_all(&old_state->fake_commit->hw_done);
> + complete_all(&old_state->fake_commit->flip_done);
> + }
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>
> @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
> list_del(&commit->commit_entry);
> spin_unlock(&crtc->commit_lock);
> }
> +
> + if (old_state->fake_commit)
> + complete_all(&old_state->fake_commit->cleanup_done);
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
>
> @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> struct drm_private_state *old_obj_state, *new_obj_state;
>
> if (stall) {
> + /*
> + * We have to stall for hw_done here before
> + * drm_atomic_helper_wait_for_dependencies() because flip
> + * depth > 1 is not yet supported by all drivers. As long as
> + * obj->state is directly dereferenced anywhere in the drivers
> + * atomic_commit_tail function, then it's unsafe to swap state
> + * before drm_atomic_helper_commit_hw_done() is called.
> + */
> +
> for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> commit = old_crtc_state->commit;
>
> @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> if (ret)
> return ret;
> }
> +
> + for_each_old_connector_in_state(state, connector, old_conn_state, i) {
> + commit = old_conn_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_interruptible(&commit->hw_done);
> + if (ret)
> + return ret;
> + }
> +
> + for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> + commit = old_plane_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_interruptible(&commit->hw_done);
> + if (ret)
> + return ret;
> + }
> }
>
> for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
> @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
> drm_framebuffer_get(state->fb);
>
> state->fence = NULL;
> + state->commit = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>
> @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>
> if (state->fence)
> dma_fence_put(state->fence);
> +
> + if (state->commit)
> + drm_crtc_commit_put(state->commit);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
> memcpy(state, connector->state, sizeof(*state));
> if (state->crtc)
> drm_connector_get(connector);
> + state->commit = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>
> @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
> {
> if (state->crtc)
> drm_connector_put(state->connector);
> +
> + if (state->commit)
> + drm_crtc_commit_put(state->commit);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a6cf1c20c712..7abbc761a635 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>
> /* Swap plane state */
> new_plane_state->fence = old_plane_state->fence;
> + new_plane_state->commit = old_plane_state->commit;
> *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> new_plane_state->fence = NULL;
> + new_plane_state->commit = NULL;
> new_plane_state->fb = old_fb;
> to_intel_plane_state(new_plane_state)->vma = NULL;
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index a80a8dadef00..07a71daa3582 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -236,6 +236,18 @@ struct drm_atomic_state {
> struct drm_modeset_acquire_ctx *acquire_ctx;
>
> /**
> + * @fake_commit:
> + *
> + * Used for signaling unbound planes/connectors.
> + * When a connector or plane is not bound to any CRTC, it's still important
> + * to preserve linearity to prevent the atomic states from being freed to early.
> + *
> + * This commit (if set) is not bound to any crtc, but will be completed when
> + * drm_atomic_helper_commit_hw_done() is called.
> + */
> + struct drm_crtc_commit *fake_commit;
> +
> + /**
> * @commit_work:
> *
> * Work item which can be used by the driver or helpers to execute the
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ea8da401c93c..8837649d16e8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -347,6 +347,13 @@ struct drm_connector_state {
>
> struct drm_atomic_state *state;
>
> + /**
> + * @commit: Tracks the pending commit to prevent use-after-free conditions.
> + *
> + * Is only set when @crtc is NULL.
> + */
> + struct drm_crtc_commit *commit;
> +
> struct drm_tv_connector_state tv;
>
> /**
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 73f90f9d057f..7d96116fd4c4 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -123,6 +123,13 @@ struct drm_plane_state {
> */
> bool visible;
>
> + /**
> + * @commit: Tracks the pending commit to prevent use-after-free conditions.
> + *
> + * Is only set when @crtc is NULL.
> + */
> + struct drm_crtc_commit *commit;
> +
> struct drm_atomic_state *state;
> };
>
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
2017-09-07 10:05 ` Daniel Vetter
@ 2017-09-07 11:08 ` Maarten Lankhorst
0 siblings, 0 replies; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-07 11:08 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Laurent Pinchart, dri-devel
Op 07-09-17 om 12:05 schreef Daniel Vetter:
> On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote:
>> Currently we neatly track the crtc state, but forget to look at
>> plane/connector state.
>>
>> When doing a nonblocking modeset, immediately followed by a setprop
>> before the modeset completes, the setprop will see the modesets new
>> state as the old state and free it.
>>
>> This has to be solved by waiting for hw_done on the connector, even
>> if it's not assigned to a crtc. When a connector is unbound we take
>> the last crtc commit, and when it stays unbound we create a new
>> fake crtc commit for that gets signaled on hw_done for all the
>> planes/connectors.
>>
>> We wait for it the same way as we do for crtc's, which will make
>> sure we never run into a use-after-free situation.
>>
>> Changes since v1:
>> - Only create a single disable commit. (danvet)
>> - Fix leak in intel_legacy_cursor_update.
>> Changes since v2:
>> - Make reference counting in drm_atomic_helper_setup_commit
>> more obvious. (pinchartl)
>> - Call cleanup_done for fake commit. (danvet)
>> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
>> - Add comment to drm_atomic_helper_swap_state. (pinchartl)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 4 +
>> drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/intel_display.c | 2 +
>> include/drm/drm_atomic.h | 12 +++
>> include/drm/drm_connector.h | 7 ++
>> include/drm/drm_plane.h | 7 ++
>> 6 files changed, 198 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 2cce48f203e0..75f5f74de9bf 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>> }
>> state->num_private_objs = 0;
>>
>> + if (state->fake_commit) {
>> + drm_crtc_commit_put(state->fake_commit);
>> + state->fake_commit = NULL;
>> + }
>> }
>> EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 04629d883114..c81d46927a74 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
>> drm_crtc_commit_put(commit);
>> }
>>
>> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
>> +{
>> + init_completion(&commit->flip_done);
>> + init_completion(&commit->hw_done);
>> + init_completion(&commit->cleanup_done);
>> + INIT_LIST_HEAD(&commit->commit_entry);
>> + kref_init(&commit->ref);
>> + commit->crtc = crtc;
>> +}
>> +
>> +static struct drm_crtc_commit *
>> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> Bikeshed: Would be nice if this function directly increases the refcount,
> instead of imposing this on all callers. Would need a rename too like
> crtc_or_fake_commit_get().
>
> But since this bug is randomly killing our hsw CI and causing lots of
> noise better to push as-is and polish later on.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I chose not to, to make it explicit that a extra refcount is used on the state object.
But sending one final version to trybot to make sure that things don't blow up with the merge conflicts in patch 6. :)
>> +{
>> + if (crtc) {
>> + struct drm_crtc_state *new_crtc_state;
>> +
>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> +
>> + return new_crtc_state->commit;
>> + }
>> +
>> + if (!state->fake_commit) {
>> + state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
>> + if (!state->fake_commit)
>> + return NULL;
>> +
>> + init_commit(state->fake_commit, NULL);
>> + }
>> +
>> + return state->fake_commit;
>> +}
>> +
>> /**
>> * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
>> * @state: new modeset state to be committed
>> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>> {
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> + struct drm_connector *conn;
>> + struct drm_connector_state *old_conn_state, *new_conn_state;
>> + struct drm_plane *plane;
>> + struct drm_plane_state *old_plane_state, *new_plane_state;
>> struct drm_crtc_commit *commit;
>> int i, ret;
>>
>> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>> if (!commit)
>> return -ENOMEM;
>>
>> - init_completion(&commit->flip_done);
>> - init_completion(&commit->hw_done);
>> - init_completion(&commit->cleanup_done);
>> - INIT_LIST_HEAD(&commit->commit_entry);
>> - kref_init(&commit->ref);
>> - commit->crtc = crtc;
>> + init_commit(commit, crtc);
>>
>> new_crtc_state->commit = commit;
>>
>> @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>> drm_crtc_commit_get(commit);
>> }
>>
>> + for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
>> + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
>> + if (new_conn_state->crtc)
>> + continue;
>> +
>> + /* Userspace is not allowed to get ahead of the previous
>> + * commit with nonblocking ones. */
>> + if (nonblock && old_conn_state->commit &&
>> + !try_wait_for_completion(&old_conn_state->commit->flip_done))
>> + return -EBUSY;
>> +
>> + commit = crtc_or_fake_commit(state, old_conn_state->crtc);
>> + if (!commit)
>> + return -ENOMEM;
>> +
>> + new_conn_state->commit = drm_crtc_commit_get(commit);
>> + }
>> +
>> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>> + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
>> + if (new_plane_state->crtc)
>> + continue;
>> +
>> + /* Userspace is not allowed to get ahead of the previous
>> + * commit with nonblocking ones. */
>> + if (nonblock && old_plane_state->commit &&
>> + !try_wait_for_completion(&old_plane_state->commit->flip_done))
>> + return -EBUSY;
>> +
>> + commit = crtc_or_fake_commit(state, old_plane_state->crtc);
>> + if (!commit)
>> + return -ENOMEM;
>> +
>> + new_plane_state->commit = drm_crtc_commit_get(commit);
>> + }
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>> @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>> {
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *old_crtc_state;
>> + struct drm_plane *plane;
>> + struct drm_plane_state *old_plane_state;
>> + struct drm_connector *conn;
>> + struct drm_connector_state *old_conn_state;
>> struct drm_crtc_commit *commit;
>> int i;
>> long ret;
>> @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>> DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>> crtc->base.id, crtc->name);
>> }
>> +
>> + for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>> + commit = old_conn_state->commit;
>> +
>> + if (!commit)
>> + continue;
>> +
>> + ret = wait_for_completion_timeout(&commit->hw_done,
>> + 10*HZ);
>> + if (ret == 0)
>> + DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
>> + conn->base.id, conn->name);
>> +
>> + /* Currently no support for overwriting flips, hence
>> + * stall for previous one to execute completely. */
>> + ret = wait_for_completion_timeout(&commit->flip_done,
>> + 10*HZ);
>> + if (ret == 0)
>> + DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
>> + conn->base.id, conn->name);
>> + }
>> +
>> + for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
>> + commit = old_plane_state->commit;
>> +
>> + if (!commit)
>> + continue;
>> +
>> + ret = wait_for_completion_timeout(&commit->hw_done,
>> + 10*HZ);
>> + if (ret == 0)
>> + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
>> + plane->base.id, plane->name);
>> +
>> + /* Currently no support for overwriting flips, hence
>> + * stall for previous one to execute completely. */
>> + ret = wait_for_completion_timeout(&commit->flip_done,
>> + 10*HZ);
>> + if (ret == 0)
>> + DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
>> + plane->base.id, plane->name);
>> + }
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>>
>> @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>> WARN_ON(new_crtc_state->event);
>> complete_all(&commit->hw_done);
>> }
>> +
>> + if (old_state->fake_commit) {
>> + complete_all(&old_state->fake_commit->hw_done);
>> + complete_all(&old_state->fake_commit->flip_done);
>> + }
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>>
>> @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>> list_del(&commit->commit_entry);
>> spin_unlock(&crtc->commit_lock);
>> }
>> +
>> + if (old_state->fake_commit)
>> + complete_all(&old_state->fake_commit->cleanup_done);
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
>>
>> @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>> struct drm_private_state *old_obj_state, *new_obj_state;
>>
>> if (stall) {
>> + /*
>> + * We have to stall for hw_done here before
>> + * drm_atomic_helper_wait_for_dependencies() because flip
>> + * depth > 1 is not yet supported by all drivers. As long as
>> + * obj->state is directly dereferenced anywhere in the drivers
>> + * atomic_commit_tail function, then it's unsafe to swap state
>> + * before drm_atomic_helper_commit_hw_done() is called.
>> + */
>> +
>> for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>> commit = old_crtc_state->commit;
>>
>> @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>> if (ret)
>> return ret;
>> }
>> +
>> + for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>> + commit = old_conn_state->commit;
>> +
>> + if (!commit)
>> + continue;
>> +
>> + ret = wait_for_completion_interruptible(&commit->hw_done);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>> + commit = old_plane_state->commit;
>> +
>> + if (!commit)
>> + continue;
>> +
>> + ret = wait_for_completion_interruptible(&commit->hw_done);
>> + if (ret)
>> + return ret;
>> + }
>> }
>>
>> for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
>> @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>> drm_framebuffer_get(state->fb);
>>
>> state->fence = NULL;
>> + state->commit = NULL;
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>>
>> @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>
>> if (state->fence)
>> dma_fence_put(state->fence);
>> +
>> + if (state->commit)
>> + drm_crtc_commit_put(state->commit);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>
>> @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>> memcpy(state, connector->state, sizeof(*state));
>> if (state->crtc)
>> drm_connector_get(connector);
>> + state->commit = NULL;
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>>
>> @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>> {
>> if (state->crtc)
>> drm_connector_put(state->connector);
>> +
>> + if (state->commit)
>> + drm_crtc_commit_put(state->commit);
>> }
>> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a6cf1c20c712..7abbc761a635 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>
>> /* Swap plane state */
>> new_plane_state->fence = old_plane_state->fence;
>> + new_plane_state->commit = old_plane_state->commit;
>> *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
>> new_plane_state->fence = NULL;
>> + new_plane_state->commit = NULL;
>> new_plane_state->fb = old_fb;
>> to_intel_plane_state(new_plane_state)->vma = NULL;
>>
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index a80a8dadef00..07a71daa3582 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -236,6 +236,18 @@ struct drm_atomic_state {
>> struct drm_modeset_acquire_ctx *acquire_ctx;
>>
>> /**
>> + * @fake_commit:
>> + *
>> + * Used for signaling unbound planes/connectors.
>> + * When a connector or plane is not bound to any CRTC, it's still important
>> + * to preserve linearity to prevent the atomic states from being freed to early.
>> + *
>> + * This commit (if set) is not bound to any crtc, but will be completed when
>> + * drm_atomic_helper_commit_hw_done() is called.
>> + */
>> + struct drm_crtc_commit *fake_commit;
>> +
>> + /**
>> * @commit_work:
>> *
>> * Work item which can be used by the driver or helpers to execute the
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index ea8da401c93c..8837649d16e8 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -347,6 +347,13 @@ struct drm_connector_state {
>>
>> struct drm_atomic_state *state;
>>
>> + /**
>> + * @commit: Tracks the pending commit to prevent use-after-free conditions.
>> + *
>> + * Is only set when @crtc is NULL.
>> + */
>> + struct drm_crtc_commit *commit;
>> +
>> struct drm_tv_connector_state tv;
>>
>> /**
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 73f90f9d057f..7d96116fd4c4 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -123,6 +123,13 @@ struct drm_plane_state {
>> */
>> bool visible;
>>
>> + /**
>> + * @commit: Tracks the pending commit to prevent use-after-free conditions.
>> + *
>> + * Is only set when @crtc is NULL.
>> + */
>> + struct drm_crtc_commit *commit;
>> +
>> struct drm_atomic_state *state;
>> };
>>
>> --
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2.
2017-09-07 9:49 ` Daniel Vetter
@ 2017-09-08 9:08 ` Maarten Lankhorst
0 siblings, 0 replies; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-08 9:08 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dri-devel
Op 07-09-17 om 11:49 schreef Daniel Vetter:
> On Mon, Sep 04, 2017 at 12:48:33PM +0200, Maarten Lankhorst wrote:
>> The next commit removes the wait for flip_done in in
>> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
>> to pass. Instead of using complicated vblank tracking which ends
>> up being ignored anyway, call the correct atomic helper. :)
>>
>> Changes since v1:
>> - Always call drm_atomic_helper_wait_for_flip_done,
>> even for legacy cursor updates. (danvet)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 3 +-
>> drivers/gpu/drm/i915/intel_display.c | 84 +++---------------------------------
>> 2 files changed, 8 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 38bd08f2539b..79533ba6f387 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
>> struct drm_atomic_state *old_state);
>> void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>> struct drm_atomic_state *old_state);
>> - void (*update_crtcs)(struct drm_atomic_state *state,
>> - unsigned int *crtc_vblank_mask);
>> + void (*update_crtcs)(struct drm_atomic_state *state);
>> void (*audio_codec_enable)(struct drm_connector *connector,
>> struct intel_encoder *encoder,
>> const struct drm_display_mode *adjusted_mode);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 216cd9e0e08f..a6cf1c20c712 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>> return dev->driver->get_vblank_counter(dev, crtc->pipe);
>> }
>>
>> -static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
>> - struct drm_i915_private *dev_priv,
>> - unsigned crtc_mask)
>> -{
>> - unsigned last_vblank_count[I915_MAX_PIPES];
>> - enum pipe pipe;
>> - int ret;
>> -
>> - if (!crtc_mask)
>> - return;
>> -
>> - for_each_pipe(dev_priv, pipe) {
>> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
>> - pipe);
>> -
>> - if (!((1 << pipe) & crtc_mask))
>> - continue;
>> -
>> - ret = drm_crtc_vblank_get(&crtc->base);
>> - if (WARN_ON(ret != 0)) {
>> - crtc_mask &= ~(1 << pipe);
>> - continue;
>> - }
>> -
>> - last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
>> - }
>> -
>> - for_each_pipe(dev_priv, pipe) {
>> - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
>> - pipe);
>> - long lret;
>> -
>> - if (!((1 << pipe) & crtc_mask))
>> - continue;
>> -
>> - lret = wait_event_timeout(dev->vblank[pipe].queue,
>> - last_vblank_count[pipe] !=
>> - drm_crtc_vblank_count(&crtc->base),
>> - msecs_to_jiffies(50));
>> -
>> - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
>> -
>> - drm_crtc_vblank_put(&crtc->base);
>> - }
>> -}
>> -
>> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>> -{
>> - /* fb updated, need to unpin old fb */
>> - if (crtc_state->fb_changed)
>> - return true;
>> -
>> - /* wm changes, need vblank before final wm's */
>> - if (crtc_state->update_wm_post)
>> - return true;
>> -
>> - if (crtc_state->wm.need_postvbl_update)
>> - return true;
>> -
>> - return false;
>> -}
>> -
>> static void intel_update_crtc(struct drm_crtc *crtc,
>> struct drm_atomic_state *state,
>> struct drm_crtc_state *old_crtc_state,
>> - struct drm_crtc_state *new_crtc_state,
>> - unsigned int *crtc_vblank_mask)
>> + struct drm_crtc_state *new_crtc_state)
>> {
>> struct drm_device *dev = crtc->dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>> }
>>
>> drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
>> -
>> - if (needs_vblank_wait(pipe_config))
>> - *crtc_vblank_mask |= drm_crtc_mask(crtc);
>> }
>>
>> -static void intel_update_crtcs(struct drm_atomic_state *state,
>> - unsigned int *crtc_vblank_mask)
>> +static void intel_update_crtcs(struct drm_atomic_state *state)
>> {
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> @@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>> continue;
>>
>> intel_update_crtc(crtc, state, old_crtc_state,
>> - new_crtc_state, crtc_vblank_mask);
>> + new_crtc_state);
>> }
>> }
>>
>> -static void skl_update_crtcs(struct drm_atomic_state *state,
>> - unsigned int *crtc_vblank_mask)
>> +static void skl_update_crtcs(struct drm_atomic_state *state)
>> {
>> struct drm_i915_private *dev_priv = to_i915(state->dev);
>> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> @@ -12306,7 +12238,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>> vbl_wait = true;
>>
>> intel_update_crtc(crtc, state, old_crtc_state,
>> - new_crtc_state, crtc_vblank_mask);
>> + new_crtc_state);
>>
>> if (vbl_wait)
>> intel_wait_for_vblank(dev_priv, pipe);
>> @@ -12368,7 +12300,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>> struct intel_crtc_state *intel_cstate;
>> bool hw_check = intel_state->modeset;
>> u64 put_domains[I915_MAX_PIPES] = {};
>> - unsigned crtc_vblank_mask = 0;
>> int i;
>>
>> intel_atomic_commit_fence_wait(intel_state);
>> @@ -12458,7 +12389,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>> }
>>
>> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
>> - dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
>> + dev_priv->display.update_crtcs(state);
>>
>> /* FIXME: We should call drm_atomic_helper_commit_hw_done() here
>> * already, but still need the state for the delayed optimization. To
>> @@ -12469,8 +12400,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>> * - switch over to the vblank wait helper in the core after that since
>> * we don't need out special handling any more.
>> */
>> - if (!state->legacy_cursor_update)
>> - intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
>> + drm_atomic_helper_wait_for_flip_done(dev, state);
> Superflous if dropped, I'm happy.
Pushed the series, thanks for review. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2.
[not found] ` <20170904104838.23822-7-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
@ 2017-09-24 14:33 ` Dmitry Osipenko
[not found] ` <54467116-df02-e6ad-ac14-90aa79e164e4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 23+ messages in thread
From: Dmitry Osipenko @ 2017-09-24 14:33 UTC (permalink / raw)
To: Maarten Lankhorst, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Gustavo Padovan, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
On 04.09.2017 13:48, Maarten Lankhorst wrote:
> By always keeping track of the last commit in plane_state, we know
> whether there is an active update on the plane or not. With that
> information we can reject the fast update, and force the slowpath
> to be used as was originally intended.
>
> We cannot use plane_state->crtc->state here, because this only mentions
> the most recent commit for the crtc, but not the planes that were part
> of it. We specifically care about what the last commit involving this
> plane is, which can only be tracked with a pointer in the plane state.
>
> Changes since v1:
> - Clean up the whole function here, instead of partially earlier.
> - Add mention in the commit message why we need commit in plane_state.
> - Swap plane->state in intel_legacy_cursor_update, instead of
> reassigning all variables. With this commit We know that the cursor
> is not part of any active commits so this hack can be removed.
>
> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++--------------------------
> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
> include/drm/drm_plane.h | 5 ++--
> 3 files changed, 35 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c81d46927a74..a2cd432d8b2d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> - struct drm_crtc_commit *commit;
> - struct drm_plane *__plane, *plane = NULL;
> - struct drm_plane_state *__plane_state, *plane_state = NULL;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
> const struct drm_plane_helper_funcs *funcs;
> - int i, j, n_planes = 0;
> + int i, n_planes = 0;
>
> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> if (drm_atomic_crtc_needs_modeset(crtc_state))
> return -EINVAL;
> }
>
> - for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> n_planes++;
> - plane = __plane;
> - plane_state = __plane_state;
> - }
>
> /* FIXME: we support only single plane updates for now */
> - if (!plane || n_planes != 1)
> + if (n_planes != 1)
> return -EINVAL;
>
> - if (!plane_state->crtc)
> + if (!new_plane_state->crtc)
> return -EINVAL;
>
Hello,
It looks like a NULL-checking of new_plane_state is missed here.
[ 70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
(drm_atomic_helper_check+0x4c/0x64)
[ 70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
(drm_atomic_check_only+0x2f4/0x56c)
[ 70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
(drm_atomic_commit+0x10/0x50)
[ 70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
(drm_atomic_helper_update_plane+0xf0/0x100)
[ 70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
(__setplane_internal+0x178/0x214)
[ 70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
(drm_mode_cursor_universal+0x118/0x1a8)
[ 70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
(drm_mode_cursor_common+0x174/0x1ec)
[ 70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
(drm_mode_cursor_ioctl+0x58/0x60)
[ 70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
(drm_ioctl+0x198/0x368)
[ 70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
[ 70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
[ 70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
(ret_fast_syscall+0x0/0x48)
It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
Tegra cursor patches sometime soon.
This variant works for me:
if (!new_plane_state || !new_plane_state->crtc)
return -EINVAL;
> funcs = plane->helper_private;
> if (!funcs->atomic_async_update)
> return -EINVAL;
>
> - if (plane_state->fence)
> + if (new_plane_state->fence)
> return -EINVAL;
>
> /*
> @@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> * the plane. This prevents our async update's changes from getting
> * overridden by a previous synchronous update's state.
> */
> - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> - if (plane->crtc != crtc)
> - continue;
> -
> - spin_lock(&crtc->commit_lock);
> - commit = list_first_entry_or_null(&crtc->commit_list,
> - struct drm_crtc_commit,
> - commit_entry);
> - if (!commit) {
> - spin_unlock(&crtc->commit_lock);
> - continue;
> - }
> - spin_unlock(&crtc->commit_lock);
> -
> - if (!crtc->state->state)
> - continue;
> + if (old_plane_state->commit &&
> + !try_wait_for_completion(&old_plane_state->commit->hw_done))
> + return -EBUSY;
>
> - for_each_plane_in_state(crtc->state->state, __plane,
> - __plane_state, j) {
> - if (__plane == plane)
> - return -EINVAL;
> - }
> - }
> -
> - return funcs->atomic_async_check(plane, plane_state);
> + return funcs->atomic_async_check(plane, new_plane_state);
> }
> EXPORT_SYMBOL(drm_atomic_helper_async_check);
>
> @@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> }
>
> for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> - /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> - if (new_plane_state->crtc)
> - continue;
> + /*
> + * Unlike connectors, always track planes explicitly for
> + * async pageflip support.
> + */
>
> /* Userspace is not allowed to get ahead of the previous
> * commit with nonblocking ones. */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7abbc761a635..0add029d95f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13064,6 +13064,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> goto slow;
>
> old_plane_state = plane->state;
> + /*
> + * Don't do an async update if there is an outstanding commit modifying
> + * the plane. This prevents our async update's changes from getting
> + * overridden by a previous synchronous update's state.
> + */
> + if (old_plane_state->commit &&
> + !try_wait_for_completion(&old_plane_state->commit->hw_done))
> + goto slow;
>
> /*
> * If any parameters change that may affect watermarks,
> @@ -13125,19 +13133,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> }
>
> old_fb = old_plane_state->fb;
> - old_vma = to_intel_plane_state(old_plane_state)->vma;
>
> i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> intel_plane->frontbuffer_bit);
>
> /* Swap plane state */
> - new_plane_state->fence = old_plane_state->fence;
> - new_plane_state->commit = old_plane_state->commit;
> - *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> - new_plane_state->fence = NULL;
> - new_plane_state->commit = NULL;
> - new_plane_state->fb = old_fb;
> - to_intel_plane_state(new_plane_state)->vma = NULL;
> + plane->state = new_plane_state;
>
> if (plane->state->visible) {
> trace_intel_update_plane(plane, to_intel_crtc(crtc));
> @@ -13149,13 +13150,19 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
> }
>
> - if (old_vma)
> + old_vma = to_intel_plane_state(old_plane_state)->vma;
> + if (old_vma) {
> + to_intel_plane_state(old_plane_state)->vma = NULL;
> intel_unpin_fb_vma(old_vma);
> + }
>
> out_unlock:
> mutex_unlock(&dev_priv->drm.struct_mutex);
> out_free:
> - intel_plane_destroy_state(plane, new_plane_state);
> + if (ret)
> + intel_plane_destroy_state(plane, new_plane_state);
> + else
> + intel_plane_destroy_state(plane, old_plane_state);
> return ret;
>
> slow:
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 7d96116fd4c4..feb9941d0cdb 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -124,9 +124,10 @@ struct drm_plane_state {
> bool visible;
>
> /**
> - * @commit: Tracks the pending commit to prevent use-after-free conditions.
> + * @commit: Tracks the pending commit to prevent use-after-free conditions,
> + * and for async plane updates.
> *
> - * Is only set when @crtc is NULL.
> + * May be NULL.
> */
> struct drm_crtc_commit *commit;
>
>
--
Dmitry
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/atomic: Make async plane update checks actually work as intended.
[not found] ` <54467116-df02-e6ad-ac14-90aa79e164e4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-09-25 6:43 ` Maarten Lankhorst
[not found] ` <a7d5fb04-c2f5-b743-5940-a1bd181d780d-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
0 siblings, 1 reply; 23+ messages in thread
From: Maarten Lankhorst @ 2017-09-25 6:43 UTC (permalink / raw)
To: Dmitry Osipenko, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Gustavo Padovan, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
Op 24-09-17 om 16:33 schreef Dmitry Osipenko:
> On 04.09.2017 13:48, Maarten Lankhorst wrote:
>> By always keeping track of the last commit in plane_state, we know
>> whether there is an active update on the plane or not. With that
>> information we can reject the fast update, and force the slowpath
>> to be used as was originally intended.
>>
>> We cannot use plane_state->crtc->state here, because this only mentions
>> the most recent commit for the crtc, but not the planes that were part
>> of it. We specifically care about what the last commit involving this
>> plane is, which can only be tracked with a pointer in the plane state.
>>
>> Changes since v1:
>> - Clean up the whole function here, instead of partially earlier.
>> - Add mention in the commit message why we need commit in plane_state.
>> - Swap plane->state in intel_legacy_cursor_update, instead of
>> reassigning all variables. With this commit We know that the cursor
>> is not part of any active commits so this hack can be removed.
>>
>> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++--------------------------
>> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
>> include/drm/drm_plane.h | 5 ++--
>> 3 files changed, 35 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index c81d46927a74..a2cd432d8b2d 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>> {
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *crtc_state;
>> - struct drm_crtc_commit *commit;
>> - struct drm_plane *__plane, *plane = NULL;
>> - struct drm_plane_state *__plane_state, *plane_state = NULL;
>> + struct drm_plane *plane;
>> + struct drm_plane_state *old_plane_state, *new_plane_state;
>> const struct drm_plane_helper_funcs *funcs;
>> - int i, j, n_planes = 0;
>> + int i, n_planes = 0;
>>
>> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> if (drm_atomic_crtc_needs_modeset(crtc_state))
>> return -EINVAL;
>> }
>>
>> - for_each_new_plane_in_state(state, __plane, __plane_state, i) {
>> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>> n_planes++;
>> - plane = __plane;
>> - plane_state = __plane_state;
>> - }
>>
>> /* FIXME: we support only single plane updates for now */
>> - if (!plane || n_planes != 1)
>> + if (n_planes != 1)
>> return -EINVAL;
>>
>> - if (!plane_state->crtc)
>> + if (!new_plane_state->crtc)
>> return -EINVAL;
>>
> Hello,
>
> It looks like a NULL-checking of new_plane_state is missed here.
>
> [ 70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
> (drm_atomic_helper_check+0x4c/0x64)
> [ 70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
> (drm_atomic_check_only+0x2f4/0x56c)
> [ 70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
> (drm_atomic_commit+0x10/0x50)
> [ 70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
> (drm_atomic_helper_update_plane+0xf0/0x100)
> [ 70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
> (__setplane_internal+0x178/0x214)
> [ 70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
> (drm_mode_cursor_universal+0x118/0x1a8)
> [ 70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
> (drm_mode_cursor_common+0x174/0x1ec)
> [ 70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
> (drm_mode_cursor_ioctl+0x58/0x60)
> [ 70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
> (drm_ioctl+0x198/0x368)
> [ 70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
> [ 70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
> [ 70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
> (ret_fast_syscall+0x0/0x48)
>
> It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
> currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
> Tegra cursor patches sometime soon.
Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally..
----->8-----
for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely
on it being set to the last valid plane. This was causing a NULL pointer deref when someone
tries to use the code. Save the plane and use the accessor functions to pull out the relevant
plane state.
Cc: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 32fb61169b4f..8573feaea8c0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
{
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
- struct drm_plane *plane;
+ struct drm_plane *plane = NULL, *__plane;
struct drm_plane_state *old_plane_state, *new_plane_state;
const struct drm_plane_helper_funcs *funcs;
- int i, n_planes = 0;
+ int i;
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
if (drm_atomic_crtc_needs_modeset(crtc_state))
return -EINVAL;
}
- for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
- n_planes++;
+ for_each_new_plane_in_state(state, __plane, new_plane_state, i) {
+ /* FIXME: we support only single plane updates for now */
+ if (plane)
+ return -EINVAL;
+
+ plane = __plane;
+ }
- /* FIXME: we support only single plane updates for now */
- if (n_planes != 1)
+ if (!plane)
return -EINVAL;
+ old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+ new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+
if (!new_plane_state->crtc)
return -EINVAL;
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/atomic: Make async plane update checks actually work as intended.
[not found] ` <a7d5fb04-c2f5-b743-5940-a1bd181d780d-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
@ 2017-09-25 14:08 ` Dmitry Osipenko
2017-09-26 4:59 ` Daniel Vetter
1 sibling, 0 replies; 23+ messages in thread
From: Dmitry Osipenko @ 2017-09-25 14:08 UTC (permalink / raw)
To: Maarten Lankhorst, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Gustavo Padovan, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
On 25.09.2017 09:43, Maarten Lankhorst wrote:
> Op 24-09-17 om 16:33 schreef Dmitry Osipenko:
>> On 04.09.2017 13:48, Maarten Lankhorst wrote:
>>> By always keeping track of the last commit in plane_state, we know
>>> whether there is an active update on the plane or not. With that
>>> information we can reject the fast update, and force the slowpath
>>> to be used as was originally intended.
>>>
>>> We cannot use plane_state->crtc->state here, because this only mentions
>>> the most recent commit for the crtc, but not the planes that were part
>>> of it. We specifically care about what the last commit involving this
>>> plane is, which can only be tracked with a pointer in the plane state.
>>>
>>> Changes since v1:
>>> - Clean up the whole function here, instead of partially earlier.
>>> - Add mention in the commit message why we need commit in plane_state.
>>> - Swap plane->state in intel_legacy_cursor_update, instead of
>>> reassigning all variables. With this commit We know that the cursor
>>> is not part of any active commits so this hack can be removed.
>>>
>>> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
>>> ---
>>> drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++--------------------------
>>> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
>>> include/drm/drm_plane.h | 5 ++--
>>> 3 files changed, 35 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c81d46927a74..a2cd432d8b2d 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>> {
>>> struct drm_crtc *crtc;
>>> struct drm_crtc_state *crtc_state;
>>> - struct drm_crtc_commit *commit;
>>> - struct drm_plane *__plane, *plane = NULL;
>>> - struct drm_plane_state *__plane_state, *plane_state = NULL;
>>> + struct drm_plane *plane;
>>> + struct drm_plane_state *old_plane_state, *new_plane_state;
>>> const struct drm_plane_helper_funcs *funcs;
>>> - int i, j, n_planes = 0;
>>> + int i, n_planes = 0;
>>>
>>> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> if (drm_atomic_crtc_needs_modeset(crtc_state))
>>> return -EINVAL;
>>> }
>>>
>>> - for_each_new_plane_in_state(state, __plane, __plane_state, i) {
>>> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>>> n_planes++;
>>> - plane = __plane;
>>> - plane_state = __plane_state;
>>> - }
>>>
>>> /* FIXME: we support only single plane updates for now */
>>> - if (!plane || n_planes != 1)
>>> + if (n_planes != 1)
>>> return -EINVAL;
>>>
>>> - if (!plane_state->crtc)
>>> + if (!new_plane_state->crtc)
>>> return -EINVAL;
>>>
>> Hello,
>>
>> It looks like a NULL-checking of new_plane_state is missed here.
>>
>> [ 70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
>> (drm_atomic_helper_check+0x4c/0x64)
>> [ 70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
>> (drm_atomic_check_only+0x2f4/0x56c)
>> [ 70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
>> (drm_atomic_commit+0x10/0x50)
>> [ 70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
>> (drm_atomic_helper_update_plane+0xf0/0x100)
>> [ 70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
>> (__setplane_internal+0x178/0x214)
>> [ 70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
>> (drm_mode_cursor_universal+0x118/0x1a8)
>> [ 70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
>> (drm_mode_cursor_common+0x174/0x1ec)
>> [ 70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
>> (drm_mode_cursor_ioctl+0x58/0x60)
>> [ 70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
>> (drm_ioctl+0x198/0x368)
>> [ 70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
>> [ 70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
>> [ 70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
>> (ret_fast_syscall+0x0/0x48)
>>
>> It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
>> currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
>> Tegra cursor patches sometime soon.
>
> Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally..
> ----->8-----
> for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely
> on it being set to the last valid plane. This was causing a NULL pointer deref when someone
> tries to use the code. Save the plane and use the accessor functions to pull out the relevant
> plane state.
>
> Cc: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
It works! Thank you.
Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 32fb61169b4f..8573feaea8c0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> - struct drm_plane *plane;
> + struct drm_plane *plane = NULL, *__plane;
> struct drm_plane_state *old_plane_state, *new_plane_state;
> const struct drm_plane_helper_funcs *funcs;
> - int i, n_planes = 0;
> + int i;
>
> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> if (drm_atomic_crtc_needs_modeset(crtc_state))
> return -EINVAL;
> }
>
> - for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> - n_planes++;
> + for_each_new_plane_in_state(state, __plane, new_plane_state, i) {
> + /* FIXME: we support only single plane updates for now */
> + if (plane)
> + return -EINVAL;
> +
> + plane = __plane;
> + }
>
> - /* FIXME: we support only single plane updates for now */
> - if (n_planes != 1)
> + if (!plane)
> return -EINVAL;
>
> + old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> + new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
> if (!new_plane_state->crtc)
> return -EINVAL;
>
>
--
Dmitry
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/atomic: Make async plane update checks actually work as intended.
[not found] ` <a7d5fb04-c2f5-b743-5940-a1bd181d780d-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-09-25 14:08 ` Dmitry Osipenko
@ 2017-09-26 4:59 ` Daniel Vetter
1 sibling, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2017-09-26 4:59 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Dmitry Osipenko, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Gustavo Padovan,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Mon, Sep 25, 2017 at 08:43:44AM +0200, Maarten Lankhorst wrote:
> Op 24-09-17 om 16:33 schreef Dmitry Osipenko:
> > On 04.09.2017 13:48, Maarten Lankhorst wrote:
> >> By always keeping track of the last commit in plane_state, we know
> >> whether there is an active update on the plane or not. With that
> >> information we can reject the fast update, and force the slowpath
> >> to be used as was originally intended.
> >>
> >> We cannot use plane_state->crtc->state here, because this only mentions
> >> the most recent commit for the crtc, but not the planes that were part
> >> of it. We specifically care about what the last commit involving this
> >> plane is, which can only be tracked with a pointer in the plane state.
> >>
> >> Changes since v1:
> >> - Clean up the whole function here, instead of partially earlier.
> >> - Add mention in the commit message why we need commit in plane_state.
> >> - Swap plane->state in intel_legacy_cursor_update, instead of
> >> reassigning all variables. With this commit We know that the cursor
> >> is not part of any active commits so this hack can be removed.
> >>
> >> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
> >> ---
> >> drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++--------------------------
> >> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
> >> include/drm/drm_plane.h | 5 ++--
> >> 3 files changed, 35 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index c81d46927a74..a2cd432d8b2d 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> >> {
> >> struct drm_crtc *crtc;
> >> struct drm_crtc_state *crtc_state;
> >> - struct drm_crtc_commit *commit;
> >> - struct drm_plane *__plane, *plane = NULL;
> >> - struct drm_plane_state *__plane_state, *plane_state = NULL;
> >> + struct drm_plane *plane;
> >> + struct drm_plane_state *old_plane_state, *new_plane_state;
> >> const struct drm_plane_helper_funcs *funcs;
> >> - int i, j, n_planes = 0;
> >> + int i, n_planes = 0;
> >>
> >> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >> if (drm_atomic_crtc_needs_modeset(crtc_state))
> >> return -EINVAL;
> >> }
> >>
> >> - for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> >> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> >> n_planes++;
> >> - plane = __plane;
> >> - plane_state = __plane_state;
> >> - }
> >>
> >> /* FIXME: we support only single plane updates for now */
> >> - if (!plane || n_planes != 1)
> >> + if (n_planes != 1)
> >> return -EINVAL;
> >>
> >> - if (!plane_state->crtc)
> >> + if (!new_plane_state->crtc)
> >> return -EINVAL;
> >>
> > Hello,
> >
> > It looks like a NULL-checking of new_plane_state is missed here.
> >
> > [ 70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
> > (drm_atomic_helper_check+0x4c/0x64)
> > [ 70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
> > (drm_atomic_check_only+0x2f4/0x56c)
> > [ 70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
> > (drm_atomic_commit+0x10/0x50)
> > [ 70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
> > (drm_atomic_helper_update_plane+0xf0/0x100)
> > [ 70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
> > (__setplane_internal+0x178/0x214)
> > [ 70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
> > (drm_mode_cursor_universal+0x118/0x1a8)
> > [ 70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
> > (drm_mode_cursor_common+0x174/0x1ec)
> > [ 70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
> > (drm_mode_cursor_ioctl+0x58/0x60)
> > [ 70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
> > (drm_ioctl+0x198/0x368)
> > [ 70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
> > [ 70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
> > [ 70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
> > (ret_fast_syscall+0x0/0x48)
> >
> > It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
> > currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
> > Tegra cursor patches sometime soon.
Please don't in-line reply fixup patches, CI doesn't pick them up but
instead wants to add it to the patch series. Which won't apply because
most of it landed already. Please resend standalone.
> Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally..
> ----->8-----
> for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely
> on it being set to the last valid plane. This was causing a NULL pointer deref when someone
> tries to use the code. Save the plane and use the accessor functions to pull out the relevant
> plane state.
Hm, that's rather tricky :-/ I guess the for_each_if() is the culprit for
this? Could we perhaps fix this somehow by pushing the assignments into
the for_each_if (again using the && trick)? That way these macros are more
robust.
> Cc: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Assuming CI also approves:
Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 32fb61169b4f..8573feaea8c0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> - struct drm_plane *plane;
> + struct drm_plane *plane = NULL, *__plane;
> struct drm_plane_state *old_plane_state, *new_plane_state;
> const struct drm_plane_helper_funcs *funcs;
> - int i, n_planes = 0;
> + int i;
>
> for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> if (drm_atomic_crtc_needs_modeset(crtc_state))
> return -EINVAL;
> }
>
> - for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> - n_planes++;
> + for_each_new_plane_in_state(state, __plane, new_plane_state, i) {
> + /* FIXME: we support only single plane updates for now */
> + if (plane)
> + return -EINVAL;
> +
> + plane = __plane;
> + }
>
> - /* FIXME: we support only single plane updates for now */
> - if (n_planes != 1)
> + if (!plane)
> return -EINVAL;
>
> + old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> + new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
> if (!new_plane_state->crtc)
> return -EINVAL;
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2017-09-26 4:59 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-04 10:48 [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2 Maarten Lankhorst
2017-09-07 9:49 ` Daniel Vetter
2017-09-08 9:08 ` Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3 Maarten Lankhorst
2017-09-04 15:04 ` [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4 Maarten Lankhorst
2017-09-04 18:01 ` kbuild test robot
2017-09-05 6:25 ` Daniel Vetter
2017-09-04 10:48 ` [PATCH v2 3/6] drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2 Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation Maarten Lankhorst
2017-09-07 9:59 ` Daniel Vetter
2017-09-04 10:48 ` [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3 Maarten Lankhorst
2017-09-07 10:05 ` Daniel Vetter
2017-09-07 11:08 ` Maarten Lankhorst
2017-09-04 10:48 ` [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2 Maarten Lankhorst
[not found] ` <20170904104838.23822-7-maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-09-24 14:33 ` Dmitry Osipenko
[not found] ` <54467116-df02-e6ad-ac14-90aa79e164e4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-09-25 6:43 ` [PATCH] drm/atomic: Make async plane update checks actually work as intended Maarten Lankhorst
[not found] ` <a7d5fb04-c2f5-b743-5940-a1bd181d780d-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2017-09-25 14:08 ` Dmitry Osipenko
2017-09-26 4:59 ` Daniel Vetter
2017-09-04 11:11 ` ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2) Patchwork
2017-09-04 12:45 ` ✗ Fi.CI.IGT: failure " Patchwork
2017-09-04 15:23 ` ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3) Patchwork
2017-09-04 16:21 ` ✗ Fi.CI.IGT: failure " Patchwork
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.