From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Lankhorst, Maarten" Subject: Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values Date: Tue, 30 May 2017 14:35:06 +0000 Message-ID: <1496154904.3120.1.camel@intel.com> References: <20170526151546.25025-1-mahesh1.kumar@intel.com> <20170526151546.25025-2-mahesh1.kumar@intel.com> <5217182a-d8e6-c5f9-370c-85295ef68231@linux.intel.com> <059c2f57-6899-ffc0-7564-ff56b6cbdb17@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0620532854==" Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7518A6E006 for ; Tue, 30 May 2017 14:35:09 +0000 (UTC) In-Reply-To: <059c2f57-6899-ffc0-7564-ff56b6cbdb17@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "intel-gfx@lists.freedesktop.org" , "Kumar, Mahesh1" , "maarten.lankhorst@linux.intel.com" Cc: "Zanoni, Paulo R" List-Id: intel-gfx@lists.freedesktop.org --===============0620532854== Content-Language: en-US Content-Type: multipart/signed; micalg=sha-1; protocol="application/x-pkcs7-signature"; boundary="=-3pg3uQPHeNjS8YxuatwX" --=-3pg3uQPHeNjS8YxuatwX Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Mahesh Kumar schreef op di 30-05-2017 om 18:26 [+0530]: > Hi Maarten, >=20 > Thanks for review :) >=20 >=20 > On Tuesday 30 May 2017 03:30 PM, Maarten Lankhorst wrote: > > Op 26-05-17 om 17:15 schreef Mahesh Kumar: > > > Don't trust cached DDB values. Recalculate the ddb value if > > > cached value > > > is zero. > > >=20 > > > If i915 is build as a module, there may be a race condition when > > > cursor_disable call comes even before intel_fbdev_initial_config. > > > Which may lead to cached value being 0. And further commit will > > > fail > > > until a modeset. > > >=20 > > > Signed-off-by: Mahesh Kumar > > > --- > > > =C2=A0 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > =C2=A0 1 file changed, 10 insertions(+), 5 deletions(-) > > >=20 > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index 936eef1634c7..b67be1355e39 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct > > > drm_device *dev, > > > =C2=A0=C2=A0 struct drm_i915_private *dev_priv =3D to_i915(dev); > > > =C2=A0=C2=A0 struct drm_crtc *for_crtc =3D cstate->base.crtc; > > > =C2=A0=C2=A0 unsigned int pipe_size, ddb_size; > > > + unsigned int active_crtcs; > > > =C2=A0=C2=A0 int nth_active_pipe; > > > =C2=A0=C2=A0 > > > =C2=A0=C2=A0 if (WARN_ON(!state) || !cstate->base.active) { > > > @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct > > > drm_device *dev, > > > =C2=A0=C2=A0 } > > > =C2=A0=C2=A0 > > > =C2=A0=C2=A0 if (intel_state->active_pipe_changes) > > > - *num_active =3D hweight32(intel_state- > > > >active_crtcs); > > > + active_crtcs =3D intel_state->active_crtcs; > > > =C2=A0=C2=A0 else > > > - *num_active =3D hweight32(dev_priv->active_crtcs); > > > + active_crtcs =3D dev_priv->active_crtcs; > > > =C2=A0=C2=A0 > > > + *num_active =3D hweight32(active_crtcs); > > > =C2=A0=C2=A0 ddb_size =3D INTEL_INFO(dev_priv)->ddb_size; > > > =C2=A0=C2=A0 WARN_ON(ddb_size =3D=3D 0); > > > =C2=A0=C2=A0 > > > @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct > > > drm_device *dev, > > > =C2=A0=C2=A0 =C2=A0* copy from old state to be sure > > > =C2=A0=C2=A0 =C2=A0*/ > > > =C2=A0=C2=A0 *alloc =3D to_intel_crtc_state(for_crtc->state)- > > > >wm.skl.ddb; > > > - return; > > > + if (!skl_ddb_entry_size(alloc)) > > > + DRM_DEBUG_KMS("Cached pipe DDB is 0 > > > recalculate\n"); > > > + else > > > + return; > > > =C2=A0=C2=A0 } > > > =C2=A0=C2=A0 > > > - nth_active_pipe =3D hweight32(intel_state->active_crtcs & > > > + nth_active_pipe =3D hweight32(active_crtcs & > > > =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0(drm_crtc_mask(for_crtc) - > > > 1)); > > > - pipe_size =3D ddb_size / hweight32(intel_state- > > > >active_crtcs); > > > + pipe_size =3D ddb_size / hweight32(active_crtcs); > > > =C2=A0=C2=A0 alloc->start =3D nth_active_pipe * ddb_size / > > > *num_active; > > > =C2=A0=C2=A0 alloc->end =3D alloc->start + pipe_size; > > > =C2=A0 } > >=20 > > I'd love it if you also add a warning somewhere for active pipe > > with no ddb allocation to prevent this from reoccuring in the > > future. :) > > But with the root cause being a invalid ddb allocation at boot, > > won't the below one liner should fix it too, but better? >=20 > Above DEBUG message is actually warning. That will hit only if pipe > is=C2=A0 > active & DDB allocated for that pipe is "0" (we do return from caller > if=C2=A0 > !cstate->active). > I can reword the msg to be WARN & make it something like "Pipe is > active=C2=A0 > with No DDB allocated, recompute ddb now". This will fix the issue > as=C2=A0 > well as add warning msg. >=20 > >=20 > > I'l ltest it when f3-kbl becomes available again. > >=20 > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index a488e068c3d6..63a47909f618 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state > > *state) > > =C2=A0=C2=A0 =C2=A0*/ > > =C2=A0=C2=A0 for_each_new_crtc_in_state(state, crtc, cstate, i) > > =C2=A0=C2=A0 changed =3D true; > > - if (!changed) > > + if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm) > > =C2=A0=C2=A0 return 0; > > =C2=A0=C2=A0 >=20 > This change should also solve the issue during boot. Yeah I did some testing and this fixes it. Looks like a bug in the original implementation of skl_compute_wm that the KBL just happened to hit. So I guess if others are hitting it too, then this needs Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic check time (v4)") Cc: # v4.8+ --=-3pg3uQPHeNjS8YxuatwX Content-Type: application/x-pkcs7-signature; name="smime.p7s" Content-Disposition: attachment; filename="smime.p7s" Content-Transfer-Encoding: base64 MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIIKfTCCBOsw ggPToAMCAQICEDabxALowUBS+21KC0JI8fcwDQYJKoZIhvcNAQEFBQAwbzELMAkGA1UEBhMCU0Ux FDASBgNVBAoTC0FkZFRydXN0IEFCMSYwJAYDVQQLEx1BZGRUcnVzdCBFeHRlcm5hbCBUVFAgTmV0 d29yazEiMCAGA1UEAxMZQWRkVHJ1c3QgRXh0ZXJuYWwgQ0EgUm9vdDAeFw0xMzEyMTEwMDAwMDBa Fw0yMDA1MzAxMDQ4MzhaMHkxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEUMBIGA1UEBxMLU2Fu dGEgQ2xhcmExGjAYBgNVBAoTEUludGVsIENvcnBvcmF0aW9uMSswKQYDVQQDEyJJbnRlbCBFeHRl cm5hbCBCYXNpYyBJc3N1aW5nIENBIDRCMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA yzuW/y/g0bznz8BD48M94luFzqHaqY9yGN9H/W0J7hOVBpl0rTQJ6kZ7z7hyDb9kf2UW4ZU25alC i+q5m6NwHg+z9pcN7bQ84SSBueaYF7cXlAg7z3XyZbzSEYP7raeuWRf5fYvYzq8/uI7VNR8o/43w PtDP10YDdO/0J5xrHxnC/9/aU+wTFSVsPqxsd7C58mnu7G4VRJ0n9PG4SfmYNC0h/5fLWuOWhxAv 6MuiK7MmvTPHLMclULgJqVSqG1MbBs0FbzoRHne4Cx0w6rtzPTrzo+bTRqhruaU18lQkzBk6OnyJ UthtaDQIlfyGy2IlZ5F6QEyjItbdKcHHdjBX8wIDAQABo4IBdzCCAXMwHwYDVR0jBBgwFoAUrb2Y ejS0Jvf6xCZU7wO94CTLVBowHQYDVR0OBBYEFNpBI5xaj3GvV4M+INPjZdsMywvbMA4GA1UdDwEB /wQEAwIBhjASBgNVHRMBAf8ECDAGAQH/AgEAMDYGA1UdJQQvMC0GCCsGAQUFBwMEBgorBgEEAYI3 CgMEBgorBgEEAYI3CgMMBgkrBgEEAYI3FQUwFwYDVR0gBBAwDjAMBgoqhkiG+E0BBQFpMEkGA1Ud HwRCMEAwPqA8oDqGOGh0dHA6Ly9jcmwudHJ1c3QtcHJvdmlkZXIuY29tL0FkZFRydXN0RXh0ZXJu YWxDQVJvb3QuY3JsMDoGCCsGAQUFBwEBBC4wLDAqBggrBgEFBQcwAYYeaHR0cDovL29jc3AudHJ1 c3QtcHJvdmlkZXIuY29tMDUGA1UdHgQuMCygKjALgQlpbnRlbC5jb20wG6AZBgorBgEEAYI3FAID oAsMCWludGVsLmNvbTANBgkqhkiG9w0BAQUFAAOCAQEAp9XGgH85hk/3IuN8F4nrFd24MAoau7Uq M/of09XtyYg2dV0TIPqtxPZw4813r78WwsGIbvtO8VQ18dNktIxaq6+ym2zebqDh0z6Bvo63jKE/ HMj8oNV3ovnuo+7rGpCppcda4iVBG2CetB3WXbUVr82EzECN+wxmC4H9Rup+gn+t+qeBTaXulQfV TYOvZ0eZPO+DyC2pVv5q5+xHljyUsVqpzsw89utuO8ZYaMsQGBRuFGOncRLEOhCtehy5B5aCI571 i4dDAv9LPODrEzm3PBfrNhlp8C0skak15VXWFzNuHd00AsxXxWSUT4TG8RiAH61Ua5GXsP1BIZwl 4WjK8DCCBYowggRyoAMCAQICEzMAADKN2wraLg2L7TkAAAAAMo0wDQYJKoZIhvcNAQEFBQAweTEL MAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNBMRQwEgYDVQQHEwtTYW50YSBDbGFyYTEaMBgGA1UEChMR SW50ZWwgQ29ycG9yYXRpb24xKzApBgNVBAMTIkludGVsIEV4dGVybmFsIEJhc2ljIElzc3Vpbmcg Q0EgNEIwHhcNMTcwMTI0MTQyOTM1WhcNMTgwMTE5MTQyOTM1WjBJMRswGQYDVQQDExJMYW5raG9y c3QsIE1hYXJ0ZW4xKjAoBgkqhkiG9w0BCQEWG21hYXJ0ZW4ubGFua2hvcnN0QGludGVsLmNvbTCC ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAONfR6BaGMj7h+KRpuIsoqZl3aDEI2hkJAhB osso6S9RNBCrzWi2I1LcpalFdHhDm/GXbFyX3lFkzp66aIv3vdzSktoo6Bze3A30l50hUzouAUdF JX0UiBAKsW8koT2flSRb78s3kYnJhVCSpaYxyKEB4PqNPGg1V5kA2Dar+jeoZtbl1jLbs/krLEW4 gc4OmzzkRo+jF2IJm5fL2S7fxCD1ZOYuaJSgXZjpWspJghtOw/ucsIe15Ub3rAyKA6WfyeZRTpKv 4QqKvoZ8s1FZkSjyNoItRMG8ry1ynpPH0uVQtrTqs59DSMQjTLRVAylj6AoVPSGdTFI404Gf7U/V RbsCAwEAAaOCAjkwggI1MB0GA1UdDgQWBBRgVzvXIeZkmvUcHidu2JGZ7lucnzAfBgNVHSMEGDAW gBTaQSOcWo9xr1eDPiDT42XbDMsL2zBlBgNVHR8EXjBcMFqgWKBWhlRodHRwOi8vd3d3LmludGVs LmNvbS9yZXBvc2l0b3J5L0NSTC9JbnRlbCUyMEV4dGVybmFsJTIwQmFzaWMlMjBJc3N1aW5nJTIw Q0ElMjA0Qi5jcmwwgZ8GCCsGAQUFBwEBBIGSMIGPMCIGCCsGAQUFBzABhhZodHRwOi8vb2NzcC5p bnRlbC5jb20vMGkGCCsGAQUFBzAChl1odHRwOi8vd3d3LmludGVsLmNvbS9yZXBvc2l0b3J5L2Nl cnRpZmljYXRlcy9JbnRlbCUyMEV4dGVybmFsJTIwQmFzaWMlMjBJc3N1aW5nJTIwQ0ElMjA0Qi5j cnQwCwYDVR0PBAQDAgeAMDwGCSsGAQQBgjcVBwQvMC0GJSsGAQQBgjcVCIbDjHWEmeVRg/2BKIWO n1OCkcAJZ4HevTmV8EMCAWQCAQkwHwYDVR0lBBgwFgYIKwYBBQUHAwQGCisGAQQBgjcKAwwwKQYJ KwYBBAGCNxUKBBwwGjAKBggrBgEFBQcDBDAMBgorBgEEAYI3CgMMMFMGA1UdEQRMMEqgKwYKKwYB BAGCNxQCA6AdDBttYWFydGVuLmxhbmtob3JzdEBpbnRlbC5jb22BG21hYXJ0ZW4ubGFua2hvcnN0 QGludGVsLmNvbTANBgkqhkiG9w0BAQUFAAOCAQEAOK0npk5+7PZnWtxZ3/GAl4/z22WrHQefYKUI +85U8BTTseVpsWjEg3DQgvTJ+a3q01iR4tEXfmrdQ9mD1d9AU41AIBVK0pXsEQc52hzBJ+xsT/en 5mixv04VAZHB6ZsHbAohXk16XiDW650AIdNPStJKWVQgkreWpIRQjZ0KJ9zJpiSeJjJYvjt3M5Q1 GCnYyh9VpKocvnsu+O+Rz9IJfd0VIVob6/nqd/nEvZZZhxuexzTZefxSs6+5rnkBFG9inVMMhsvP VHumop5vbakXSunxIdJLXGij3hhjk9NnYc5/yHI/qzl4U5CHOHrxcQN1eg4I7x5wpCFQgG7F2t2x rDGCAhcwggITAgEBMIGQMHkxCzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEUMBIGA1UEBxMLU2Fu dGEgQ2xhcmExGjAYBgNVBAoTEUludGVsIENvcnBvcmF0aW9uMSswKQYDVQQDEyJJbnRlbCBFeHRl cm5hbCBCYXNpYyBJc3N1aW5nIENBIDRCAhMzAAAyjdsK2i4Ni+05AAAAADKNMAkGBSsOAwIaBQCg XTAYBgkqhkiG9w0BCQMxCwYJKoZIhvcNAQcBMBwGCSqGSIb3DQEJBTEPFw0xNzA1MzAxNDM1MDRa MCMGCSqGSIb3DQEJBDEWBBQpw2BbHznvodDqHX4t1qcFFt4L2DANBgkqhkiG9w0BAQEFAASCAQBy zyF7eLPPECLBQcGtXy3+nLX1mQodhzyqq6eQhDTbTczbJVhUS2kpICRdtOFW8qsacTpgOI5CdBnZ FBsHJW0BQ5REOqguqLlDPHVcM7jT9CTMtSGnGVf16Rky6VFRxRQXfUS0h4ptp1oO4ms6m+GYAD/V DhooIc1S9mD2/aiS1vB+ZKgpDO/1oKauiHiqvCrUZ46w02KSHKfqvNNjBzjJ38eAt44ulXbppKBF cHZWOeWDxzKSN/la9AS3U/1kenwaJCibvnHntWSegzREQR/BvlmV4P36SF3kyUjkgm3Qhv/KHunQ mBUGNe22RMCRkF2uotfBUhbevP/YE/vq3xywAAAAAAAA --=-3pg3uQPHeNjS8YxuatwX-- --===============0620532854== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0620532854==--