From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756306AbcK2Iuh (ORCPT ); Tue, 29 Nov 2016 03:50:37 -0500 Received: from mail-wj0-f194.google.com ([209.85.210.194]:34598 "EHLO mail-wj0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753904AbcK2Iua (ORCPT ); Tue, 29 Nov 2016 03:50:30 -0500 Date: Tue, 29 Nov 2016 09:50:24 +0100 From: Daniel Vetter To: Neil Armstrong Cc: airlied@linux.ie, khilman@baylibre.com, carlo@caione.org, Xing.Xu@amlogic.com, victor.wan@amlogic.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, jerry.cao@amlogic.com, linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller Message-ID: <20161129085024.tqf5takulvv7f34x@phenom.ffwll.local> Mail-Followup-To: Neil Armstrong , airlied@linux.ie, khilman@baylibre.com, carlo@caione.org, Xing.Xu@amlogic.com, victor.wan@amlogic.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, jerry.cao@amlogic.com, linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org References: <1480089791-12517-1-git-send-email-narmstrong@baylibre.com> <1480089791-12517-2-git-send-email-narmstrong@baylibre.com> <20161128081601.wapare3pyzmzghvp@phenom.ffwll.local> <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> X-Operating-System: Linux phenom 4.8.0-1-amd64 User-Agent: NeoMutt/20161104 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Neil, On Mon, Nov 28, 2016 at 10:34:58AM +0100, Neil Armstrong wrote: > On 11/28/2016 09:16 AM, Daniel Vetter wrote: > > On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote: > >> +static void meson_cvbs_encoder_disable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder); > >> + > >> + meson_venci_cvbs_disable(meson_cvbs->priv); > >> +} > >> + > >> +static void meson_cvbs_encoder_enable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder); > >> + > >> + meson_venci_cvbs_enable(meson_cvbs->priv); > >> +} > > > > Personally I'd remove the indirection above, more direct code is easier to > > read. > > I understand, I'll maybe change the meson_venci_cvbs_XXable to be > directly added to the ops. > > I want to keep the registers setup in separate files and keep a clean > DRM/HW separation. I figured this is worth clarifying, and I'm somewhat guessing at your motivation here for a clean drm/hw split. There's of course various levels of how much you can split the drm side from your hw backend, but in general that design approach is really unpopular with upstream. It goes by the name of "midlayer", and the trouble with it is that it makes subsystem refactoring more complicated. For the driver itself it's nice, because it isolates you a bit from drm core. But that exact isolation is the problem when someone wants (or more often, needs to) refactor something across the entire subsystem. Then all these driver-private little (or sometimes much bigger) abstractions get in the way. That's way I suggested to remove it (both here and in the plane code), because for upstream the overall subsystem matters more than each individual driver. GPUs change fast, we need to be able to adapt fast, too. Anyway you're driver's pretty small, so personally I don't mind much. I'd still think removing the indirection would be better though. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Tue, 29 Nov 2016 09:50:24 +0100 Subject: [RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller In-Reply-To: <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> References: <1480089791-12517-1-git-send-email-narmstrong@baylibre.com> <1480089791-12517-2-git-send-email-narmstrong@baylibre.com> <20161128081601.wapare3pyzmzghvp@phenom.ffwll.local> <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> Message-ID: <20161129085024.tqf5takulvv7f34x@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Neil, On Mon, Nov 28, 2016 at 10:34:58AM +0100, Neil Armstrong wrote: > On 11/28/2016 09:16 AM, Daniel Vetter wrote: > > On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote: > >> +static void meson_cvbs_encoder_disable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder); > >> + > >> + meson_venci_cvbs_disable(meson_cvbs->priv); > >> +} > >> + > >> +static void meson_cvbs_encoder_enable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder); > >> + > >> + meson_venci_cvbs_enable(meson_cvbs->priv); > >> +} > > > > Personally I'd remove the indirection above, more direct code is easier to > > read. > > I understand, I'll maybe change the meson_venci_cvbs_XXable to be > directly added to the ops. > > I want to keep the registers setup in separate files and keep a clean > DRM/HW separation. I figured this is worth clarifying, and I'm somewhat guessing at your motivation here for a clean drm/hw split. There's of course various levels of how much you can split the drm side from your hw backend, but in general that design approach is really unpopular with upstream. It goes by the name of "midlayer", and the trouble with it is that it makes subsystem refactoring more complicated. For the driver itself it's nice, because it isolates you a bit from drm core. But that exact isolation is the problem when someone wants (or more often, needs to) refactor something across the entire subsystem. Then all these driver-private little (or sometimes much bigger) abstractions get in the way. That's way I suggested to remove it (both here and in the plane code), because for upstream the overall subsystem matters more than each individual driver. GPUs change fast, we need to be able to adapt fast, too. Anyway you're driver's pretty small, so personally I don't mind much. I'd still think removing the indirection would be better though. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller Date: Tue, 29 Nov 2016 09:50:24 +0100 Message-ID: <20161129085024.tqf5takulvv7f34x@phenom.ffwll.local> References: <1480089791-12517-1-git-send-email-narmstrong@baylibre.com> <1480089791-12517-2-git-send-email-narmstrong@baylibre.com> <20161128081601.wapare3pyzmzghvp@phenom.ffwll.local> <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wj0-x244.google.com (mail-wj0-x244.google.com [IPv6:2a00:1450:400c:c01::244]) by gabe.freedesktop.org (Postfix) with ESMTPS id 885C36E3F7 for ; Tue, 29 Nov 2016 08:50:30 +0000 (UTC) Received: by mail-wj0-x244.google.com with SMTP id kp2so17167100wjc.0 for ; Tue, 29 Nov 2016 00:50:30 -0800 (PST) Content-Disposition: inline In-Reply-To: <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Neil Armstrong Cc: Xing.Xu@amlogic.com, victor.wan@amlogic.com, khilman@baylibre.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-amlogic@lists.infradead.org, carlo@caione.org, jerry.cao@amlogic.com, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org SGkgTmVpbCwKCk9uIE1vbiwgTm92IDI4LCAyMDE2IGF0IDEwOjM0OjU4QU0gKzAxMDAsIE5laWwg QXJtc3Ryb25nIHdyb3RlOgo+IE9uIDExLzI4LzIwMTYgMDk6MTYgQU0sIERhbmllbCBWZXR0ZXIg d3JvdGU6Cj4gPiBPbiBGcmksIE5vdiAyNSwgMjAxNiBhdCAwNTowMzowOVBNICswMTAwLCBOZWls IEFybXN0cm9uZyB3cm90ZToKPiA+PiArc3RhdGljIHZvaWQgbWVzb25fY3Zic19lbmNvZGVyX2Rp c2FibGUoc3RydWN0IGRybV9lbmNvZGVyICplbmNvZGVyKQo+ID4+ICt7Cj4gPj4gKwlzdHJ1Y3Qg bWVzb25fY3ZicyAqbWVzb25fY3ZicyA9IGVuY29kZXJfdG9fbWVzb25fY3ZicyhlbmNvZGVyKTsK PiA+PiArCj4gPj4gKwltZXNvbl92ZW5jaV9jdmJzX2Rpc2FibGUobWVzb25fY3Zicy0+cHJpdik7 Cj4gPj4gK30KPiA+PiArCj4gPj4gK3N0YXRpYyB2b2lkIG1lc29uX2N2YnNfZW5jb2Rlcl9lbmFi bGUoc3RydWN0IGRybV9lbmNvZGVyICplbmNvZGVyKQo+ID4+ICt7Cj4gPj4gKwlzdHJ1Y3QgbWVz b25fY3ZicyAqbWVzb25fY3ZicyA9IGVuY29kZXJfdG9fbWVzb25fY3ZicyhlbmNvZGVyKTsKPiA+ PiArCj4gPj4gKwltZXNvbl92ZW5jaV9jdmJzX2VuYWJsZShtZXNvbl9jdmJzLT5wcml2KTsKPiA+ PiArfQo+ID4gCj4gPiBQZXJzb25hbGx5IEknZCByZW1vdmUgdGhlIGluZGlyZWN0aW9uIGFib3Zl LCBtb3JlIGRpcmVjdCBjb2RlIGlzIGVhc2llciB0bwo+ID4gcmVhZC4KPiAKPiBJIHVuZGVyc3Rh bmQsIEknbGwgbWF5YmUgY2hhbmdlIHRoZSBtZXNvbl92ZW5jaV9jdmJzX1hYYWJsZSB0byBiZQo+ IGRpcmVjdGx5IGFkZGVkIHRvIHRoZSBvcHMuCj4gCj4gSSB3YW50IHRvIGtlZXAgdGhlIHJlZ2lz dGVycyBzZXR1cCBpbiBzZXBhcmF0ZSBmaWxlcyBhbmQga2VlcCBhIGNsZWFuCj4gRFJNL0hXIHNl cGFyYXRpb24uCgpJIGZpZ3VyZWQgdGhpcyBpcyB3b3J0aCBjbGFyaWZ5aW5nLCBhbmQgSSdtIHNv bWV3aGF0IGd1ZXNzaW5nIGF0IHlvdXIKbW90aXZhdGlvbiBoZXJlIGZvciBhIGNsZWFuIGRybS9o dyBzcGxpdC4gVGhlcmUncyBvZiBjb3Vyc2UgdmFyaW91cyBsZXZlbHMKb2YgaG93IG11Y2ggeW91 IGNhbiBzcGxpdCB0aGUgZHJtIHNpZGUgZnJvbSB5b3VyIGh3IGJhY2tlbmQsIGJ1dCBpbgpnZW5l cmFsIHRoYXQgZGVzaWduIGFwcHJvYWNoIGlzIHJlYWxseSB1bnBvcHVsYXIgd2l0aCB1cHN0cmVh bS4gSXQgZ29lcyBieQp0aGUgbmFtZSBvZiAibWlkbGF5ZXIiLCBhbmQgdGhlIHRyb3VibGUgd2l0 aCBpdCBpcyB0aGF0IGl0IG1ha2VzCnN1YnN5c3RlbSByZWZhY3RvcmluZyBtb3JlIGNvbXBsaWNh dGVkLgoKRm9yIHRoZSBkcml2ZXIgaXRzZWxmIGl0J3MgbmljZSwgYmVjYXVzZSBpdCBpc29sYXRl cyB5b3UgYSBiaXQgZnJvbSBkcm0KY29yZS4gQnV0IHRoYXQgZXhhY3QgaXNvbGF0aW9uIGlzIHRo ZSBwcm9ibGVtIHdoZW4gc29tZW9uZSB3YW50cyAob3IgbW9yZQpvZnRlbiwgbmVlZHMgdG8pIHJl ZmFjdG9yIHNvbWV0aGluZyBhY3Jvc3MgdGhlIGVudGlyZSBzdWJzeXN0ZW0uIFRoZW4gYWxsCnRo ZXNlIGRyaXZlci1wcml2YXRlIGxpdHRsZSAob3Igc29tZXRpbWVzIG11Y2ggYmlnZ2VyKSBhYnN0 cmFjdGlvbnMgZ2V0IGluCnRoZSB3YXkuIFRoYXQncyB3YXkgSSBzdWdnZXN0ZWQgdG8gcmVtb3Zl IGl0IChib3RoIGhlcmUgYW5kIGluIHRoZSBwbGFuZQpjb2RlKSwgYmVjYXVzZSBmb3IgdXBzdHJl YW0gdGhlIG92ZXJhbGwgc3Vic3lzdGVtIG1hdHRlcnMgbW9yZSB0aGFuIGVhY2gKaW5kaXZpZHVh bCBkcml2ZXIuIEdQVXMgY2hhbmdlIGZhc3QsIHdlIG5lZWQgdG8gYmUgYWJsZSB0byBhZGFwdCBm YXN0LAp0b28uCgpBbnl3YXkgeW91J3JlIGRyaXZlcidzIHByZXR0eSBzbWFsbCwgc28gcGVyc29u YWxseSBJIGRvbid0IG1pbmQgbXVjaC4gSSdkCnN0aWxsIHRoaW5rIHJlbW92aW5nIHRoZSBpbmRp cmVjdGlvbiB3b3VsZCBiZSBiZXR0ZXIgdGhvdWdoLgoKVGhhbmtzLCBEYW5pZWwKLS0gCkRhbmll bCBWZXR0ZXIKU29mdHdhcmUgRW5naW5lZXIsIEludGVsIENvcnBvcmF0aW9uCmh0dHA6Ly9ibG9n LmZmd2xsLmNoCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f CmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpo dHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Tue, 29 Nov 2016 09:50:24 +0100 Subject: [RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller In-Reply-To: <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> References: <1480089791-12517-1-git-send-email-narmstrong@baylibre.com> <1480089791-12517-2-git-send-email-narmstrong@baylibre.com> <20161128081601.wapare3pyzmzghvp@phenom.ffwll.local> <99e71b1b-b0cc-ec54-6ca9-417d20195aca@baylibre.com> Message-ID: <20161129085024.tqf5takulvv7f34x@phenom.ffwll.local> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org Hi Neil, On Mon, Nov 28, 2016 at 10:34:58AM +0100, Neil Armstrong wrote: > On 11/28/2016 09:16 AM, Daniel Vetter wrote: > > On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote: > >> +static void meson_cvbs_encoder_disable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder); > >> + > >> + meson_venci_cvbs_disable(meson_cvbs->priv); > >> +} > >> + > >> +static void meson_cvbs_encoder_enable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder); > >> + > >> + meson_venci_cvbs_enable(meson_cvbs->priv); > >> +} > > > > Personally I'd remove the indirection above, more direct code is easier to > > read. > > I understand, I'll maybe change the meson_venci_cvbs_XXable to be > directly added to the ops. > > I want to keep the registers setup in separate files and keep a clean > DRM/HW separation. I figured this is worth clarifying, and I'm somewhat guessing at your motivation here for a clean drm/hw split. There's of course various levels of how much you can split the drm side from your hw backend, but in general that design approach is really unpopular with upstream. It goes by the name of "midlayer", and the trouble with it is that it makes subsystem refactoring more complicated. For the driver itself it's nice, because it isolates you a bit from drm core. But that exact isolation is the problem when someone wants (or more often, needs to) refactor something across the entire subsystem. Then all these driver-private little (or sometimes much bigger) abstractions get in the way. That's way I suggested to remove it (both here and in the plane code), because for upstream the overall subsystem matters more than each individual driver. GPUs change fast, we need to be able to adapt fast, too. Anyway you're driver's pretty small, so personally I don't mind much. I'd still think removing the indirection would be better though. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch