From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 00/13] drm/kms/mode: using helper func drm_display_mode_to/from_videomode for calculating timing parameters Date: Sun, 13 May 2018 16:12:18 +0200 Message-ID: References: <1525777110-11378-1-git-send-email-satendra.t@samsung.com> <1525866725-16685-1-git-send-email-satendra.t@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1525866725-16685-1-git-send-email-satendra.t@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Satendra Singh Thakur Cc: linux-samsung-soc , Linux Kernel Mailing List , dri-devel , "moderated list:ARM/Mediatek SoC support" , linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org, sst2005@gmail.com, Linux ARM List-Id: linux-tegra@vger.kernel.org T24gV2VkLCBNYXkgOSwgMjAxOCBhdCAxOjUyIFBNLCBTYXRlbmRyYSBTaW5naCBUaGFrdXIKPHNh dGVuZHJhLnRAc2Ftc3VuZy5jb20+IHdyb3RlOgo+IE9uIFRodSwgTWF5IDA4LCAyMDE4IGF0IDE2 OjI4OjMwICswNTMwLCBTYXRlbmRyYSBTaW5naCBUaGFrdXIgd3JvdGU6Cj4+IE9uIFRodSwgTWF5 IDA3LCAyMDE4IGF0IDE1OjQ2OjAyICswMjAwLCBEYW5pZWwgVmV0dGVyIHdyb3RlOgo+PiA+IE9u IFRodSwgTWF5IDAzLCAyMDE4IGF0IDAxOjUzOjU1UE0gKzA1MzAsIFNhdGVuZHJhIFNpbmdoIFRo YWt1ciB3cm90ZToKPj4gPiA+IDEuVGhlcmUgaXMgYSBmdW5jdGlvbiBpbiBkcm0tY29yZSB0byBj YWxjdWxhdGUgZGlzcGxheSB0aW1pbmcgcGFyYW1ldGVyczoKPj4gPiA+IGhvcml6b250YWwgZnJv bnQgcG9yY2gsIGJhY2sgcG9yY2gsIHN5bmMgbGVuZ3RoLAo+PiA+ID4gdmVydGljYWwgZnJvbnQg cG9yY2gsIGJhY2sgcG9yY2gsIHN5bmMgbGVuZ3RoIGFuZAo+PiA+ID4gY2xvY2sgaW4gSHouCj4+ ID4gPiBIb3dldmVyLCBzb21lIGRyaXZlcnMgYXJlIHN0aWxsIGNhbGN1bGF0aW5nIHRoZXNlIHBh cmFtZXRlcnMgdGhlbXNlbHZlcy4KPj4gPiA+IFRoZXJlZm9yZSwgdGhlcmUgaXMgYSBkdXBsaWNh dGlvbiBvZiB0aGUgY29kZS4KPj4gPiA+IFRoaXMgcGF0Y2ggc2VyaWVzIHJlcGxhY2VzIHRoaXMg cmVkdW5kYW50IGNvZGUgd2l0aCB0aGUgZnVuY3Rpb24KPj4gPiA+IGRybV9kaXNwbGF5X21vZGVf dG9fdmlkZW9tb2RlLgo+PiA+ID4gVGhpcyByZW1vdmVzIG5lYXJseSAxMDAgcmVkdW5kYW50IGxp bmVzIGZyb20gdGhlIHJlbGF0ZWQgZHJpdmVycy4KPj4gPiA+IDIuRm9yIHNvbWUgZHJpdmVycyAo c3VuNGkpIHRoZSByZXZlcnNlIGhlbHBlcgo+PiA+ID4gZHJtX2Rpc3BsYXlfbW9kZV9mcm9tX3Zp ZGVvbW9kZSBpcyB1c2VkLgo+PiA+ID4gMy5Gb3Igc29tZSBkcml2ZXJzIGl0IHJlcGxhY2VzIGFy aXRobWF0aWMgb3BlcmF0b3JzICgqLCAvKSB3aXRoIHNoaWZ0aW5nCj4+ID4gPiBvcGVyYXRvcnMg KD4+LCA8PCkuCj4+ID4gPiA0LkZvciBzb21lIGRyaXZlcnMgRFJNX01PREVfRkxBR18qIGFyZSBy ZXBsYWNlZCB3aXRoIERJU1BMQVlfRkxBR1NfKiBmbGFncy4KPj4gPiA+IDUuVGhlc2UgY2hhbmdl cyBhcHBseSB0byBmb2xsb3dpbmcgY3J0YyBhbmQgZW5jb2RlciBkcml2ZXJzOgo+PiA+ID4gYXRt ZWwtaGxjZGMKPj4gPiA+IGJyaWRnZS10YzM1ODc2Nwo+PiA+ID4gZXh5bm9zLWRzaQo+PiA+ID4g ZnNsLWRjdQo+PiA+ID4gZ21hNTAwLW1kZmxkX2RzaV9kcGkKPj4gPiA+IGhpc2lsaWNvbi1raXJp bl9kc2ksIGFkZQo+PiA+ID4gbWVzb24tZW5jb2Rlcgo+PiA+ID4gcGwxMTEtZGlzcGxheQo+PiA+ ID4gc3VuNGktdHYKPj4gPiA+IHRpIGxjZGMKPj4gPiA+IHRlZ3JhIGRjCj4+ID4gPiBtZWRpYXRl ayBkcGkgZHNpCj4+ID4gPiBicmlkZ2UtYWR2NzUzMwo+PiA+Cj4+ID4gVGhlIGRybV9tb2RlX3Rv X3ZpZGVvbW9kZSBoZWxwZXIgaXMgbWVhbnQgZm9yIGludGVyb3AgYmV0d2VlbiBkcm0gYW5kIHY0 bCwKPj4gPiB3aGljaCBoYXZlIGRpZmZlcmVudCBpbnRlcm5hbCBzdHJ1Y3R1cmVzIHRvIHJlcHJl c2VudCBtb2Rlcy4KPj4gPgo+PiA+IEZvciBkcml2ZXJzIHRoYXQgb25seSB1c2UgZHJtIEkgdGhp bmsgdGhlIGJldHRlciBvcHRpb24gd291bGQgYmUgdG8gYWRkCj4+ID4gdGhlc2UgZmllbGRzIHRv IHN0cnVjdCBkcm1fZGlzcGxheV9tb2RlIGFzIGFub3RoZXIgc2V0IG9mIGNydGNfKiB2YWx1ZXMK Pj4gPiAodGhlIGNvbXB1dGVkIHZhbHVlcyBhcmUgc3RvcmVkIGluIGNydGNfIHByZWZpeGVkIG1l bWJlcnMpLiBBbmQgY29tcHV0ZQo+PiA+IGZyb250L2JhY2sgcG9yY2ggaW4gZHJtX21vZGVfc2V0 X2NydGNpbmZvLgo+PiA+Cj4+ID4gVGhlbiB3ZSBjYW4gdXNlIHRoZXNlIG5ldyBkcm1fZGlzcGxh eV9tb2RlLT5jcnRjX2h8dmZyb250fGJhY2tfcG9yY2gKPj4gPiBmaWVsZHMgaW4gYWxsIHRoZSBk cml2ZXJzIHlvdSdyZSBjaGFuZ2luZy4gVGhpcyB3YXkgeW91IGF2b2lkIGhhdmluZyB0bwo+PiA+ IGNoYW5nZSBhbGwgdGhlIGRybSBkcml2ZXJzIHRvIHVzZSB2NGwgI2RlZmluZXMuCj4+ID4KPj4g PiBUaGFua3MsCj4+ID4gRGFuaWVsCj4+Cj4+IEhpIERhbmllbCwKPj4gVGhhbmtzIGZvciB0aGUg Y29tbWVudHMuCj4+IEkgd2lsbCBsb29rIGludG8gaXQuCj4+Cj4+IFRoYW5rcwo+PiAtU2F0ZW5k cmEKPgo+IEhpIERhbmllbCwKPiBUaGFua3MgZm9yIHRoZSBjb21tZW50cy4KPiBQbGVhc2UgZmlu ZCBiZWxvdyBteSB1bmRlcnN0YW5kaW5nIGluIHRoaXMgZGlyZWN0aW9uLgo+Cj4gMS4gQ3VycmVu dGx5IHN0cnVjdCB2aWRlb21vZGUgaXMgYmVpbmcgdXNlZCBpbiA1MCBkcm0gZHJpdmVycyBhbmQg MTQgZmJkZXYgZHJpdmVycy4KPiAgICBTaW5jZSwgaXQncyBhbHJlYWR5IGJlaW5nIHVzZWQgYnkg c28gbWFueSBkcm0gZHJpdmVycywgdGhhdCBpcyB0aGUgcmVhc29uCj4gICAgdGhlc2UgZmJkZXYg b2JqZWN0cyAoc3RydWN0IHZpZGVtb2RlIGFuZCBmdW5jIGRybV9kaXNwbGF5X21vZGVfdG9fdmlk ZW9tb2RlKSB3ZXJlIHVzZWQgaW4gdGhpcyBwYXRjaCBzZXJpZXMuCgpUaGUgYmlnZ2VzdCBjb250 cmlidXRvciBmb3IgdGhhdCBzZWVtcyB0byBiZSBvZl9nZXRfdmlkZW9tb2RlLiBXZQpzaG91bGQg cHJvYmFibHkgaGF2ZSBhIG9mX2RybV9nZXRfZGlzcGxheV9tb2RlIGhlbHBlciwgd2hpY2gKYXV0 b21hdGljYWxseSBjb252ZXJ0cyB0aGUgb2YvZHQgdmlkZW8gZGVzY3JpcHRpb24gaW50byB0aGUg ZHJtCmRpc3BsYXkgbW9kZSBzdHJ1Y3R1cmUuCgpBbmQgSSBmb3VuZCB3YXkgbGVzcyB0aGFuIDUw IGRybSBkcml2ZXJzIHVzaW5nIHZpZGVvbW9kZSwgbXVjaCBsZXNzIGlmCndlIGlnbm9yZSBvZi4K Cj4gMi4gQW55d2F5LCBpZiBmYmRldiByZWxhdGVkIG9iamVjdHMgKHN0cnVjdC9mdW5jKSBhcmUg bm90IGVuY291cmFnZWQgaW4gZHJtIGRyaXZlcnMsIHRoZW4gd2UgbWF5IGFkZAo+ICAgIGgvdiBm cm9udC9iYWNrIHBvcmNoZXMgaW4gc3RydWN0IGRybV9kaXNwbGF5X21vZGUgYXMgYWR2aWNlZCBi eSB5b3UuCj4KPiAzLiBXZSBjYW4gY2FsY3VsYXRlIHRoZXNlIHBhcmFtcyBpbiBmdW5jIGRybV9t b2RlX3NldF9jcnRjaW5mbyBhdCB0aGUgZW5kIG9mIGl0Lgo+ICAgIGludCBkcm1fbW9kZV9zZXRf Y3J0Y2luZm8gKCkKPiAgICB7Cj4gICAgLgo+ICAgIC4KPiAgICBjcnRjX2hmcm9udF9wb3JjaCA9 IGNydGNfaHN5bmNfc3RhcnQgLSBjcnRjX2hkaXNwbGF5Owo+ICAgIGNydGNfdmZyb250X3BvcmNo ID0gY3J0Y192c3luY19zdGFydCAtIGNydGNfdmRpc3BsYXk7Cj4gICAgLgo+ICAgIC4KPiAgICBj cnRjX2Nsb2NrX2h6ID0gY3J0Y19jbG9jayoxMDAwOwo+ICAgIH07Cj4KPiA0LiBOb3JtYWxseSBt b2RlIGlzIHByb2dyYW1tZWQgaW4gSFcgaW4gZm9sbG93aW5nIGNhbGxiYWNrcyBvZiBjcnRjIGFu ZCBlbmNvZGVyIGRyaXZlcnMKPiAgICAtbW9kZV9zZXQKPiAgICAtbW9kZV9zZXRfbm9mYgo+ICAg IC1hdG9taWNfZW5hYmxlCj4KPgo+ICAgIE5vcm1hbGx5IGRybV9tb2RlX3NldF9jcnRjaW5mbyBp cyB1c2VkIGluCj4gICAgLW1vZGVfZml4dXAgY2FsbGJhY2tzIChDQnMpCj4gICAgb2YgZW5jb2Rl ciBhbmQgY3J0YyBkcml2ZXJzLgo+Cj4gICAgaWYgbW9kZV9maXh1cCBDQnMgYXJlIGNhbGxlZCBi ZWZvcmUgbW9kZV9zZXQgQ0JzIHRoZW4KPiAgICBkcm1fbW9kZV9zZXRfY3J0Y2luZm8gaXMgcmln aHQgcGxhY2UgdG8gY2FsY3VsYXRlIHN5bmMvcG9yY2ggcGFyYW1zLgo+ICAgIFdlIGNhbiB1c2Ug Y3J0Y19oZnJvbnQvYmFja19wb3JjaGVzIGRpcmVjdGx5IGFuZCBwcm9ncmFtIHRoZW0gdG8gSFcK PiAgICBpbiBhYm92ZSBtZW50aW9uZWQgY2FsbGJhY2tzLgo+Cj4gICAgaW50IG15X21vZGVfc2V0 ICgpCj4gICAgewo+ICAgICAgICAgUkVHX1dSSVRFKGNydGNfaGZyb250X3BvcmNoKTsKPiAgICAg ICAgIFJFR19XUklURShjcnRjX2hiYWNrX3BvcmNoKTsKPiAgICAgICAgIC4KPiAgICAgICAgIC4K PiAgICB9CgpBZ3JlZWQgd2l0aCB5b3VyIHBsYW4gdXAgdG8gcG9pbnQgNSBoZXJlLgoKPiAgNi4g IEhvd2V2ZXIsIGlmIHRoZXNlIHBhcmFtcyBhcmUgYmVpbmcgbW9kaWZpZWQgYWZ0ZXIgY2FsbGlu ZyBkcm1fc2V0X2NydGNpbmZvIGFzIG1lbnRpb25lZCBiZWxvdzoKPgo+ICAgIGJvb2wgYW1kZ3B1 X2F0b21iaW9zX2VuY29kZXJfbW9kZV9maXh1cChzdHJ1Y3QgZHJtX2VuY29kZXIgKmVuY29kZXIs Cj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgY29uc3Qgc3RydWN0IGRybV9kaXNw bGF5X21vZGUgKm1vZGUsCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc3RydWN0 IGRybV9kaXNwbGF5X21vZGUgKmFkanVzdGVkX21vZGUpCj4gICAgewo+ICAgICAgICAgc3RydWN0 IGFtZGdwdV9lbmNvZGVyICphbWRncHVfZW5jb2RlciA9IHRvX2FtZGdwdV9lbmNvZGVyKGVuY29k ZXIpOwo+Cj4gICAgICAgICAvKiBzZXQgdGhlIGFjdGl2ZSBlbmNvZGVyIHRvIGNvbm5lY3RvciBy b3V0aW5nICovCj4gICAgICAgICBhbWRncHVfZW5jb2Rlcl9zZXRfYWN0aXZlX2RldmljZShlbmNv ZGVyKTsKPiAgICAgICAgICoqKmRybV9tb2RlX3NldF9jcnRjaW5mbyhhZGp1c3RlZF9tb2RlLCAw KTsqKioqCj4KPiAgICAgICAgIC8qIGh3IGJ1ZyAqLwo+ICAgICAgICAgaWYgKChtb2RlLT5mbGFn cyAmIERSTV9NT0RFX0ZMQUdfSU5URVJMQUNFKQo+ICAgICAgICAgICAgICYmIChtb2RlLT5jcnRj X3ZzeW5jX3N0YXJ0IDwgKG1vZGUtPmNydGNfdmRpc3BsYXkgKyAyKSkpCj4gICAgICAgICAgICAg ICAgIGFkanVzdGVkX21vZGUtPmNydGNfdnN5bmNfc3RhcnQgPSBhZGp1c3RlZF9tb2RlLT5jcnRj X3ZkaXNwbGF5ICsgMjsKPgo+ICAgVGhlbiB3ZSBtYXkgbmVlZCB0byBkZWZpbmUgbmV3IGZ1bmMg bGlrZQo+Cj4gICB2b2lkIGRybV9jYWxjX2h2X3BvcmNoZXNfc3luYygpCj4gICB7Cj4gICAgY3J0 Y19oZnJvbnRfcG9yY2ggPSBjcnRjX2hzeW5jX3N0YXJ0IC0gY3J0Y19oZGlzcGxheTsKPiAgICBj cnRjX3Zmcm9udF9wb3JjaCA9IGNydGNfdnN5bmNfc3RhcnQgLSBjcnRjX3ZkaXNwbGF5Owo+ICAg IC4KPiAgICAuCj4gICAgY3J0Y19jbG9ja19oeiA9IGNydGNfY2xvY2sqMTAwMDsKPiAgIH0gIGFu ZCBjYWxsIHRoaXMgZnVuYyBpbiBtb2RlX3NldCosIGF0b21pY19lbmFibGUgQ0JTIGJlZm9yZSBw cm9ncmFtbWluZyB0aGUgSFcgd2l0aCB0aGlzIG1vZGUuCj4KPgo+ICAgU2hvdWxkIEkgY3JlYXRl IHBhdGNoZXMgYXJvdW5kIHRoaXMgaWRlYSA/Cj4gICBQbGVhc2UgbGV0IG1lIGtub3cgeW91ciBj b21tZW50cy4KCkknbSBub3Qgc3VyZSBhYm91dCBwb2ludCA2LiBJIHRoaW5rIHdlIHNob3VsZCB3 YWl0IHdpdGggY29taW5nIHVwIHdpdGgKYSBzb2x1dGlvbiB0byB0aGlzIHByb2JsZW0gb25jZSB0 aGVyZSdzIGEgY2xlYXIgbmVlZCBmb3IgaXQuIE1vc3QKbGlrZWx5IEkgdGhpbmsgZHJpdmVycyB3 aG8gYm90aCBuZWVkIHRvIGFkanVzdCBjb21wdXRlZCB0aW1pbmdzIGFuZAp3aG8gbmVlZCB2L2hm cm9udC9iYWNrIHBvcmNoIGp1c3QgbmVlZCB0byBhZGp1c3QgZXZlcnl0aGluZyBvbiB0aGVpcgpv d24uIEFuZCB3ZSB3b24ndCBwcm92aWRlIGFueSBhZGRpdGlvbmFsIGhlbHBlcnMuCgpDaGVlcnMs IERhbmllbAotLSAKRGFuaWVsIFZldHRlcgpTb2Z0d2FyZSBFbmdpbmVlciwgSW50ZWwgQ29ycG9y YXRpb24KKzQxICgwKSA3OSAzNjUgNTcgNDggLSBodHRwOi8vYmxvZy5mZndsbC5jaApfX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGlu ZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVl ZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751790AbeEMOMW (ORCPT ); Sun, 13 May 2018 10:12:22 -0400 Received: from mail-io0-f172.google.com ([209.85.223.172]:42348 "EHLO mail-io0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751438AbeEMOMU (ORCPT ); Sun, 13 May 2018 10:12:20 -0400 X-Google-Smtp-Source: AB8JxZo8xv/zeBC3qcI3B6NtC4zL0w9RKlKUKmA8PVEEE5Xtgsucz1fxLubiECKWmF7oYHwdIK8wulE6ADeq2jQ5ni4= MIME-Version: 1.0 X-Originating-IP: [2a02:168:5635:0:39d2:f87e:2033:9f6] In-Reply-To: <1525866725-16685-1-git-send-email-satendra.t@samsung.com> References: <1525777110-11378-1-git-send-email-satendra.t@samsung.com> <1525866725-16685-1-git-send-email-satendra.t@samsung.com> From: Daniel Vetter Date: Sun, 13 May 2018 16:12:18 +0200 X-Google-Sender-Auth: JeJb8B8F7bLfUx-CBlAdaxw2Th4 Message-ID: Subject: Re: [PATCH 00/13] drm/kms/mode: using helper func drm_display_mode_to/from_videomode for calculating timing parameters To: Satendra Singh Thakur Cc: Linux Kernel Mailing List , dri-devel , linux-tegra@vger.kernel.org, Linux ARM , linux-samsung-soc , "moderated list:ARM/Mediatek SoC support" , linux-amlogic@lists.infradead.org, sst2005@gmail.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 9, 2018 at 1:52 PM, Satendra Singh Thakur wrote: > On Thu, May 08, 2018 at 16:28:30 +0530, Satendra Singh Thakur wrote: >> On Thu, May 07, 2018 at 15:46:02 +0200, Daniel Vetter wrote: >> > On Thu, May 03, 2018 at 01:53:55PM +0530, Satendra Singh Thakur wrote: >> > > 1.There is a function in drm-core to calculate display timing parameters: >> > > horizontal front porch, back porch, sync length, >> > > vertical front porch, back porch, sync length and >> > > clock in Hz. >> > > However, some drivers are still calculating these parameters themselves. >> > > Therefore, there is a duplication of the code. >> > > This patch series replaces this redundant code with the function >> > > drm_display_mode_to_videomode. >> > > This removes nearly 100 redundant lines from the related drivers. >> > > 2.For some drivers (sun4i) the reverse helper >> > > drm_display_mode_from_videomode is used. >> > > 3.For some drivers it replaces arithmatic operators (*, /) with shifting >> > > operators (>>, <<). >> > > 4.For some drivers DRM_MODE_FLAG_* are replaced with DISPLAY_FLAGS_* flags. >> > > 5.These changes apply to following crtc and encoder drivers: >> > > atmel-hlcdc >> > > bridge-tc358767 >> > > exynos-dsi >> > > fsl-dcu >> > > gma500-mdfld_dsi_dpi >> > > hisilicon-kirin_dsi, ade >> > > meson-encoder >> > > pl111-display >> > > sun4i-tv >> > > ti lcdc >> > > tegra dc >> > > mediatek dpi dsi >> > > bridge-adv7533 >> > >> > The drm_mode_to_videomode helper is meant for interop between drm and v4l, >> > which have different internal structures to represent modes. >> > >> > For drivers that only use drm I think the better option would be to add >> > these fields to struct drm_display_mode as another set of crtc_* values >> > (the computed values are stored in crtc_ prefixed members). And compute >> > front/back porch in drm_mode_set_crtcinfo. >> > >> > Then we can use these new drm_display_mode->crtc_h|vfront|back_porch >> > fields in all the drivers you're changing. This way you avoid having to >> > change all the drm drivers to use v4l #defines. >> > >> > Thanks, >> > Daniel >> >> Hi Daniel, >> Thanks for the comments. >> I will look into it. >> >> Thanks >> -Satendra > > Hi Daniel, > Thanks for the comments. > Please find below my understanding in this direction. > > 1. Currently struct videomode is being used in 50 drm drivers and 14 fbdev drivers. > Since, it's already being used by so many drm drivers, that is the reason > these fbdev objects (struct videmode and func drm_display_mode_to_videomode) were used in this patch series. The biggest contributor for that seems to be of_get_videomode. We should probably have a of_drm_get_display_mode helper, which automatically converts the of/dt video description into the drm display mode structure. And I found way less than 50 drm drivers using videomode, much less if we ignore of. > 2. Anyway, if fbdev related objects (struct/func) are not encouraged in drm drivers, then we may add > h/v front/back porches in struct drm_display_mode as adviced by you. > > 3. We can calculate these params in func drm_mode_set_crtcinfo at the end of it. > int drm_mode_set_crtcinfo () > { > . > . > crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay; > crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay; > . > . > crtc_clock_hz = crtc_clock*1000; > }; > > 4. Normally mode is programmed in HW in following callbacks of crtc and encoder drivers > -mode_set > -mode_set_nofb > -atomic_enable > > > Normally drm_mode_set_crtcinfo is used in > -mode_fixup callbacks (CBs) > of encoder and crtc drivers. > > if mode_fixup CBs are called before mode_set CBs then > drm_mode_set_crtcinfo is right place to calculate sync/porch params. > We can use crtc_hfront/back_porches directly and program them to HW > in above mentioned callbacks. > > int my_mode_set () > { > REG_WRITE(crtc_hfront_porch); > REG_WRITE(crtc_hback_porch); > . > . > } Agreed with your plan up to point 5 here. > 6. However, if these params are being modified after calling drm_set_crtcinfo as mentioned below: > > bool amdgpu_atombios_encoder_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode) > { > struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder); > > /* set the active encoder to connector routing */ > amdgpu_encoder_set_active_device(encoder); > ***drm_mode_set_crtcinfo(adjusted_mode, 0);**** > > /* hw bug */ > if ((mode->flags & DRM_MODE_FLAG_INTERLACE) > && (mode->crtc_vsync_start < (mode->crtc_vdisplay + 2))) > adjusted_mode->crtc_vsync_start = adjusted_mode->crtc_vdisplay + 2; > > Then we may need to define new func like > > void drm_calc_hv_porches_sync() > { > crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay; > crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay; > . > . > crtc_clock_hz = crtc_clock*1000; > } and call this func in mode_set*, atomic_enable CBS before programming the HW with this mode. > > > Should I create patches around this idea ? > Please let me know your comments. I'm not sure about point 6. I think we should wait with coming up with a solution to this problem once there's a clear need for it. Most likely I think drivers who both need to adjust computed timings and who need v/hfront/back porch just need to adjust everything on their own. And we won't provide any additional helpers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Sun, 13 May 2018 16:12:18 +0200 Subject: [PATCH 00/13] drm/kms/mode: using helper func drm_display_mode_to/from_videomode for calculating timing parameters In-Reply-To: <1525866725-16685-1-git-send-email-satendra.t@samsung.com> References: <1525777110-11378-1-git-send-email-satendra.t@samsung.com> <1525866725-16685-1-git-send-email-satendra.t@samsung.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, May 9, 2018 at 1:52 PM, Satendra Singh Thakur wrote: > On Thu, May 08, 2018 at 16:28:30 +0530, Satendra Singh Thakur wrote: >> On Thu, May 07, 2018 at 15:46:02 +0200, Daniel Vetter wrote: >> > On Thu, May 03, 2018 at 01:53:55PM +0530, Satendra Singh Thakur wrote: >> > > 1.There is a function in drm-core to calculate display timing parameters: >> > > horizontal front porch, back porch, sync length, >> > > vertical front porch, back porch, sync length and >> > > clock in Hz. >> > > However, some drivers are still calculating these parameters themselves. >> > > Therefore, there is a duplication of the code. >> > > This patch series replaces this redundant code with the function >> > > drm_display_mode_to_videomode. >> > > This removes nearly 100 redundant lines from the related drivers. >> > > 2.For some drivers (sun4i) the reverse helper >> > > drm_display_mode_from_videomode is used. >> > > 3.For some drivers it replaces arithmatic operators (*, /) with shifting >> > > operators (>>, <<). >> > > 4.For some drivers DRM_MODE_FLAG_* are replaced with DISPLAY_FLAGS_* flags. >> > > 5.These changes apply to following crtc and encoder drivers: >> > > atmel-hlcdc >> > > bridge-tc358767 >> > > exynos-dsi >> > > fsl-dcu >> > > gma500-mdfld_dsi_dpi >> > > hisilicon-kirin_dsi, ade >> > > meson-encoder >> > > pl111-display >> > > sun4i-tv >> > > ti lcdc >> > > tegra dc >> > > mediatek dpi dsi >> > > bridge-adv7533 >> > >> > The drm_mode_to_videomode helper is meant for interop between drm and v4l, >> > which have different internal structures to represent modes. >> > >> > For drivers that only use drm I think the better option would be to add >> > these fields to struct drm_display_mode as another set of crtc_* values >> > (the computed values are stored in crtc_ prefixed members). And compute >> > front/back porch in drm_mode_set_crtcinfo. >> > >> > Then we can use these new drm_display_mode->crtc_h|vfront|back_porch >> > fields in all the drivers you're changing. This way you avoid having to >> > change all the drm drivers to use v4l #defines. >> > >> > Thanks, >> > Daniel >> >> Hi Daniel, >> Thanks for the comments. >> I will look into it. >> >> Thanks >> -Satendra > > Hi Daniel, > Thanks for the comments. > Please find below my understanding in this direction. > > 1. Currently struct videomode is being used in 50 drm drivers and 14 fbdev drivers. > Since, it's already being used by so many drm drivers, that is the reason > these fbdev objects (struct videmode and func drm_display_mode_to_videomode) were used in this patch series. The biggest contributor for that seems to be of_get_videomode. We should probably have a of_drm_get_display_mode helper, which automatically converts the of/dt video description into the drm display mode structure. And I found way less than 50 drm drivers using videomode, much less if we ignore of. > 2. Anyway, if fbdev related objects (struct/func) are not encouraged in drm drivers, then we may add > h/v front/back porches in struct drm_display_mode as adviced by you. > > 3. We can calculate these params in func drm_mode_set_crtcinfo at the end of it. > int drm_mode_set_crtcinfo () > { > . > . > crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay; > crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay; > . > . > crtc_clock_hz = crtc_clock*1000; > }; > > 4. Normally mode is programmed in HW in following callbacks of crtc and encoder drivers > -mode_set > -mode_set_nofb > -atomic_enable > > > Normally drm_mode_set_crtcinfo is used in > -mode_fixup callbacks (CBs) > of encoder and crtc drivers. > > if mode_fixup CBs are called before mode_set CBs then > drm_mode_set_crtcinfo is right place to calculate sync/porch params. > We can use crtc_hfront/back_porches directly and program them to HW > in above mentioned callbacks. > > int my_mode_set () > { > REG_WRITE(crtc_hfront_porch); > REG_WRITE(crtc_hback_porch); > . > . > } Agreed with your plan up to point 5 here. > 6. However, if these params are being modified after calling drm_set_crtcinfo as mentioned below: > > bool amdgpu_atombios_encoder_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode) > { > struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder); > > /* set the active encoder to connector routing */ > amdgpu_encoder_set_active_device(encoder); > ***drm_mode_set_crtcinfo(adjusted_mode, 0);**** > > /* hw bug */ > if ((mode->flags & DRM_MODE_FLAG_INTERLACE) > && (mode->crtc_vsync_start < (mode->crtc_vdisplay + 2))) > adjusted_mode->crtc_vsync_start = adjusted_mode->crtc_vdisplay + 2; > > Then we may need to define new func like > > void drm_calc_hv_porches_sync() > { > crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay; > crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay; > . > . > crtc_clock_hz = crtc_clock*1000; > } and call this func in mode_set*, atomic_enable CBS before programming the HW with this mode. > > > Should I create patches around this idea ? > Please let me know your comments. I'm not sure about point 6. I think we should wait with coming up with a solution to this problem once there's a clear need for it. Most likely I think drivers who both need to adjust computed timings and who need v/hfront/back porch just need to adjust everything on their own. And we won't provide any additional helpers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Sun, 13 May 2018 16:12:18 +0200 Subject: [PATCH 00/13] drm/kms/mode: using helper func drm_display_mode_to/from_videomode for calculating timing parameters In-Reply-To: <1525866725-16685-1-git-send-email-satendra.t@samsung.com> References: <1525777110-11378-1-git-send-email-satendra.t@samsung.com> <1525866725-16685-1-git-send-email-satendra.t@samsung.com> Message-ID: To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On Wed, May 9, 2018 at 1:52 PM, Satendra Singh Thakur wrote: > On Thu, May 08, 2018 at 16:28:30 +0530, Satendra Singh Thakur wrote: >> On Thu, May 07, 2018 at 15:46:02 +0200, Daniel Vetter wrote: >> > On Thu, May 03, 2018 at 01:53:55PM +0530, Satendra Singh Thakur wrote: >> > > 1.There is a function in drm-core to calculate display timing parameters: >> > > horizontal front porch, back porch, sync length, >> > > vertical front porch, back porch, sync length and >> > > clock in Hz. >> > > However, some drivers are still calculating these parameters themselves. >> > > Therefore, there is a duplication of the code. >> > > This patch series replaces this redundant code with the function >> > > drm_display_mode_to_videomode. >> > > This removes nearly 100 redundant lines from the related drivers. >> > > 2.For some drivers (sun4i) the reverse helper >> > > drm_display_mode_from_videomode is used. >> > > 3.For some drivers it replaces arithmatic operators (*, /) with shifting >> > > operators (>>, <<). >> > > 4.For some drivers DRM_MODE_FLAG_* are replaced with DISPLAY_FLAGS_* flags. >> > > 5.These changes apply to following crtc and encoder drivers: >> > > atmel-hlcdc >> > > bridge-tc358767 >> > > exynos-dsi >> > > fsl-dcu >> > > gma500-mdfld_dsi_dpi >> > > hisilicon-kirin_dsi, ade >> > > meson-encoder >> > > pl111-display >> > > sun4i-tv >> > > ti lcdc >> > > tegra dc >> > > mediatek dpi dsi >> > > bridge-adv7533 >> > >> > The drm_mode_to_videomode helper is meant for interop between drm and v4l, >> > which have different internal structures to represent modes. >> > >> > For drivers that only use drm I think the better option would be to add >> > these fields to struct drm_display_mode as another set of crtc_* values >> > (the computed values are stored in crtc_ prefixed members). And compute >> > front/back porch in drm_mode_set_crtcinfo. >> > >> > Then we can use these new drm_display_mode->crtc_h|vfront|back_porch >> > fields in all the drivers you're changing. This way you avoid having to >> > change all the drm drivers to use v4l #defines. >> > >> > Thanks, >> > Daniel >> >> Hi Daniel, >> Thanks for the comments. >> I will look into it. >> >> Thanks >> -Satendra > > Hi Daniel, > Thanks for the comments. > Please find below my understanding in this direction. > > 1. Currently struct videomode is being used in 50 drm drivers and 14 fbdev drivers. > Since, it's already being used by so many drm drivers, that is the reason > these fbdev objects (struct videmode and func drm_display_mode_to_videomode) were used in this patch series. The biggest contributor for that seems to be of_get_videomode. We should probably have a of_drm_get_display_mode helper, which automatically converts the of/dt video description into the drm display mode structure. And I found way less than 50 drm drivers using videomode, much less if we ignore of. > 2. Anyway, if fbdev related objects (struct/func) are not encouraged in drm drivers, then we may add > h/v front/back porches in struct drm_display_mode as adviced by you. > > 3. We can calculate these params in func drm_mode_set_crtcinfo at the end of it. > int drm_mode_set_crtcinfo () > { > . > . > crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay; > crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay; > . > . > crtc_clock_hz = crtc_clock*1000; > }; > > 4. Normally mode is programmed in HW in following callbacks of crtc and encoder drivers > -mode_set > -mode_set_nofb > -atomic_enable > > > Normally drm_mode_set_crtcinfo is used in > -mode_fixup callbacks (CBs) > of encoder and crtc drivers. > > if mode_fixup CBs are called before mode_set CBs then > drm_mode_set_crtcinfo is right place to calculate sync/porch params. > We can use crtc_hfront/back_porches directly and program them to HW > in above mentioned callbacks. > > int my_mode_set () > { > REG_WRITE(crtc_hfront_porch); > REG_WRITE(crtc_hback_porch); > . > . > } Agreed with your plan up to point 5 here. > 6. However, if these params are being modified after calling drm_set_crtcinfo as mentioned below: > > bool amdgpu_atombios_encoder_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode) > { > struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder); > > /* set the active encoder to connector routing */ > amdgpu_encoder_set_active_device(encoder); > ***drm_mode_set_crtcinfo(adjusted_mode, 0);**** > > /* hw bug */ > if ((mode->flags & DRM_MODE_FLAG_INTERLACE) > && (mode->crtc_vsync_start < (mode->crtc_vdisplay + 2))) > adjusted_mode->crtc_vsync_start = adjusted_mode->crtc_vdisplay + 2; > > Then we may need to define new func like > > void drm_calc_hv_porches_sync() > { > crtc_hfront_porch = crtc_hsync_start - crtc_hdisplay; > crtc_vfront_porch = crtc_vsync_start - crtc_vdisplay; > . > . > crtc_clock_hz = crtc_clock*1000; > } and call this func in mode_set*, atomic_enable CBS before programming the HW with this mode. > > > Should I create patches around this idea ? > Please let me know your comments. I'm not sure about point 6. I think we should wait with coming up with a solution to this problem once there's a clear need for it. Most likely I think drivers who both need to adjust computed timings and who need v/hfront/back porch just need to adjust everything on their own. And we won't provide any additional helpers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch