From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Lankhorst, Maarten" Subject: Re: [PATCH v3 7/8] drm: Connector helper function to release resources Date: Thu, 16 Feb 2017 09:09:50 +0000 Message-ID: <1487236189.2622.3.camel@intel.com> References: <1486622291-3524-1-git-send-email-dhinakaran.pandiyan@intel.com> <1486622291-3524-8-git-send-email-dhinakaran.pandiyan@intel.com> <1486630868.30626.17.camel@intel.com> <1486667623.32142.7.camel@dk-H97M-D3H> <1486976736.7640.11.camel@intel.com> <1487022288.32142.52.camel@dk-H97M-D3H> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1575706639==" Return-path: In-Reply-To: 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: "Pandiyan, Dhinakaran" , "daniel.vetter@ffwll.ch" Cc: "alexander.deucher@amd.com" , "intel-gfx@lists.freedesktop.org" , "harry.wentland@amd.com" , "bskeggs@redhat.com" , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org --===============1575706639== Content-Language: en-US Content-Type: multipart/signed; micalg=sha-1; protocol="application/x-pkcs7-signature"; boundary="=-ibpzXzhJNJLKZB9fkilk" --=-ibpzXzhJNJLKZB9fkilk Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]: > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran > wrote: > > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote: > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]: > > > > On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote: > > > > >=20 > > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [- > > > > > 0800]: > > > > > >=20 > > > > > > Having a ->atomic_release callback is useful to release > > > > > > shared > > > > > > resources > > > > > > that get allocated in compute_config(). This function is > > > > > > expected > > > > > > to > > > > > > be > > > > > > called in the atomic_check() phase before new resources are > > > > > > acquired. > > > > > >=20 > > > > > > v2: Moved the caller hunk to this patch (Daniel) > > > > > >=20 > > > > > > Suggested-by: Daniel Vetter > > > > > > Signed-off-by: Dhinakaran Pandiyan > > > > > el.com > > > > > > >=20 > > > > > >=20 > > > > > > --- > > > > > > =C2=A0drivers/gpu/drm/drm_atomic_helper.c=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0| 19 > > > > > > +++++++++++++++++++ > > > > > > =C2=A0include/drm/drm_modeset_helper_vtables.h | 13 > > > > > > +++++++++++++ > > > > > > =C2=A02 files changed, 32 insertions(+) > > > > > >=20 > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > > > > b/drivers/gpu/drm/drm_atomic_helper.c > > > > > > index 8795088..92bd741 100644 > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct > > > > > > drm_device *dev, > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > > >=20 > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for_each_connector_i= n_state(state, connector, > > > > > > connector_state, i) { > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const struct drm_connector_helper_funcs > > > > > > *conn_funcs; > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct drm_crtc_state *crtc_state; > > > > > > + > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0conn_funcs =3D connector->helper_private; > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!conn_funcs->atomic_release) > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0continue; > > > > > > + > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!connector->state->crtc) > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0continue; > > > > > > + > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0crtc_state =3D > > > > > > drm_atomic_get_existing_crtc_state(state, connector->state- > > > > > > > crtc); > > > > > >=20 > > > > > > + > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (crtc_state->connectors_changed || > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0crtc_state->mode_chan= ged || > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(crtc_state->active_c= hanged && > > > > > > !crtc_state- > > > > > > >=20 > > > > > > > active)) > > > > > >=20 > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0conn_funcs- > > > > > > >atomic_release(connector, > > > > > > connector_state); > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > >=20 > > > > > Could we deal with the VCPI state separately in > > > > > intel_modeset_checks, > > > > > like we do with dpll? > > > >=20 > > > > We'd want to release the VCPI slots before they are acquired in > > > > ->compute_config(). intel_modeset_checks() will be too late to > > > > release > > > > them. Are you suggesting both acquiring and releasing slots > > > > should be > > > > done in intel_modeset_checks()? > > >=20 > > > That makes things a bit more nasty. Maybe add a > > > conn_funcs->atomic_check that always gets called, something like > > > I did > > > below? > > >=20 > > > I'd love to use it for some atomic connector properties too. > >=20 > >=20 > > Adding and unconditionally calling conn_funcs->atomic_check() > > should be > > doable. It also follows the pattern we have for encoders and CRTCs. > > But > > I'll have to move the connector->state->crtc state checks inside > > the > > function. >=20 > Adding ->atomic_check that's unconditionally called sounds troubling, > because all the other ->atomic_check functions are _only_ called when > enabling stuff. ->atomic_release sounds much better to me, and from a > helper pov DK's patch above is the right place. Having an atomic check would be nice for implementing connector properties. Some of them may need to be validated regardless of crtc. I would really like to be able to do the validation in atomic_check instead of during the set_property callback. The state is not completely valid at that point yet, so this would be a logical place. > If that place doesn't work for i915.ko, then we need our own callback > (like we already have with e.g. ->compute_config, we could do a > ->release_config). But if it's just cosmetics, then I don't see the > reason why we need to change this. On that issue: How exactly does > our > compute_config work if we haven't updated the routing (using the > above > helper) yet? That sounds like a pretty big misdesign on our side ... > -Daniel >=20 >=20 > >=20 > > -DK > > > > >=20 > > > > >=20 > > > > > Maybe implementing the relevant VCPI state could be done as > > > > > an > > > > > atomic > > > > > helper function too, so other atomic drivers can just plug it > > > > > in. > > > > >=20 > > > >=20 > > > > The idea was to reduce boilerplate in the drivers and use the > > > > private_obj state for different object types. > > > >=20 > > > >=20 > > > > >=20 > > > > > Not sure how doable this is, but if it's not too hard, then > > > > > it's > > > > > probably cleaner :) --=-ibpzXzhJNJLKZB9fkilk 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 XTAYBgkqhkiG9w0BCQMxCwYJKoZIhvcNAQcBMBwGCSqGSIb3DQEJBTEPFw0xNzAyMTYwOTA5NDla MCMGCSqGSIb3DQEJBDEWBBQBXaUmIpUz389AOOALAlrLxy6tjTANBgkqhkiG9w0BAQEFAASCAQBP 4GcTY6SxTYjigadBKjTPrR3G3HLblt7xhAOc0xu+VGTuZ+jmRsZ7Pb9w79OheHdvCok/dyeyJdC/ c/bKoKE+aWQHBOhVk+lodc95JXE9UowBNv+zUiUeYyKtcsYQABjDF4ol7nrd79q7Q5pZ2uRYeTKE KOduNd4sQZwDMIiya4KAuuczY05tA5D48+35NPZQLef4nqfqS7YA36S2py1kHu2Kr/zZUR1r8q/T PirZ2Lq+qTRwXY4Em6hKuKlfTzs4uM5cGmdicQepsTUY2O9DR1GyJu8sIIsYQSDaFRgcKL8/QZLu vLwjpDB3c4vR8IeosvZdqLa3w7Vqf+xrEenpAAAAAAAA --=-ibpzXzhJNJLKZB9fkilk-- --===============1575706639== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1575706639==--