From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xinwei Kong Subject: Re: [PATCH RFC 0/8] Add New DRM Driver for Hisilicon's Hi6220 SoC Date: Fri, 18 Sep 2015 18:32:32 +0800 Message-ID: <55FBE840.2040406@hisilicon.com> References: <1442309834-21420-1-git-send-email-kong.kongxinwei@hisilicon.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [119.145.14.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id AE7C66E13D for ; Fri, 18 Sep 2015 03:35:57 -0700 (PDT) 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: Daniel Stone Cc: yinshengbao@hisilicon.com, xuyiping@hisilicon.com, Catalin Marinas , Will Deacon , yanhaifeng@hisilicon.com, dri-devel , Linux Kernel Mailing List , andy.green@linaro.org, haojian.zhuang@linaro.org, fangdechun@hisilicon.com, puck.chen@hisilicon.com, ml.yang@hisilicon.com, gongyu@hisilicon.com, xuwei5@hisilicon.com, liguozhu@hisilicon.com, qijiwen@hisilicon.com, james.yanglong@hisilicon.com List-Id: dri-devel@lists.freedesktop.org aGkgRGFuaWVsIFN0b25lOgoKT24gMjAxNS85LzE2IDIzOjIzLCBEYW5pZWwgU3RvbmUgd3JvdGU6 Cj4gSGkgWGlud2VpLAo+IFRoYW5rcyBmb3IgdGhpcyBjb250cmlidXRpb24hIFdlIGxvb2sgZm9y d2FyZCB0byBzZWVpbmcgc3VwcG9ydCBmb3IKPiB0aGVzZSBkZXZpY2VzLgo+IAo+IFRoaXMgaXNu J3QgYW4gZXhoYXVzdGl2ZSByZXZpZXcsIGJ1dCB0d28gdmVyeSBoaWdoLWxldmVsIGNvbW1lbnRz Cj4gd2hpY2ggc2hvdWxkIHJlc3VsdCBpbiBhIGxvdCBvZiBjaGFuZ2VzIC4uLgo+IAo+IE9uIDE1 IFNlcHRlbWJlciAyMDE1IGF0IDEwOjM3LCBYaW53ZWkgS29uZwo+IDxrb25nLmtvbmd4aW53ZWlA aGlzaWxpY29uLmNvbT4gd3JvdGU6Cj4+IDEuIEhhcmR3YXJlIERldGFpbAo+PiAgIFRoZSBkaXNw bGF5IHN1YnN5c3RlbSBvZiBIaTYyMjAgU29DIGlzIHNob3duIGFzIGJlbGxvdzoKPj4gICstLS0t LSsgICAgICAgKy0tLS0tLS0tLS0rICAgICArLS0tLS0rICAgICArLS0tLS0tLS0tKwo+PiAgfCAg ICAgfCAgICAgICB8ICAgICAgICAgIHwgICAgIHwgICAgIHwgICAgIHwgICAgICAgICB8Cj4+ICB8 IEZCICB8LS0tLS0tPnwgICBBREUgICAgfC0tLS0+fCBEU0kgfC0tLS0+fCBFeHRlcm5hbHwKPj4g IHwgICAgIHwgICAgICAgfCAgICAgICAgICB8ICAgICB8ICAgICB8ICAgICB8ICBIRE1JICAgfAo+ PiAgKy0tLS0tKyAgICAgICArLS0tLS0tLS0tLSsgICAgICstLS0tLSsgICAgICstLS0tLS0tLS0r Cj4+Cj4+IC0gQURFKEFkdmFuY2VkIERpc3BsYXkgRW5naW5lKSBpcyB0aGUgZGlzcGxheSBjb250 cm9sbGVyLiBJdCBjb250YWlucyA3Cj4+IGNoYW5uZWxzIG9yIHBpcGVzLCAzIG92ZXJsYXkgYW5k IGEgTERJLgo+PiAgIC0gQSBjaGFubmVsL3BpcGUgbG9va3MgbGlrZTogRE1BLS0+Y2xpcC0tPnNj YWxlLS0+Y3RyYW5zL2NzYy4KPj4gICAtIE92ZXJsYXkgaXMgcmVzcG9uc2UgdG8gY29tcG9zZSBw bGFuZXMgd2hpY2ggY29tZSBmcm9tIDcgY2hhbm5lbHMgYW5kCj4+ICAgcGFzcyBjb21wb3NlZCBp bWFnZSB0byBMREkuCj4gCj4gVGhpcyB0ZXJtaW5vbG9neSBpcyBiYWNrd2FyZHMgZnJvbSB3aGF0 IHdlIHVzdWFsbHkgdXNlIGluIERSTSwgd2hlcmUKPiBhbiBvdmVybGF5IGlzIGEgc3BlY2lhbCBj YXNlIG9mIERSTSBwbGFuZXMsIGFuZCBwaXBlcyBhcmUgRFJNIENSVENzLgo+IAo+PiAgIC0gTERJ IGlzIHJlc3BvbnNlIHRvIGdlbmVyYXRlIHRpbWluZ3MgYW5kIFJHQiBkYXRhIHN0cmVhbS4KPj4g LSBEU0kgY29udmVydHMgdGhlIFJHQiBkYXRhIHN0cmVhbSBmcm9tIEFERSB0byBEU0kgcGFja2V0 cy4KPj4gLSBFeHRlcm5hbCBIRE1JIG1vZHVsZSBpcyBjb25uZWN0ZWQgd2l0aCBEU0kgYnVzLiBO b3cgSGlrZXkgdXNlIGEgQURJJ3MKPj4gICBBRFY3NTMzIGV4dGVybmFsIEhETUkgY2hpcC4KPiAK PiBTbyB0aGlzIGlzIGJhc2ljYWxseSBqdXN0IGFuIGltcGxlbWVudGF0aW9uIGRldGFpbCBvZiBE U0k/Cj4gCj4+IDIuIFNvZnR3YXJlIERldGFpbAo+PiAgIEFib3V0IHRoZSBzb2Z0d2FyZSBvcmdh bml6YXRpb24gYW5kIGltcGxlbWVudGF0aW9uIGRldGFpbDoKPj4gV2UgaGF2ZSBhIG1haW4gZHJt IHBsYXRmb3JtIGRyaXZlciAoaGlzaV9kcm1fZHJ2LmMpLCBhZGUgcGxhdGZvcm0gZHJpdmVyCj4+ IChoaXNpX2FkZS5jKSBhbmQgYSBkc2kgcGxhdGZvcm0gZHJpdmVyIChoaXNpX2RybV9kc2kuYyku IEFkZSBkcml2ZXIKPj4gaW1wbGVtZW50cyB0aGUgcGxhbmUgYW5kIGNydGMgZHJpdmVyIGludGVy ZmFjZXMgYW5kIGRzaSBpbXBsZW1lbnRzIHRoZQo+PiBlbmNvZGVyIGFuZCBjb25uZWN0b3IgZHJp dmVyIGludGVyZmFjZXMuIFdlIHRha2UgYWR2YW50YWdlIG9mIGNvbXBvbmVudAo+PiBmcmFtZXdv cmsgdG8gaW5pdGlhbGl6ZSBlYWNoIGRyaXZlci4KPj4gICBJbiBvcmRlciB0byBzdXBwb3J0IG11 bHRpIGNvbWluZyBIaXNpbGljb24ncyBTb0NzLCB3ZSBwbGFuIHRvIHNlcGFyYXRlCj4+IGNvbW1v biBkcml2ZXIgY29kZSBhbmQgU29DIHNwZWNpZmljIGltcGxlbWVudGVkIGNvZGUgYXMgcG9zc2lw bGUgYXMgd2UgY2FuLgo+PiBXZSBhYnN0cmFjdCBhbiBvcHMgZm9yIGVhY2ggY29tcG9uZW50KGNy dGMsIHBsYW5lLCBlbmNvZGVyIGFuZCBjb25uZWN0b3IpCj4+IHRvIHJldXNlIHRoZSBjb21tb24g aW50ZXJmYWNlIGltcGxlbWVudGF0aW9uIGxvZ2ljIChGSVhNRTogTm90IHN1cmUgaWYgd2UKPj4g Y2FuIGFjaGlldmUgdGhpcyB0YXJnZXQgYW5kIGlmIGl0IGlzIGdvb2Qgb3Igbm90KS4gVGh1cywg d2UgcHV0IHRoZXNlCj4+IGNvbW1vbiBkcml2ZXIgY29kZSBpbnRvIGhpc2lfZHJtX2Rydi9jcnRj L3BsYW5lL2VuY29kZXIvY29ubmVjdG9yLmMgZmlsZXMuCj4gCj4gUGxlYXNlIGRvIG5vdCBkbyB0 aGlzOyBpbiBnZW5lcmFsLCB0aGUgYWJzdHJhY3Rpb24gbGF5ZXJzIGNhdXNlIG1vcmUKPiBwcm9i bGVtcyB0aGFuIHRoZXkgY3JlYXRlLiBXZSBoYXZlIG9ubHkganVzdCBmaW5pc2hlZCByZW1vdmlu ZyBhbGwgdGhlCj4gYWJzdHJhY3Rpb24gbGF5ZXJzIGZyb20gZHJpdmVycy9ncHUvZHJtL2V4eW5v cy8sIHdoaWNoIHN0YXJ0ZWQgb2ZmCj4gd2l0aCBleGFjdGx5IHRoZSBzYW1lIGlkZWEsIGJ1dCBv bmx5IGNyZWF0ZWQgcHJvYmxlbXMuIFRoZSBpc3N1ZSBpcwo+IHRoYXQgZXZlcnkgdGltZSB0aGUg RFJNIGNvcmUgaW50ZXJmYWNlIGNoYW5nZXMsIHlvdSBoYXZlIHRvIG1ha2UgdGhlCj4gZXhhY3Qg c2FtZSBjaGFuZ2VzIGluIHlvdXIgY29waWVzIG9mIHRoZSBpbnRlcmZhY2UuIEluIGdlbmVyYWws IHRoZXJlCj4gc2VlbXMgdG8gYmUgbm8gYmVuZWZpdCB0byBoYXZpbmcgdGhlc2UgaGVyZTogeW91 IGNhbiBqdXN0IGFzc2lnbiB0aGUKPiBEUk0gZnVuY3Rpb25zIGRpcmVjdGx5IGFjY29yZGluZyB0 byBnZW5lcmF0aW9uLiBTZWUgY3VycmVudCBFeHlub3MgZm9yCj4gYW4gZXhhbXBsZSBvZiB0aGlz Lgo+IApJIHVuZGVyc3RhbmQgdGhhdCB5b3Ugd2FudCB0byBsZXQgdXMgcmVtb3ZlIHRoZSBoaXNp X2RybV9jcnRjL3BsYW5lLwplbmNvZGVyL2Nvbm5lY3Rvci5jIGZpbGVzIGluIG15IGRyaXZlci4K CldoZW4gd2UgcGxhbiB0byB1c2UgYWJzdHJhY3Rpb24gbGF5ZXJzLCBvdXIgcHVycG9zZSBpcyB0 aGF0IG91ciBjb21tbW9uCmRybSBpbnRlcmZhY2Ugd2lsbCBiZSB1c2VkIGluIGRpZmYgaGlzaWxp Y29uIHNvYyBwbGF0Zm9ybSBzdWNoIGFzIG1vYmlsZQpzZXJpZXPjgIFUViBzZXJpZXMgc29jICwg aWYgdGhpcyBjb21tb24gaW50ZXJmYWNlIGRvbid0IHVzZSBpbiBkaWZmIHNvYwpvciBub3QgdGFr ZSBhZHZhbnRhZ2Ugb2YgZm9sbG93aW5nIERSTSBjb3JlIGludGVyZmFjZSBjaGFuZ2VzLCB3ZSB3 aWxsIGZpeCBpdC4KCmJ1dCBpZiBJIHdpbGwgcG9ydCBoaXNpX2RybV9jcnRjL3BsYW5lLy5jIGZp bGUgY29kZSBpbnRvIGhpc2lfYWRlLmMgZmlsZQphbmQgcG9ydCBoaXNpX2RybV9lbmNvZGVyL2Nv bm5lY3Rvci5jIGludG8gaGlzaV9kcm1fZHNpLmMgZmlsZSwKd2hlbiB3ZSBhZGQgc29tZSBvdGhl ciBzb2MgcGxhdGZvcm0sIHdlIHdpbGwgYmUgc2ltaWxhciB0byBjcmVhdGUgaGFyZHdhcmUKaXAg Ki5jIGZpbGUsIGlmIERSTSBjb3JlIGludGVyZmFjZSB3aWxsIGNoYW5nZXMgLCB3ZSB3aWxsIGNo YW5nZSBhbGwgcmVmZXJpbmcKaXAuYyBmaWxlLgoKSSB3aXNoIHRoYXQgeW91IGNhbiBnaXZlIG1l IHNvbWUgZ3VpZGVzIGZvciB0aGlzIGFic3RyYWN0aW9uIGxheWVycy4KClRoYW5rIHlvdQp4aW53 ZWkKCgo+IFRoZSBiaWdnZXN0IGlzc3VlIHRob3VnaCwgaXMgdGhhdCB0aGlzIGRyaXZlciBzaG91 bGQgYmVjb21lIGFuIGF0b21pYwo+IG1vZGVzZXR0aW5nIGRyaXZlci4gQXRvbWljIG1vZGVzZXR0 aW5nLCByYXRoZXIgdGhhbiBzZW5kaW5nIHNtYWxsCj4gaW5kaXZpZHVhbCBjb21tYW5kcyAoZW5h YmxlIENSVEMsIGNoYW5nZSBwbGFuZSBwb3NpdGlvbiwgZXRjKSBpcyBiYXNlZAo+IG9uIHZhbGlk YXRpbmcgYW5kIHBhc3NpbmcgYXJvdW5kIGNvbXBsZXRlIHNldHMgb2YgaGFyZHdhcmUgc3RhdGUu Cj4gRGFuaWVsIFZldHRlcidzIGJsb2cgaGFzIGFuIGFydGljbGUgb24gaG93IHRvIGNvbnZlcnQg eW91ciBkcml2ZXI6Cj4gaHR0cDovL2Jsb2cuZmZ3bGwuY2gvMjAxNC8xMS9hdG9taWMtbW9kZXNl dC1zdXBwb3J0LWZvci1rbXMtZHJpdmVycy5odG1sCj4gCj4gSW4gYWRkaXRpb24sIHRoZXJlIGFy ZSBzb21lIGRyaXZlcnMgY29udmVydGVkIGFscmVhZHkgdGhhdCB5b3UgY2FuCj4gbG9vayBhdDog dGVncmEgKHZlcnkgc2ltcGxlKSwgZXh5bm9zIChyZWFzb25hYmx5IHNpbXBsZSksIGZzbC1kY3UK PiAobW9kZXJhdGUpLCBtc20gKHF1aXRlIGNvbXBsZXgpLCBpOTE1IChpbmNyZWRpYmx5IGNvbXBs ZXgpLCByY2FyLWR1Cj4gKD8/PykuCj4gCj4gT25jZSB5b3VyIGRyaXZlciBpcyBjb252ZXJ0ZWQg dG8gYXRvbWljIGFuZCB0aGUgYWJzdHJhY3Rpb24gbGF5ZXJzCj4gcmVtb3ZlZCwgdGhlbiBpdCB3 aWxsIGJlIG11Y2ggZWFzaWVyIHRvIHJldmlldyB0aGUgc3VibWlzc2lvbiBpbgo+IGRldGFpbC4K PiAKPiBUaGFua3MgdmVyeSBtdWNoIQo+IAo+IENoZWVycywKPiBEYW5pZWwKPiAKPiAuCj4gCgpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwg bWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753446AbbIRKl3 (ORCPT ); Fri, 18 Sep 2015 06:41:29 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:34323 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752400AbbIRKl1 (ORCPT ); Fri, 18 Sep 2015 06:41:27 -0400 Message-ID: <55FBE840.2040406@hisilicon.com> Date: Fri, 18 Sep 2015 18:32:32 +0800 From: Xinwei Kong User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Daniel Stone CC: David Airlie , Catalin Marinas , Will Deacon , dri-devel , , , , , , , , , , , , Linux Kernel Mailing List , , Subject: Re: [PATCH RFC 0/8] Add New DRM Driver for Hisilicon's Hi6220 SoC References: <1442309834-21420-1-git-send-email-kong.kongxinwei@hisilicon.com> In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.46.22.103] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org hi Daniel Stone: On 2015/9/16 23:23, Daniel Stone wrote: > Hi Xinwei, > Thanks for this contribution! We look forward to seeing support for > these devices. > > This isn't an exhaustive review, but two very high-level comments > which should result in a lot of changes ... > > On 15 September 2015 at 10:37, Xinwei Kong > wrote: >> 1. Hardware Detail >> The display subsystem of Hi6220 SoC is shown as bellow: >> +-----+ +----------+ +-----+ +---------+ >> | | | | | | | | >> | FB |------>| ADE |---->| DSI |---->| External| >> | | | | | | | HDMI | >> +-----+ +----------+ +-----+ +---------+ >> >> - ADE(Advanced Display Engine) is the display controller. It contains 7 >> channels or pipes, 3 overlay and a LDI. >> - A channel/pipe looks like: DMA-->clip-->scale-->ctrans/csc. >> - Overlay is response to compose planes which come from 7 channels and >> pass composed image to LDI. > > This terminology is backwards from what we usually use in DRM, where > an overlay is a special case of DRM planes, and pipes are DRM CRTCs. > >> - LDI is response to generate timings and RGB data stream. >> - DSI converts the RGB data stream from ADE to DSI packets. >> - External HDMI module is connected with DSI bus. Now Hikey use a ADI's >> ADV7533 external HDMI chip. > > So this is basically just an implementation detail of DSI? > >> 2. Software Detail >> About the software organization and implementation detail: >> We have a main drm platform driver (hisi_drm_drv.c), ade platform driver >> (hisi_ade.c) and a dsi platform driver (hisi_drm_dsi.c). Ade driver >> implements the plane and crtc driver interfaces and dsi implements the >> encoder and connector driver interfaces. We take advantage of component >> framework to initialize each driver. >> In order to support multi coming Hisilicon's SoCs, we plan to separate >> common driver code and SoC specific implemented code as possiple as we can. >> We abstract an ops for each component(crtc, plane, encoder and connector) >> to reuse the common interface implementation logic (FIXME: Not sure if we >> can achieve this target and if it is good or not). Thus, we put these >> common driver code into hisi_drm_drv/crtc/plane/encoder/connector.c files. > > Please do not do this; in general, the abstraction layers cause more > problems than they create. We have only just finished removing all the > abstraction layers from drivers/gpu/drm/exynos/, which started off > with exactly the same idea, but only created problems. The issue is > that every time the DRM core interface changes, you have to make the > exact same changes in your copies of the interface. In general, there > seems to be no benefit to having these here: you can just assign the > DRM functions directly according to generation. See current Exynos for > an example of this. > I understand that you want to let us remove the hisi_drm_crtc/plane/ encoder/connector.c files in my driver. When we plan to use abstraction layers, our purpose is that our commmon drm interface will be used in diff hisilicon soc platform such as mobile series、TV series soc , if this common interface don't use in diff soc or not take advantage of following DRM core interface changes, we will fix it. but if I will port hisi_drm_crtc/plane/.c file code into hisi_ade.c file and port hisi_drm_encoder/connector.c into hisi_drm_dsi.c file, when we add some other soc platform, we will be similar to create hardware ip *.c file, if DRM core interface will changes , we will change all refering ip.c file. I wish that you can give me some guides for this abstraction layers. Thank you xinwei > The biggest issue though, is that this driver should become an atomic > modesetting driver. Atomic modesetting, rather than sending small > individual commands (enable CRTC, change plane position, etc) is based > on validating and passing around complete sets of hardware state. > Daniel Vetter's blog has an article on how to convert your driver: > http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html > > In addition, there are some drivers converted already that you can > look at: tegra (very simple), exynos (reasonably simple), fsl-dcu > (moderate), msm (quite complex), i915 (incredibly complex), rcar-du > (???). > > Once your driver is converted to atomic and the abstraction layers > removed, then it will be much easier to review the submission in > detail. > > Thanks very much! > > Cheers, > Daniel > > . >