On Wednesday, 30 August 2017 17:17:36 EEST Daniel Vetter wrote:
On Wed, Aug 30, 2017 at 05:10:43PM +0300, Laurent Pinchart wrote:
Hi Maarten,
Thank you for the patch.
On Wednesday, 30 August 2017 15:17:51 EEST 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.
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 | 156 ++++++++++++++++++++++++++++++-- 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, 182 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 8ccb8b6536c0..034f563fb130 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1644,6 +1644,40 @@ 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) +{
You could allocate the commit in this function too, the kzalloc() is currently duplicated. The function should probably be called alloc_commit() then.>
- 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 * +fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) +{
- struct drm_crtc_commit *commit;
- if (crtc) {
struct drm_crtc_state *new_crtc_state;new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);commit = new_crtc_state->commit;- } else if (!state->fake_commit) {
state->fake_commit = commit = kzalloc(sizeof(*commit),
GFP_KERNEL);
if (!commit)return NULL;init_commit(commit, NULL);- } else
commit = state->fake_commit;- drm_crtc_commit_get(commit);
I believe the reference counting is right. The double reference in the second case (kref_init() when initializing the commit and drm_crtc_commit_get()) should not cause a leak. The kref_init() takes a reference to store the commit in state->fake_commit, released in drm_atomic_state_default_clear(), and the drm_crtc_commit_get() takes a reference returned by the function, stored in new_*_state->commit by the caller.
This being said, I think the reference counting is confusing, as proved by Daniel thinking there was a leak here (or by me thinking there's no leak while there's one :-)). To make the implementation clearer, I propose turning the definition of drm_crtc_commit_get() to
static inline struct drm_crtc_commit * drm_crtc_commit_get(struct drm_crtc_commit *commit) {
kref_get(&commit->ref); return commit;
}
and writing this function as
/* Return a new reference to the commit object */ static struct drm_crtc_commit * fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) {
struct drm_crtc_commit *commit;
if (crtc) {
struct drm_crtc_state *new_crtc_state;new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); commit = new_crtc_state->commit;
} else {
if (!state->fake_commit)state->fake_commit = alloc_commit(NULL); commit = state->fake_commit;
}
return drm_crtc_commit_get(commit);
}
+1 on something like this, the compressed layout with assigning both commit and ->fake_commit is what tricked me into seeing a leak.
What I think makes it clearer is to store the return value of functions returning a reference directly in the pointer that stores the reference. That's why I prefer
1. if (!state->fake_commit) 2. state->fake_commit = alloc_commit(NULL); 3. 4. commit = state->fake_commit;
Line 2 stores the pointer in an object that thus requires a reference, which is returned by alloc_commit(). Line 4 stores the pointer in a local variable, and thus doesn't require a reference.