From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback Date: Fri, 28 Apr 2017 15:58:53 +0300 Message-ID: <20170428125853.GZ30290@intel.com> References: <3e1dba0cd5fc053e56f1c9c94d0cb8789ecca4ae.1493203049.git.joabreu@synopsys.com> <20170428114127.GY30290@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2C1CE6E0EF for ; Fri, 28 Apr 2017 12:59:01 +0000 (UTC) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jose Abreu Cc: Daniel Vetter , Alexey Brodkin , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Carlos Palminha List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBBcHIgMjgsIDIwMTcgYXQgMDE6MzA6MTZQTSArMDEwMCwgSm9zZSBBYnJldSB3cm90 ZToKPiBIaSBWaWxsZSwKPiAKPiAKPiBUaGFua3MgZm9yIHRoZSByZXZpZXchIE15IGNvbW1lbnRz IGlubGluZS4KPiAKPiAKPiBPbiAyOC0wNC0yMDE3IDEyOjQxLCBWaWxsZSBTeXJqw6Rsw6Qgd3Jv dGU6Cj4gPiBPbiBXZWQsIEFwciAyNiwgMjAxNyBhdCAxMTo0ODozNEFNICswMTAwLCBKb3NlIEFi cmV1IHdyb3RlOgo+ID4+IFNvbWUgY3J0YydzIG1heSBoYXZlIHJlc3RyaWN0aW9ucyBpbiB0aGUg bW9kZSB0aGV5IGNhbiBkaXNwbGF5LiBJbgo+ID4+IHRoaXMgcGF0Y2ggYSBuZXcgY2FsbGJhY2sg KGNydGMtPm1vZGVfdmFsaWQoKSkgaXMgaW50cm9kdWNlZCB0aGF0Cj4gPj4gaXMgY2FsbGVkIGF0 IHRoZSBzYW1lIHN0YWdlIG9mIGNvbm5lY3Rvci0+bW9kZV92YWxpZCgpIGNhbGxiYWNrLgo+ID4+ Cj4gPj4gVGhpcyBzaGFsbCBiZSBpbXBsZW1lbnRlZCBpZiB0aGUgY3J0YyBoYXMgc29tZSBzb3J0 IG9mIHJlc3RyaWN0aW9uCj4gPj4gc28gdGhhdCB3ZSBkb24ndCBwcm9iZSBtb2RlcyB0aGF0IHdp bGwgZmFpbCBpbiB0aGUgY29tbWl0KCkgc3RhZ2UuCj4gPj4gRm9yIGV4YW1wbGU6IEEgZ2l2ZW4g Y3J0YyBtYXkgYmUgcmVzcG9uc2libGUgdG8gc2V0IGEgY2xvY2sgdmFsdWUuCj4gPj4gSWYgdGhl IGNsb2NrIGNhbiBub3QgcHJvZHVjZSBhbGwgdGhlIHZhbHVlcyBmb3IgdGhlIGF2YWlsYWJsZQo+ ID4+IG1vZGVzIHRoZW4gdGhpcyBjYWxsYmFjayBjYW4gYmUgdXNlZCB0byByZXN0cmljdCB0aGUg bnVtYmVyIG9mCj4gPj4gcHJvYmJlZCBtb2RlcyB0byBvbmx5IHRoZSBvbmVzIHRoYXQgY2FuIGJl IGRpc3BsYXllZC4KPiA+Pgo+ID4+IElmIHRoZSBjcnRjIGRvZXMgbm90IGltcGxlbWVudCB0aGUg Y2FsbGJhY2sgdGhlbiB0aGUgYmVoYXZpb3VyIHdpbGwKPiA+PiByZW1haW4gdGhlIHNhbWUuIEFs c28sIGZvciBhIGdpdmVuIHNldCBvZiBjcnRjcyB0aGF0IGNhbiBiZSBib3VuZCB0bwo+ID4+IHRo ZSBjb25uZWN0b3IsIGlmIGF0IGxlYXN0IG9uZSBjYW4gZGlzcGxheSB0aGUgbW9kZSB0aGVuIHRo ZSBtb2RlCj4gPj4gd2lsbCBiZSBwcm9iYmVkLgo+ID4+Cj4gPj4gU2lnbmVkLW9mZi1ieTogSm9z ZSBBYnJldSA8am9hYnJldUBzeW5vcHN5cy5jb20+Cj4gPj4gQ2M6IENhcmxvcyBQYWxtaW5oYSA8 cGFsbWluaGFAc3lub3BzeXMuY29tPgo+ID4+IENjOiBBbGV4ZXkgQnJvZGtpbiA8YWJyb2RraW5A c3lub3BzeXMuY29tPgo+ID4+IENjOiBWaWxsZSBTeXJqw6Rsw6QgPHZpbGxlLnN5cmphbGFAbGlu dXguaW50ZWwuY29tPgo+ID4+IENjOiBEYW5pZWwgVmV0dGVyIDxkYW5pZWwudmV0dGVyQGZmd2xs LmNoPgo+ID4+IENjOiBEYXZlIEFpcmxpZSA8YWlybGllZEBsaW51eC5pZT4KPiA+PiAtLS0KPiA+ PiAgZHJpdmVycy9ncHUvZHJtL2RybV9wcm9iZV9oZWxwZXIuYyAgICAgICB8IDQ0ICsrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrCj4gPj4gIGluY2x1ZGUvZHJtL2RybV9tb2Rlc2V0X2hl bHBlcl92dGFibGVzLmggfCAyNiArKysrKysrKysrKysrKysrKysrCj4gPj4gIDIgZmlsZXMgY2hh bmdlZCwgNzAgaW5zZXJ0aW9ucygrKQo+ID4+Cj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1 L2RybS9kcm1fcHJvYmVfaGVscGVyLmMgYi9kcml2ZXJzL2dwdS9kcm0vZHJtX3Byb2JlX2hlbHBl ci5jCj4gPj4gaW5kZXggMWIwYzE0YS4uNjFlYWMzMCAxMDA2NDQKPiA+PiAtLS0gYS9kcml2ZXJz L2dwdS9kcm0vZHJtX3Byb2JlX2hlbHBlci5jCj4gPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2Ry bV9wcm9iZV9oZWxwZXIuYwo+ID4+IEBAIC04MCw2ICs4MCw0NiBAQAo+ID4+ICAJcmV0dXJuIE1P REVfT0s7Cj4gPj4gIH0KPiA+PiAgCj4gPj4gK3N0YXRpYyBlbnVtIGRybV9tb2RlX3N0YXR1cyBk cm1fbW9kZV92YWxpZGF0ZV9jb25uZWN0b3JfY3J0YygKPiA+PiArCQlzdHJ1Y3QgZHJtX2Nvbm5l Y3RvciAqY29ubmVjdG9yLAo+ID4+ICsJCXN0cnVjdCBkcm1fZGlzcGxheV9tb2RlICptb2RlKQo+ ID4gUHJvYmFibHkgbW9yZSB0eXBpY2FsIHdheSB0byBzcGxpdCB0aGUgbGluZXMgd291bGQgYmU6 Cj4gPiBzdGF0aWMgZW51bSBkcm1fbW9kZV9zdGF0dXMKPiA+IGRybV9tb2RlX3ZhbGlkYXRlX2Nv bm5lY3Rvcl9jcnRjKHN0cnVjdCBkcm1fY29ubmVjdG9yICpjb25uZWN0b3IsCj4gPiAJCQkJIHN0 cnVjdCBkcm1fZGlzcGxheV9tb2RlICptb2RlKQo+IAo+IEFncmVlLgo+IAo+ID4KPiA+IEFsc28g J21vZGUnIHNob3VsZCBiZSBjb25zdC4gTG9va3MgbGlrZSB0aGUgY29ubmVjdG9yLT5tb2RlX3Zh bGlkKCkKPiA+IGhvb2sgaXMgbWlzc2luZyB0aGUgY29uc3QgYXMgd2VsbCwgc28gdGhhdCB0b28g c2hvdWxkIGJlIGZpeGVkLgo+IAo+IFllYWgsIHRoYXRzIHdoeSBJIGRpZG4ndCB1c2UgY29uc3Qg aGVyZS4gSWYgSSBjaGFuZ2UKPiBjb25uZWN0b3ItPm1vZGVfdmFsaWQoKSB0aGVuIEkgd2lsbCBu ZWVkIHRvIGNoYW5nZSBldmVyeSBkcml2ZXIKPiB3aG8gdXNlcyBpdC4gUGxlYXNlIGRvbid0IGdl dCBtZSB3cm9uZyBidXQgSSdtIG5vdCBmdWxseQo+IGFsbG9jYXRlZCB0byBwYXRjaCBzdWJtaXNz aW9uLCBjYW4gd2UgZm9jdXMgb24gZ2V0dGluZyB0aGlzIHBhdGNoCj4gbWVyZ2VkIGZpcnN0IGFu ZCB0aGVuIGNoYW5nZSB0aGUgY29ubmVjdG9yLT5tb2RlX3ZhbGlkKCkgdG8gY29uc3QKPiB3aGVu IEkgaGF2ZSB0aGUgdGltZT8KClNob3VsZCBiZSBhIGdvb2QgY29jY2luZWxsZSBleGVyY2lzZSA7 KSBCdXQgaXQgY2FuIGNlcnRhaW5seSBiZSBkb25lCmxhdGVyLgoKPiAKPiA+Cj4gPiBOb3Qgc3Vy ZSBhYm91dCB0aGUgbmFtZSBvZiB0aGUgZnVuY3Rpb24uIEl0IGRvZXNuJ3QgcmVhbGx5IHJlZmxl Y3Qgd2hhdAo+ID4gaXQgZG9lcyBpbiB0aGUgYmVzdCB3YXkgcG9zc2libGUuIGRybV9tb2RlX3Zh bGlkYXRlX2Nvbm5lY3Rvcl9wb3NzaWJsZV9jcnRjcygpCj4gPiBwZXJoYXBzPyBCdXQgdGhhdCBp cyBnZXR0aW5nIHF1aXRlIGxvbmcgc28gbm90IHN1cmUgaWYgaXQnIGEgZ29vZCBpZGVhCj4gPiBl aXRoZXIuIEkgZ3Vlc3Mgc2ltcGx5IG1ha2luZyB0aGUgbmFtZSByZWZlciB0byBwbHVyYWwgY3J0 Y3MgbWlnaHQgYmUKPiA+IGEgcmVhc29uYWJsZSBjb21wcm9taXNlLCBpZS4gZHJtX21vZGVfdmFs aWRhdGVfY29ubmVjdG9yX2NydGNzKCk/Cj4gCj4gU291bmRzIGZpbmUgYnkgbWUgOikKPiAKPiA+ Cj4gPj4gK3sKPiA+PiArCWNvbnN0IHN0cnVjdCBkcm1fY3J0Y19oZWxwZXJfZnVuY3MgKmNydGNf ZnVuY3MgPSBOVUxMOwo+ID4+ICsJZW51bSBkcm1fbW9kZV9zdGF0dXMgbW9kZV9zdGF0dXMgPSBN T0RFX0VSUk9SOwo+ID4+ICsJc3RydWN0IGRybV9kZXZpY2UgKmRldiA9IGNvbm5lY3Rvci0+ZGV2 Owo+ID4+ICsJc3RydWN0IGRybV9lbmNvZGVyICplbmNvZGVyOwo+ID4+ICsJc3RydWN0IGRybV9j cnRjICpjcnRjOwo+ID4gQSBsb3Qgb2YgdGhlc2UgY2FuIGJlIG1vdmVkIGludG8gdGlnaHRlciBz Y29wZS4KPiAKPiBBZ3JlZS4KPiAKPiA+Cj4gPj4gKwlib29sIGNhbGxiYWNrX2ZvdW5kID0gZmFs c2U7Cj4gPiBJIGRvbid0IHRoaW5rIHlvdSBuZWVkIHRoaXMgdmFyaWFibGUgYXQgYWxsLiBTZWUg YmVsb3cuCj4gPgo+ID4+ICsJaW50IGk7Cj4gPj4gKwo+ID4+ICsJZm9yIChpID0gMDsgaSA8IERS TV9DT05ORUNUT1JfTUFYX0VOQ09ERVI7IGkrKykgewo+ID4+ICsJCWVuY29kZXIgPSBkcm1fZW5j b2Rlcl9maW5kKGRldiwgY29ubmVjdG9yLT5lbmNvZGVyX2lkc1tpXSk7Cj4gPj4gKwo+ID4+ICsJ CWlmICghZW5jb2RlcikKPiA+PiArCQkJY29udGludWU7Cj4gPj4gKwo+ID4+ICsJCWRybV9mb3Jf ZWFjaF9jcnRjKGNydGMsIGRldikgewo+ID4+ICsJCQljcnRjX2Z1bmNzID0gY3J0Yy0+aGVscGVy X3ByaXZhdGU7Cj4gPj4gKwo+ID4+ICsJCQlpZiAoIWRybV9lbmNvZGVyX2NydGNfb2soZW5jb2Rl ciwgY3J0YykpCj4gPj4gKwkJCQljb250aW51ZTsKPiA+PiArCQkJaWYgKCFjcnRjX2Z1bmNzIHx8 ICFjcnRjX2Z1bmNzLT5tb2RlX3ZhbGlkKQo+ID4+ICsJCQkJY29udGludWU7Cj4gPj4gKwo+ID4+ ICsJCQkvKiBNT0RFX09LPTAgYW5kIGRlZmF1bHQgbW9kZV9zdGF0dXM9TU9ERV9FUlJPUj0tMQo+ ID4+ICsJCQkgKiBzbyBpZiBhdCBsZWFzdCBvbmUgY3J0YyBhY2NlcHRzIHRoZSBtb2RlIHdlIGdl dAo+ID4+ICsJCQkgKiBNT0RFX09LICovCj4gPj4gKwkJCW1vZGVfc3RhdHVzICY9IGNydGNfZnVu Y3MtPm1vZGVfdmFsaWQoY3J0YywgbW9kZSk7Cj4gPj4gKwkJCWNhbGxiYWNrX2ZvdW5kIHw9IHRy dWU7Cj4gPiBUaGUgbmVlZCBmb3IgYSBjb21tZW50IGhlcmUgdGVsbHMgbWUgdGhhdCBpdCdzIHBy b2JhYmx5IGJldHRlcgo+ID4gdG8gZ290IGZvciBhIG1vcmUgc3RyYWlnaHRmb3J3YXJkIGNvZGUu IFNvbWV0aGluZyBsaWtlOgo+ID4KPiA+IG1vZGVfc3RhdHVzID0gY3J0Y19mdW5jcy0+bW9kZV92 YWxpZChjcnRjLCBtb2RlKTsKPiA+IGlmIChtb2RlX3N0YXR5cyA9PSBNT0RFX09LKQo+ID4gCXJl dHVybiBtb2RlX3N0YXR1czsKPiAKPiBBY3R1YWxseSBJIHdhcyBwbGFubmluZyB0byBzZW5kIGEg bmV3IHZlcnNpb24gd2l0aCAicmV0dXJuIiBpbgo+IHRoZSBpbm5lciBsb29wIHRvIGF2b2lkIHVu bmVlZGVkIHdvcmsgYXMgdGhlIG1vZGUgd291bGQgYmUKPiBhY2NlcHRlZCBhbnl3YXksIHNvIHRo YXRzIG1vcmUgdGhhbiBhZ3JlZWQgOikKPiAKPiA+Cj4gPiBBbmQgYXQgdGhlIGVuZCBvZiB0aGUg ZnVuY3Rpb24ganVzdCByZXR1cm4gTU9ERV9FUlJPUiwgb3IKPiA+IHNvbWUgb3RoZXIgZXJyb3Ig dmFsdWUgaWYgd2UgaGF2ZSBzb21ldGhpbmcgc3VpdGFibGUuIEhtbS4KPiA+IFBlcmhhcHMgd2Ug c2hvdWxkIGp1c3QgcmV0dXJuIHRoZSBlcnJvciBmcm9tIHRoZSBmaXJzdCBvciBsYXN0Cj4gPiBj cnRjPyBFaXRoZXIgc2hvdWxkIGJlIHByZXR0eSBlYXN5LCBqdXN0ICJyZXQgPSBtb2RlX3N0YXR1 cyIgd2l0aGluCj4gPiB0aGUgbG9vcCBpZiBpdCBkaWRuJ3QgcmV0dXJuIE1PREVfT0sgYW5kIHRo ZW4gJ3JldHVybiByZXQnIGF0IHRoZQo+ID4gZW5kIG9mIHRoZSBmdW5jdGlvbi4KPiAKPiBXaGVu IEkgd2FzIHdyaXRpbmcgdGhpcyBJIGFsc28gdGhvdWdoIGFib3V0IHdoYXQgZXJyb3Igc2hvdWxk IGJlCj4gcmV0dXJuZWQgb24gZmFpbHVyZS4gSSBsZWZ0IHRoZSAiYW5kIiBtYXNrIGJ1dCBtYXli ZSBNT0RFX0VSUk9SCj4gd291bGQgYmUgbW9yZSBzdWl0YWJsZSBhcyBpdHMgbW9yZSBnZW5lcmFs LiBJJ20gdGhpbmtpbmcgdGhhdCBpZgo+IHdlIHJldHVybiBNT0RFX1NPTUVUSElORyB0aGVuIHVz ZXIgY2FuIGJlICJ0cmlja2VkIiBiZWNhdXNlIG5vdAo+IGFsbCBjcnRjcyBjYW4gYWdyZWUgb24g dGhlIGVycm9yLgoKV2VsbCBpdCB3b3VsZCBtZWFuIHRoYXQgd2Ugd291bGQgYXQgbGVhc3QgZ2l2 ZSBpbmZvcm1hdGlvbiBvbiB3aGF0Cm9uZSBjcnRjIHRob3VnaCB0aGUgZXJyb3Igd2FzLiBPdGhl cndpc2Ugd2UgZG9uJ3QgZ2l2ZSBhbnkKaW5mb3JtYXRpb24gd2h5IHRoZSBtb2RlIHdhcyByZWpl Y3RlZC4gSSdtIHRoaW5raW5nIHNvbWUgaW5mb21hdGlvbiBpcwpiZXR0ZXIgdGhhbiBub25lLgoK PiAKPiA+Cj4gPj4gKwkJfQo+ID4+ICsJfQo+ID4+ICsKPiA+PiArCS8qIFdlIGNhbiByZWFjaCBo ZXJlIHdpdGhvdXQgY2FsbGluZyBtb2RlX3ZhbGlkIGlmIHRoZXJlIGlzIG5vCj4gPj4gKwkgKiBy ZWdpc3RlcmVkIGNydGMgd2l0aCB0aGlzIGNhbGxiYWNrLCBsZXRzIHJldHVybiBNT0RFX09LIGlu IHRoaXMKPiA+PiArCSAqIGNhc2UgKi8KPiA+PiArCXJldHVybiBjYWxsYmFja19mb3VuZCA/IG1v ZGVfc3RhdHVzIDogTU9ERV9PSzsKPiA+PiArfQo+ID4+ICsKPiA+PiAgc3RhdGljIGludCBkcm1f aGVscGVyX3Byb2JlX2FkZF9jbWRsaW5lX21vZGUoc3RydWN0IGRybV9jb25uZWN0b3IgKmNvbm5l Y3RvcikKPiA+PiAgewo+ID4+ICAJc3RydWN0IGRybV9jbWRsaW5lX21vZGUgKmNtZGxpbmVfbW9k ZTsKPiA+PiBAQCAtNDMxLDYgKzQ3MSwxMCBAQCBpbnQgZHJtX2hlbHBlcl9wcm9iZV9zaW5nbGVf Y29ubmVjdG9yX21vZGVzKHN0cnVjdCBkcm1fY29ubmVjdG9yICpjb25uZWN0b3IsCj4gPj4gIAkJ aWYgKG1vZGUtPnN0YXR1cyA9PSBNT0RFX09LICYmIGNvbm5lY3Rvcl9mdW5jcy0+bW9kZV92YWxp ZCkKPiA+PiAgCQkJbW9kZS0+c3RhdHVzID0gY29ubmVjdG9yX2Z1bmNzLT5tb2RlX3ZhbGlkKGNv bm5lY3RvciwKPiA+PiAgCQkJCQkJCQkgICBtb2RlKTsKPiA+PiArCj4gPj4gKwkJaWYgKG1vZGUt PnN0YXR1cyA9PSBNT0RFX09LKQo+ID4+ICsJCQltb2RlLT5zdGF0dXMgPSBkcm1fbW9kZV92YWxp ZGF0ZV9jb25uZWN0b3JfY3J0YygKPiA+PiArCQkJCQljb25uZWN0b3IsIG1vZGUpOwo+ID4gSW5k ZW50YXRpb24uCj4gPgo+ID4gQW5kIGFjdHVhbGx5LCBtYXliZSB5b3Ugc2hvdWxkIGFsc28gbW92 ZSB0aGUgY29ubmVjdG9yX2Z1bmNzLT5tb2RlX3ZhbGlkKCkKPiA+IGNhbGwgaW50byB0aGUgbmV3 IGZ1bmN0aW9uLCBhbmQgdGhlbiB5b3UgY291bGQganVzdCBjYWxsIHRoZSBuZXcKPiA+IGZ1bmN0 aW9uIGRybV9tb2RlX3ZhbGlkYXRlX2Nvbm5lY3RvcigpIG9yIHNvbWV0aGluZyBhbG9uZyB0aG9z ZSBsaW5lcy4KPiAKPiAiZHJtX21vZGVfdmFsaWRhdGVfcGlwZWxpbmUiID8gaS5lLiBtYXliZSBp ZiBpbiB0aGUgZnV0dXJlIHdlCj4gaGF2ZSB0aGUgbmVlZCBmb3IgYW4gZW5jb2Rlci0+bW9kZV92 YWxpZD8gKHJlbW90ZSB0aG91Z2h0KQoKV2VsbCwgd2UgcGFzcyBjb25uZWN0b3IgYXMgdGhlIHRo aW5nIHRoYXQgZHJpdmVzIHRoZSB2YWxpZGF0aW9uIGhlcmUsIHNvCmhhdmluZyBjb25uZWN0b3Ig aXMgdGhlIG5hbWUgaXMgYXBwcm9wcmlhdGUgSSB0aGluay4KCj4gCj4gPgo+ID4+ICAJfQo+ID4+ ICAKPiA+PiAgcHJ1bmU6Cj4gPj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvZHJtL2RybV9tb2Rlc2V0 X2hlbHBlcl92dGFibGVzLmggYi9pbmNsdWRlL2RybS9kcm1fbW9kZXNldF9oZWxwZXJfdnRhYmxl cy5oCj4gPj4gaW5kZXggYzAxYzMyOC4uNTlmZmZiYSAxMDA2NDQKPiA+PiAtLS0gYS9pbmNsdWRl L2RybS9kcm1fbW9kZXNldF9oZWxwZXJfdnRhYmxlcy5oCj4gPj4gKysrIGIvaW5jbHVkZS9kcm0v ZHJtX21vZGVzZXRfaGVscGVyX3Z0YWJsZXMuaAo+ID4+IEBAIC0xMDYsNiArMTA2LDMyIEBAIHN0 cnVjdCBkcm1fY3J0Y19oZWxwZXJfZnVuY3Mgewo+ID4+ICAJdm9pZCAoKmNvbW1pdCkoc3RydWN0 IGRybV9jcnRjICpjcnRjKTsKPiA+PiAgCj4gPj4gIAkvKioKPiA+PiArCSAqIEBtb2RlX3ZhbGlk Ogo+ID4+ICsJICoKPiA+PiArCSAqIFRoaXMgY2FsbGJhY2sgc2hvdWxkIGJlIGltcGxlbWVudGVk IGlmIHRoZSBjcnRjIGhhcyBzb21lIHNvcnQgb2YKPiA+PiArCSAqIHJlc3RyaWN0aW9uIGluIHRo ZSBtb2RlcyBpdCBjYW4gZGlzcGxheS4gRm9yIGV4YW1wbGUsIGEgZ2l2ZW4gY3J0Ywo+ID4+ICsJ ICogbWF5IGJlIHJlc3BvbnNpYmxlIHRvIHNldCBhIGNsb2NrIHZhbHVlLiBJZiB0aGUgY2xvY2sg Y2FuIG5vdAo+ID4+ICsJICogcHJvZHVjZSBhbGwgdGhlIHZhbHVlcyBmb3IgdGhlIGF2YWlsYWJs ZSBtb2RlcyB0aGVuIHRoaXMgY2FsbGJhY2sKPiA+PiArCSAqIGNhbiBiZSB1c2VkIHRvIHJlc3Ry aWN0IHRoZSBudW1iZXIgb2YgcHJvYmJlZCBtb2RlcyB0byBvbmx5IHRoZSBvbmVzCj4gPj4gKwkg KiB0aGF0IGNhbiBiZSBkaXNwbGF5ZWQuCj4gPj4gKwkgKgo+ID4+ICsJICogVGhpcyBpcyBkaXJl Y3RseSBjYWxsZWQgYXQgdGhlIHNhbWUgc3RhZ2Ugb2YgY29ubmVjdG9yLT5tb2RlX3ZhbGlkCj4g Pj4gKwkgKiBjYWxsYmFjay4KPiA+PiArCSAqCj4gPj4gKwkgKiBOT1RFOgo+ID4+ICsJICoKPiA+ PiArCSAqIEZvciBhIGdpdmVuIHNldCBvZiBjcnRjJ3MgaW4gYSBkcm1fZGV2aWNlLCBpZiBhdCBs ZWFzdCBvbmUgZG9lcyBub3QKPiA+PiArCSAqIGhhdmUgdGhlIG1vZGVfdmFsaWQgY2FsbGJhY2ss IG9yLCBhdCBsZWFzdCBvbmUgcmV0dXJucyBNT0RFX09LIHRoZW4KPiA+PiArCSAqIHRoZSBtb2Rl IHdpbGwgYmUgcHJvYmJlZC4KPiA+PiArCSAqCj4gPj4gKwkgKiBSRVRVUk5TOgo+ID4+ICsJICoK PiA+PiArCSAqIGRybV9tb2RlX3N0YXR1cyBFbnVtCj4gPj4gKwkgKi8KPiA+PiArCWVudW0gZHJt X21vZGVfc3RhdHVzICgqbW9kZV92YWxpZCkoc3RydWN0IGRybV9jcnRjICpjcnRjLAo+ID4+ICsJ CQlzdHJ1Y3QgZHJtX2Rpc3BsYXlfbW9kZSAqbW9kZSk7Cj4gPiBDb25zdC4gSW5kZW50YXRpb24g bG9va3Mgb2ZmIGFnYWluLgo+IAo+IEFncmVlLgo+IAo+ID4KPiA+PiArCj4gPj4gKwkvKioKPiA+ PiAgCSAqIEBtb2RlX2ZpeHVwOgo+ID4+ICAJICoKPiA+PiAgCSAqIFRoaXMgY2FsbGJhY2sgaXMg dXNlZCB0byB2YWxpZGF0ZSBhIG1vZGUuIFRoZSBwYXJhbWV0ZXIgbW9kZSBpcyB0aGUKPiA+PiAt LSAKPiA+PiAxLjkuMQo+ID4+Cj4gCj4gCj4gQmVzdCByZWdhcmRzLAo+IEpvc2UgTWlndWVsIEFi cmV1CgotLSAKVmlsbGUgU3lyasOkbMOkCkludGVsIE9UQwpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZl bEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFp bG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1165305AbdD1M7H (ORCPT ); Fri, 28 Apr 2017 08:59:07 -0400 Received: from mga03.intel.com ([134.134.136.65]:37419 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1165271AbdD1M67 (ORCPT ); Fri, 28 Apr 2017 08:58:59 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,388,1488873600"; d="scan'208";a="1162180040" Date: Fri, 28 Apr 2017 15:58:53 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jose Abreu Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Carlos Palminha , Alexey Brodkin , Daniel Vetter , Dave Airlie Subject: Re: [PATCH 1/2] drm: Introduce crtc->mode_valid() callback Message-ID: <20170428125853.GZ30290@intel.com> References: <3e1dba0cd5fc053e56f1c9c94d0cb8789ecca4ae.1493203049.git.joabreu@synopsys.com> <20170428114127.GY30290@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 28, 2017 at 01:30:16PM +0100, Jose Abreu wrote: > Hi Ville, > > > Thanks for the review! My comments inline. > > > On 28-04-2017 12:41, Ville Syrjälä wrote: > > On Wed, Apr 26, 2017 at 11:48:34AM +0100, Jose Abreu wrote: > >> Some crtc's may have restrictions in the mode they can display. In > >> this patch a new callback (crtc->mode_valid()) is introduced that > >> is called at the same stage of connector->mode_valid() callback. > >> > >> This shall be implemented if the crtc has some sort of restriction > >> so that we don't probe modes that will fail in the commit() stage. > >> For example: A given crtc may be responsible to set a clock value. > >> If the clock can not produce all the values for the available > >> modes then this callback can be used to restrict the number of > >> probbed modes to only the ones that can be displayed. > >> > >> If the crtc does not implement the callback then the behaviour will > >> remain the same. Also, for a given set of crtcs that can be bound to > >> the connector, if at least one can display the mode then the mode > >> will be probbed. > >> > >> Signed-off-by: Jose Abreu > >> Cc: Carlos Palminha > >> Cc: Alexey Brodkin > >> Cc: Ville Syrjälä > >> Cc: Daniel Vetter > >> Cc: Dave Airlie > >> --- > >> drivers/gpu/drm/drm_probe_helper.c | 44 ++++++++++++++++++++++++++++++++ > >> include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++ > >> 2 files changed, 70 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > >> index 1b0c14a..61eac30 100644 > >> --- a/drivers/gpu/drm/drm_probe_helper.c > >> +++ b/drivers/gpu/drm/drm_probe_helper.c > >> @@ -80,6 +80,46 @@ > >> return MODE_OK; > >> } > >> > >> +static enum drm_mode_status drm_mode_validate_connector_crtc( > >> + struct drm_connector *connector, > >> + struct drm_display_mode *mode) > > Probably more typical way to split the lines would be: > > static enum drm_mode_status > > drm_mode_validate_connector_crtc(struct drm_connector *connector, > > struct drm_display_mode *mode) > > Agree. > > > > > Also 'mode' should be const. Looks like the connector->mode_valid() > > hook is missing the const as well, so that too should be fixed. > > Yeah, thats why I didn't use const here. If I change > connector->mode_valid() then I will need to change every driver > who uses it. Please don't get me wrong but I'm not fully > allocated to patch submission, can we focus on getting this patch > merged first and then change the connector->mode_valid() to const > when I have the time? Should be a good coccinelle exercise ;) But it can certainly be done later. > > > > > Not sure about the name of the function. It doesn't really reflect what > > it does in the best way possible. drm_mode_validate_connector_possible_crtcs() > > perhaps? But that is getting quite long so not sure if it' a good idea > > either. I guess simply making the name refer to plural crtcs might be > > a reasonable compromise, ie. drm_mode_validate_connector_crtcs()? > > Sounds fine by me :) > > > > >> +{ > >> + const struct drm_crtc_helper_funcs *crtc_funcs = NULL; > >> + enum drm_mode_status mode_status = MODE_ERROR; > >> + struct drm_device *dev = connector->dev; > >> + struct drm_encoder *encoder; > >> + struct drm_crtc *crtc; > > A lot of these can be moved into tighter scope. > > Agree. > > > > >> + bool callback_found = false; > > I don't think you need this variable at all. See below. > > > >> + int i; > >> + > >> + for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > >> + encoder = drm_encoder_find(dev, connector->encoder_ids[i]); > >> + > >> + if (!encoder) > >> + continue; > >> + > >> + drm_for_each_crtc(crtc, dev) { > >> + crtc_funcs = crtc->helper_private; > >> + > >> + if (!drm_encoder_crtc_ok(encoder, crtc)) > >> + continue; > >> + if (!crtc_funcs || !crtc_funcs->mode_valid) > >> + continue; > >> + > >> + /* MODE_OK=0 and default mode_status=MODE_ERROR=-1 > >> + * so if at least one crtc accepts the mode we get > >> + * MODE_OK */ > >> + mode_status &= crtc_funcs->mode_valid(crtc, mode); > >> + callback_found |= true; > > The need for a comment here tells me that it's probably better > > to got for a more straightforward code. Something like: > > > > mode_status = crtc_funcs->mode_valid(crtc, mode); > > if (mode_statys == MODE_OK) > > return mode_status; > > Actually I was planning to send a new version with "return" in > the inner loop to avoid unneeded work as the mode would be > accepted anyway, so thats more than agreed :) > > > > > And at the end of the function just return MODE_ERROR, or > > some other error value if we have something suitable. Hmm. > > Perhaps we should just return the error from the first or last > > crtc? Either should be pretty easy, just "ret = mode_status" within > > the loop if it didn't return MODE_OK and then 'return ret' at the > > end of the function. > > When I was writing this I also though about what error should be > returned on failure. I left the "and" mask but maybe MODE_ERROR > would be more suitable as its more general. I'm thinking that if > we return MODE_SOMETHING then user can be "tricked" because not > all crtcs can agree on the error. Well it would mean that we would at least give information on what one crtc though the error was. Otherwise we don't give any information why the mode was rejected. I'm thinking some infomation is better than none. > > > > >> + } > >> + } > >> + > >> + /* We can reach here without calling mode_valid if there is no > >> + * registered crtc with this callback, lets return MODE_OK in this > >> + * case */ > >> + return callback_found ? mode_status : MODE_OK; > >> +} > >> + > >> static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) > >> { > >> struct drm_cmdline_mode *cmdline_mode; > >> @@ -431,6 +471,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > >> if (mode->status == MODE_OK && connector_funcs->mode_valid) > >> mode->status = connector_funcs->mode_valid(connector, > >> mode); > >> + > >> + if (mode->status == MODE_OK) > >> + mode->status = drm_mode_validate_connector_crtc( > >> + connector, mode); > > Indentation. > > > > And actually, maybe you should also move the connector_funcs->mode_valid() > > call into the new function, and then you could just call the new > > function drm_mode_validate_connector() or something along those lines. > > "drm_mode_validate_pipeline" ? i.e. maybe if in the future we > have the need for an encoder->mode_valid? (remote thought) Well, we pass connector as the thing that drives the validation here, so having connector is the name is appropriate I think. > > > > >> } > >> > >> prune: > >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > >> index c01c328..59fffba 100644 > >> --- a/include/drm/drm_modeset_helper_vtables.h > >> +++ b/include/drm/drm_modeset_helper_vtables.h > >> @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs { > >> void (*commit)(struct drm_crtc *crtc); > >> > >> /** > >> + * @mode_valid: > >> + * > >> + * This callback should be implemented if the crtc has some sort of > >> + * restriction in the modes it can display. For example, a given crtc > >> + * may be responsible to set a clock value. If the clock can not > >> + * produce all the values for the available modes then this callback > >> + * can be used to restrict the number of probbed modes to only the ones > >> + * that can be displayed. > >> + * > >> + * This is directly called at the same stage of connector->mode_valid > >> + * callback. > >> + * > >> + * NOTE: > >> + * > >> + * For a given set of crtc's in a drm_device, if at least one does not > >> + * have the mode_valid callback, or, at least one returns MODE_OK then > >> + * the mode will be probbed. > >> + * > >> + * RETURNS: > >> + * > >> + * drm_mode_status Enum > >> + */ > >> + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > >> + struct drm_display_mode *mode); > > Const. Indentation looks off again. > > Agree. > > > > >> + > >> + /** > >> * @mode_fixup: > >> * > >> * This callback is used to validate a mode. The parameter mode is the > >> -- > >> 1.9.1 > >> > > > Best regards, > Jose Miguel Abreu -- Ville Syrjälä Intel OTC