From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH 1/2] drm/atomic: fix self-refresh helpers crtc state dereference Date: Mon, 4 Nov 2019 10:29:21 +0100 Message-ID: <3a67eed5-8be7-df5b-84fa-61b133e1fea2@linux.intel.com> References: <20191101180713.5470-1-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Sean Paul , Rob Clark Cc: dri-devel , Sean Paul , Rob Clark , Maxime Ripard , David Airlie , Daniel Vetter , open list List-Id: dri-devel@lists.freedesktop.org Op 01-11-2019 om 21:06 schreef Sean Paul: > On Fri, Nov 1, 2019 at 2:09 PM Rob Clark wrote: >> From: Rob Clark >> >> drm_self_refresh_helper_update_avg_times() was incorrectly accessing the >> new incoming state after drm_atomic_helper_commit_hw_done(). But this >> state might have already been superceeded by an !nonblock atomic update >> resulting in dereferencing an already free'd crtc_state. >> >> Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing") >> Cc: Sean Paul >> Signed-off-by: Rob Clark >> --- >> TODO I *think* this will more or less do the right thing.. althought I'm >> not 100% sure if, for example, we enter psr in a nonblock commit, and >> then leave psr in a !nonblock commit that overtakes the completion of >> the nonblock commit. Not sure if this sort of scenario can happen in >> practice. But not crashing is better than crashing, so I guess we >> should either take this patch or rever the self-refresh helpers until >> Sean can figure out a better solution. >> >> drivers/gpu/drm/drm_atomic_helper.c | 2 ++ >> drivers/gpu/drm/drm_self_refresh_helper.c | 11 ++++++----- >> include/drm/drm_atomic.h | 8 ++++++++ >> 3 files changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 3ef2ac52ce94..732bd0ce9241 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -2240,6 +2240,8 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) >> int i; >> >> for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { >> + old_state->crtcs[i].new_self_refresh_active = new_crtc_state->self_refresh_active; >> + >> commit = new_crtc_state->commit; >> if (!commit) >> continue; >> diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c >> index 68f4765a5896..77b9079fa578 100644 >> --- a/drivers/gpu/drm/drm_self_refresh_helper.c >> +++ b/drivers/gpu/drm/drm_self_refresh_helper.c >> @@ -143,19 +143,20 @@ void drm_self_refresh_helper_update_avg_times(struct drm_atomic_state *state, >> unsigned int commit_time_ms) >> { >> struct drm_crtc *crtc; >> - struct drm_crtc_state *old_crtc_state, *new_crtc_state; >> + struct drm_crtc_state *old_crtc_state; >> int i; >> >> - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, >> - new_crtc_state, i) { >> + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { >> + bool new_self_refresh_active = >> + state->crtcs[i].new_self_refresh_active; >> struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; >> struct ewma_psr_time *time; >> >> if (old_crtc_state->self_refresh_active == >> - new_crtc_state->self_refresh_active) >> + new_self_refresh_active) >> continue; >> >> - if (new_crtc_state->self_refresh_active) >> + if (new_self_refresh_active) >> time = &sr_data->entry_avg_ms; >> else >> time = &sr_data->exit_avg_ms; >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >> index 927e1205d7aa..86baf2b38bb3 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -155,6 +155,14 @@ struct __drm_crtcs_state { >> struct drm_crtc *ptr; >> struct drm_crtc_state *state, *old_state, *new_state; >> >> + /** >> + * @new_self_refresh_active: >> + * >> + * drm_atomic_helper_commit_hw_done() stashes new_crtc_state->self_refresh_active >> + * so that it can be accessed late in drm_self_refresh_helper_update_avg_times(). >> + */ >> + bool new_self_refresh_active; >> + > Instead of stashing this in crtc, we could generate a bitmask local to > commit_tail and pass that to calc_avg_times? That way we don't have to > worry about someone using this when they shouldn't Yeah would make sense to have a bitmask, instead of making the property special. :) Current solution seems a bit ugly. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7B7A2CA9EA0 for ; Mon, 4 Nov 2019 11:37:59 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 51DE22184C for ; Mon, 4 Nov 2019 11:37:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 51DE22184C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AF1096E20A; Mon, 4 Nov 2019 11:37:58 +0000 (UTC) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2A91B6E20A for ; Mon, 4 Nov 2019 11:37:57 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Nov 2019 01:29:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,266,1569308400"; d="scan'208";a="401550886" Received: from savicrad-mobl.ger.corp.intel.com (HELO [10.249.40.217]) ([10.249.40.217]) by fmsmga005.fm.intel.com with ESMTP; 04 Nov 2019 01:29:36 -0800 Subject: Re: [PATCH 1/2] drm/atomic: fix self-refresh helpers crtc state dereference To: Sean Paul , Rob Clark References: <20191101180713.5470-1-robdclark@gmail.com> From: Maarten Lankhorst Message-ID: <3a67eed5-8be7-df5b-84fa-61b133e1fea2@linux.intel.com> Date: Mon, 4 Nov 2019 10:29:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Clark , David Airlie , open list , Sean Paul , dri-devel Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Message-ID: <20191104092921.EhAzQzT_vYjabonk9EvGQie4fQR3wwSCCW8Cq45BgS0@z> T3AgMDEtMTEtMjAxOSBvbSAyMTowNiBzY2hyZWVmIFNlYW4gUGF1bDoKPiBPbiBGcmksIE5vdiAx LCAyMDE5IGF0IDI6MDkgUE0gUm9iIENsYXJrIDxyb2JkY2xhcmtAZ21haWwuY29tPiB3cm90ZToK Pj4gRnJvbTogUm9iIENsYXJrIDxyb2JkY2xhcmtAY2hyb21pdW0ub3JnPgo+Pgo+PiBkcm1fc2Vs Zl9yZWZyZXNoX2hlbHBlcl91cGRhdGVfYXZnX3RpbWVzKCkgd2FzIGluY29ycmVjdGx5IGFjY2Vz c2luZyB0aGUKPj4gbmV3IGluY29taW5nIHN0YXRlIGFmdGVyIGRybV9hdG9taWNfaGVscGVyX2Nv bW1pdF9od19kb25lKCkuICBCdXQgdGhpcwo+PiBzdGF0ZSBtaWdodCBoYXZlIGFscmVhZHkgYmVl biBzdXBlcmNlZWRlZCBieSBhbiAhbm9uYmxvY2sgYXRvbWljIHVwZGF0ZQo+PiByZXN1bHRpbmcg aW4gZGVyZWZlcmVuY2luZyBhbiBhbHJlYWR5IGZyZWUnZCBjcnRjX3N0YXRlLgo+Pgo+PiBGaXhl czogZDRkYTRlMzMzNDFjICgiZHJtOiBNZWFzdXJlIFNlbGYgUmVmcmVzaCBFbnRyeS9FeGl0IHRp bWVzIHRvIGF2b2lkIHRocmFzaGluZyIpCj4+IENjOiBTZWFuIFBhdWwgPHNlYW5wYXVsQGNocm9t aXVtLm9yZz4KPj4gU2lnbmVkLW9mZi1ieTogUm9iIENsYXJrIDxyb2JkY2xhcmtAY2hyb21pdW0u b3JnPgo+PiAtLS0KPj4gVE9ETyBJICp0aGluayogdGhpcyB3aWxsIG1vcmUgb3IgbGVzcyBkbyB0 aGUgcmlnaHQgdGhpbmcuLiBhbHRob3VnaHQgSSdtCj4+IG5vdCAxMDAlIHN1cmUgaWYsIGZvciBl eGFtcGxlLCB3ZSBlbnRlciBwc3IgaW4gYSBub25ibG9jayBjb21taXQsIGFuZAo+PiB0aGVuIGxl YXZlIHBzciBpbiBhICFub25ibG9jayBjb21taXQgdGhhdCBvdmVydGFrZXMgdGhlIGNvbXBsZXRp b24gb2YKPj4gdGhlIG5vbmJsb2NrIGNvbW1pdC4gIE5vdCBzdXJlIGlmIHRoaXMgc29ydCBvZiBz Y2VuYXJpbyBjYW4gaGFwcGVuIGluCj4+IHByYWN0aWNlLiAgQnV0IG5vdCBjcmFzaGluZyBpcyBi ZXR0ZXIgdGhhbiBjcmFzaGluZywgc28gSSBndWVzcyB3ZQo+PiBzaG91bGQgZWl0aGVyIHRha2Ug dGhpcyBwYXRjaCBvciByZXZlciB0aGUgc2VsZi1yZWZyZXNoIGhlbHBlcnMgdW50aWwKPj4gU2Vh biBjYW4gZmlndXJlIG91dCBhIGJldHRlciBzb2x1dGlvbi4KPj4KPj4gIGRyaXZlcnMvZ3B1L2Ry bS9kcm1fYXRvbWljX2hlbHBlci5jICAgICAgIHwgIDIgKysKPj4gIGRyaXZlcnMvZ3B1L2RybS9k cm1fc2VsZl9yZWZyZXNoX2hlbHBlci5jIHwgMTEgKysrKysrLS0tLS0KPj4gIGluY2x1ZGUvZHJt L2RybV9hdG9taWMuaCAgICAgICAgICAgICAgICAgIHwgIDggKysrKysrKysKPj4gIDMgZmlsZXMg Y2hhbmdlZCwgMTYgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKPj4KPj4gZGlmZiAtLWdp dCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fYXRvbWljX2hlbHBlci5jIGIvZHJpdmVycy9ncHUvZHJt L2RybV9hdG9taWNfaGVscGVyLmMKPj4gaW5kZXggM2VmMmFjNTJjZTk0Li43MzJiZDBjZTkyNDEg MTAwNjQ0Cj4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fYXRvbWljX2hlbHBlci5jCj4+ICsr KyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fYXRvbWljX2hlbHBlci5jCj4+IEBAIC0yMjQwLDYgKzIy NDAsOCBAQCB2b2lkIGRybV9hdG9taWNfaGVscGVyX2NvbW1pdF9od19kb25lKHN0cnVjdCBkcm1f YXRvbWljX3N0YXRlICpvbGRfc3RhdGUpCj4+ICAgICAgICAgaW50IGk7Cj4+Cj4+ICAgICAgICAg Zm9yX2VhY2hfb2xkbmV3X2NydGNfaW5fc3RhdGUob2xkX3N0YXRlLCBjcnRjLCBvbGRfY3J0Y19z dGF0ZSwgbmV3X2NydGNfc3RhdGUsIGkpIHsKPj4gKyAgICAgICAgICAgICAgIG9sZF9zdGF0ZS0+ Y3J0Y3NbaV0ubmV3X3NlbGZfcmVmcmVzaF9hY3RpdmUgPSBuZXdfY3J0Y19zdGF0ZS0+c2VsZl9y ZWZyZXNoX2FjdGl2ZTsKPj4gKwo+PiAgICAgICAgICAgICAgICAgY29tbWl0ID0gbmV3X2NydGNf c3RhdGUtPmNvbW1pdDsKPj4gICAgICAgICAgICAgICAgIGlmICghY29tbWl0KQo+PiAgICAgICAg ICAgICAgICAgICAgICAgICBjb250aW51ZTsKPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2Ry bS9kcm1fc2VsZl9yZWZyZXNoX2hlbHBlci5jIGIvZHJpdmVycy9ncHUvZHJtL2RybV9zZWxmX3Jl ZnJlc2hfaGVscGVyLmMKPj4gaW5kZXggNjhmNDc2NWE1ODk2Li43N2I5MDc5ZmE1NzggMTAwNjQ0 Cj4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fc2VsZl9yZWZyZXNoX2hlbHBlci5jCj4+ICsr KyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fc2VsZl9yZWZyZXNoX2hlbHBlci5jCj4+IEBAIC0xNDMs MTkgKzE0MywyMCBAQCB2b2lkIGRybV9zZWxmX3JlZnJlc2hfaGVscGVyX3VwZGF0ZV9hdmdfdGlt ZXMoc3RydWN0IGRybV9hdG9taWNfc3RhdGUgKnN0YXRlLAo+PiAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgdW5zaWduZWQgaW50IGNvbW1pdF90aW1lX21zKQo+ PiAgewo+PiAgICAgICAgIHN0cnVjdCBkcm1fY3J0YyAqY3J0YzsKPj4gLSAgICAgICBzdHJ1Y3Qg ZHJtX2NydGNfc3RhdGUgKm9sZF9jcnRjX3N0YXRlLCAqbmV3X2NydGNfc3RhdGU7Cj4+ICsgICAg ICAgc3RydWN0IGRybV9jcnRjX3N0YXRlICpvbGRfY3J0Y19zdGF0ZTsKPj4gICAgICAgICBpbnQg aTsKPj4KPj4gLSAgICAgICBmb3JfZWFjaF9vbGRuZXdfY3J0Y19pbl9zdGF0ZShzdGF0ZSwgY3J0 Yywgb2xkX2NydGNfc3RhdGUsCj4+IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgbmV3X2NydGNfc3RhdGUsIGkpIHsKPj4gKyAgICAgICBmb3JfZWFjaF9vbGRfY3J0Y19pbl9z dGF0ZShzdGF0ZSwgY3J0Yywgb2xkX2NydGNfc3RhdGUsIGkpIHsKPj4gKyAgICAgICAgICAgICAg IGJvb2wgbmV3X3NlbGZfcmVmcmVzaF9hY3RpdmUgPQo+PiArICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgIHN0YXRlLT5jcnRjc1tpXS5uZXdfc2VsZl9yZWZyZXNoX2FjdGl2ZTsKPj4gICAg ICAgICAgICAgICAgIHN0cnVjdCBkcm1fc2VsZl9yZWZyZXNoX2RhdGEgKnNyX2RhdGEgPSBjcnRj LT5zZWxmX3JlZnJlc2hfZGF0YTsKPj4gICAgICAgICAgICAgICAgIHN0cnVjdCBld21hX3Bzcl90 aW1lICp0aW1lOwo+Pgo+PiAgICAgICAgICAgICAgICAgaWYgKG9sZF9jcnRjX3N0YXRlLT5zZWxm X3JlZnJlc2hfYWN0aXZlID09Cj4+IC0gICAgICAgICAgICAgICAgICAgbmV3X2NydGNfc3RhdGUt PnNlbGZfcmVmcmVzaF9hY3RpdmUpCj4+ICsgICAgICAgICAgICAgICAgICAgbmV3X3NlbGZfcmVm cmVzaF9hY3RpdmUpCj4+ICAgICAgICAgICAgICAgICAgICAgICAgIGNvbnRpbnVlOwo+Pgo+PiAt ICAgICAgICAgICAgICAgaWYgKG5ld19jcnRjX3N0YXRlLT5zZWxmX3JlZnJlc2hfYWN0aXZlKQo+ PiArICAgICAgICAgICAgICAgaWYgKG5ld19zZWxmX3JlZnJlc2hfYWN0aXZlKQo+PiAgICAgICAg ICAgICAgICAgICAgICAgICB0aW1lID0gJnNyX2RhdGEtPmVudHJ5X2F2Z19tczsKPj4gICAgICAg ICAgICAgICAgIGVsc2UKPj4gICAgICAgICAgICAgICAgICAgICAgICAgdGltZSA9ICZzcl9kYXRh LT5leGl0X2F2Z19tczsKPj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvZHJtL2RybV9hdG9taWMuaCBi L2luY2x1ZGUvZHJtL2RybV9hdG9taWMuaAo+PiBpbmRleCA5MjdlMTIwNWQ3YWEuLjg2YmFmMmIz OGJiMyAxMDA2NDQKPj4gLS0tIGEvaW5jbHVkZS9kcm0vZHJtX2F0b21pYy5oCj4+ICsrKyBiL2lu Y2x1ZGUvZHJtL2RybV9hdG9taWMuaAo+PiBAQCAtMTU1LDYgKzE1NSwxNCBAQCBzdHJ1Y3QgX19k cm1fY3J0Y3Nfc3RhdGUgewo+PiAgICAgICAgIHN0cnVjdCBkcm1fY3J0YyAqcHRyOwo+PiAgICAg ICAgIHN0cnVjdCBkcm1fY3J0Y19zdGF0ZSAqc3RhdGUsICpvbGRfc3RhdGUsICpuZXdfc3RhdGU7 Cj4+Cj4+ICsgICAgICAgLyoqCj4+ICsgICAgICAgICogQG5ld19zZWxmX3JlZnJlc2hfYWN0aXZl Ogo+PiArICAgICAgICAqCj4+ICsgICAgICAgICogZHJtX2F0b21pY19oZWxwZXJfY29tbWl0X2h3 X2RvbmUoKSBzdGFzaGVzIG5ld19jcnRjX3N0YXRlLT5zZWxmX3JlZnJlc2hfYWN0aXZlCj4+ICsg ICAgICAgICogc28gdGhhdCBpdCBjYW4gYmUgYWNjZXNzZWQgbGF0ZSBpbiBkcm1fc2VsZl9yZWZy ZXNoX2hlbHBlcl91cGRhdGVfYXZnX3RpbWVzKCkuCj4+ICsgICAgICAgICovCj4+ICsgICAgICAg Ym9vbCBuZXdfc2VsZl9yZWZyZXNoX2FjdGl2ZTsKPj4gKwo+IEluc3RlYWQgb2Ygc3Rhc2hpbmcg dGhpcyBpbiBjcnRjLCB3ZSBjb3VsZCBnZW5lcmF0ZSBhIGJpdG1hc2sgbG9jYWwgdG8KPiBjb21t aXRfdGFpbCBhbmQgcGFzcyB0aGF0IHRvIGNhbGNfYXZnX3RpbWVzPyBUaGF0IHdheSB3ZSBkb24n dCBoYXZlIHRvCj4gd29ycnkgYWJvdXQgc29tZW9uZSB1c2luZyB0aGlzIHdoZW4gdGhleSBzaG91 bGRuJ3QKClllYWggd291bGQgbWFrZSBzZW5zZSB0byBoYXZlIGEgYml0bWFzaywgaW5zdGVhZCBv ZiBtYWtpbmcgdGhlIHByb3BlcnR5IHNwZWNpYWwuIDopCgpDdXJyZW50IHNvbHV0aW9uIHNlZW1z IGEgYml0IHVnbHkuCgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Au b3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRl dmVs