From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754051AbdDDNvN (ORCPT ); Tue, 4 Apr 2017 09:51:13 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35958 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753491AbdDDNvM (ORCPT ); Tue, 4 Apr 2017 09:51:12 -0400 Date: Tue, 4 Apr 2017 15:50:56 +0200 From: Daniel Vetter To: Neil Armstrong Cc: airlied@linux.ie, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 07/13] drm/meson: Add support for HDMI encoder and DW-HDMI bridge + PHY Message-ID: <20170404134957.my2p7hocjolvjqbl@phenom.ffwll.local> Mail-Followup-To: Neil Armstrong , airlied@linux.ie, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org References: <1490109950-21421-1-git-send-email-narmstrong@baylibre.com> <1490109950-21421-8-git-send-email-narmstrong@baylibre.com> <20170404085752.jg246ilczhgmyyhy@phenom.ffwll.local> <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> X-Operating-System: Linux phenom 4.9.0-2-amd64 User-Agent: NeoMutt/20170306 (1.8.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 04, 2017 at 11:16:23AM +0200, Neil Armstrong wrote: > On 04/04/2017 10:57 AM, Daniel Vetter wrote: > > On Tue, Mar 21, 2017 at 04:25:44PM +0100, Neil Armstrong wrote: > >> The Amlogic Meson GXBB/GXL/GXM SoCs embeds a Synopsys DesignWare HDMI TX > >> Controller with a custom Bridge + PHY around the Controller. > >> > >> This driver makes uses of all the custom PHY plat data callbacks and enables > >> the compatible HDMI modes to be configured as a drm_encoder instance. > >> > >> Signed-off-by: Neil Armstrong > > > > [snip] > > > >> +static int meson_venc_hdmi_encoder_atomic_check(struct drm_encoder *encoder, > >> + struct drm_crtc_state *crtc_state, > >> + struct drm_connector_state *conn_state) > >> +{ > >> + return 0; > >> +} > > > > Given the over-the-top complicated mode encoding you seem to have, this > > feels like it's a bit too simply. > > Indeed, but the HW is really weird, every supported modes have very specific > timings/parameters so moving the mode validation code from the dw-hdmi mode_valid > to here would only make sense if we need to support a different HDMI controller. > > But you are right, but I would have preferred to have a better HW for sure... Oh, if your constraints on the meson encoder match what dw-hdmi needs, then the mode_valid checks in there are good enough. A comment might be good in that case. But it looked to me (at a very cursory glance) that the meson encoder has some additional fun restrictions on top. > >> + > >> +static void meson_venc_hdmi_encoder_disable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + > >> + DRM_DEBUG_DRIVER("\n"); > >> + > >> + writel_bits_relaxed(0x3, 0, > >> + priv->io_base + _REG(VPU_HDMI_SETTING)); > >> + > >> + writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN)); > >> + writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN)); > >> +} > >> + > >> +static void meson_venc_hdmi_encoder_enable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + > >> + DRM_DEBUG_DRIVER("%s\n", priv->venc.hdmi_use_enci ? "VENCI" : "VENCP"); > >> + > >> + if (priv->venc.hdmi_use_enci) > >> + writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN)); > >> + else > >> + writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN)); > >> +} > >> + > >> +static void meson_venc_hdmi_encoder_mode_set(struct drm_encoder *encoder, > >> + struct drm_display_mode *mode, > >> + struct drm_display_mode *adjusted_mode) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + int vic = drm_match_cea_mode(mode); > >> + > >> + DRM_DEBUG_DRIVER("%d:\"%s\" vic %d\n", > >> + mode->base.id, mode->name, vic); > >> + > >> + /* Should have been filtered */ > >> + if (!vic) > >> + return; > >> + > >> + /* VENC + VENC-DVI Mode setup */ > >> + meson_venc_hdmi_mode_set(priv, vic, mode); > > > > So this calls a different module which export_symbol_gpls that thing. I > > have no idea why arm-soc people love modularized-to-the-function level > > drivers, but it feels over the top. amd/nouveau/i915 all smash everything > > into one driver, makes life so much easier. > > I know, we are doomed on that ! > But here, since the wrapping around the dw-hdmi controller is completely custom > if was necessary to add a separate encoder tied to HDMI and have the physical > encoding code in the common driver. > Note that the platform is also able to driver a LCD via LVDS, so this encoder code > should be reusable here. I'm not talking about the custom encoder or anything like that, or code reuse. I'm talking about doing piles of separate .ko when you could have just one (with a bunch of component drivers contained within). At least this is how the really big drivers all work. Of course shared ip (like the dw-hdmi bridge driver) need to be in separate .ko, that part completely makes sense. > > Note: bridge drivers as separate .ko makes sense, but separate .ko for > > every single functional unit in your vendor IP imo totally doesn't. > > Actually I added a global ko for the "DRM" driver with crtc, planes and CVBS, > and another ko for the HDMI bridge wrapping. Or maybe I just misunderstood things, but meson_venc_hdmi_mode_set looks like it could be pulled into this module? -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, 4 Apr 2017 15:50:56 +0200 Subject: [PATCH v2 07/13] drm/meson: Add support for HDMI encoder and DW-HDMI bridge + PHY In-Reply-To: <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> References: <1490109950-21421-1-git-send-email-narmstrong@baylibre.com> <1490109950-21421-8-git-send-email-narmstrong@baylibre.com> <20170404085752.jg246ilczhgmyyhy@phenom.ffwll.local> <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> Message-ID: <20170404134957.my2p7hocjolvjqbl@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Apr 04, 2017 at 11:16:23AM +0200, Neil Armstrong wrote: > On 04/04/2017 10:57 AM, Daniel Vetter wrote: > > On Tue, Mar 21, 2017 at 04:25:44PM +0100, Neil Armstrong wrote: > >> The Amlogic Meson GXBB/GXL/GXM SoCs embeds a Synopsys DesignWare HDMI TX > >> Controller with a custom Bridge + PHY around the Controller. > >> > >> This driver makes uses of all the custom PHY plat data callbacks and enables > >> the compatible HDMI modes to be configured as a drm_encoder instance. > >> > >> Signed-off-by: Neil Armstrong > > > > [snip] > > > >> +static int meson_venc_hdmi_encoder_atomic_check(struct drm_encoder *encoder, > >> + struct drm_crtc_state *crtc_state, > >> + struct drm_connector_state *conn_state) > >> +{ > >> + return 0; > >> +} > > > > Given the over-the-top complicated mode encoding you seem to have, this > > feels like it's a bit too simply. > > Indeed, but the HW is really weird, every supported modes have very specific > timings/parameters so moving the mode validation code from the dw-hdmi mode_valid > to here would only make sense if we need to support a different HDMI controller. > > But you are right, but I would have preferred to have a better HW for sure... Oh, if your constraints on the meson encoder match what dw-hdmi needs, then the mode_valid checks in there are good enough. A comment might be good in that case. But it looked to me (at a very cursory glance) that the meson encoder has some additional fun restrictions on top. > >> + > >> +static void meson_venc_hdmi_encoder_disable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + > >> + DRM_DEBUG_DRIVER("\n"); > >> + > >> + writel_bits_relaxed(0x3, 0, > >> + priv->io_base + _REG(VPU_HDMI_SETTING)); > >> + > >> + writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN)); > >> + writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN)); > >> +} > >> + > >> +static void meson_venc_hdmi_encoder_enable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + > >> + DRM_DEBUG_DRIVER("%s\n", priv->venc.hdmi_use_enci ? "VENCI" : "VENCP"); > >> + > >> + if (priv->venc.hdmi_use_enci) > >> + writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN)); > >> + else > >> + writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN)); > >> +} > >> + > >> +static void meson_venc_hdmi_encoder_mode_set(struct drm_encoder *encoder, > >> + struct drm_display_mode *mode, > >> + struct drm_display_mode *adjusted_mode) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + int vic = drm_match_cea_mode(mode); > >> + > >> + DRM_DEBUG_DRIVER("%d:\"%s\" vic %d\n", > >> + mode->base.id, mode->name, vic); > >> + > >> + /* Should have been filtered */ > >> + if (!vic) > >> + return; > >> + > >> + /* VENC + VENC-DVI Mode setup */ > >> + meson_venc_hdmi_mode_set(priv, vic, mode); > > > > So this calls a different module which export_symbol_gpls that thing. I > > have no idea why arm-soc people love modularized-to-the-function level > > drivers, but it feels over the top. amd/nouveau/i915 all smash everything > > into one driver, makes life so much easier. > > I know, we are doomed on that ! > But here, since the wrapping around the dw-hdmi controller is completely custom > if was necessary to add a separate encoder tied to HDMI and have the physical > encoding code in the common driver. > Note that the platform is also able to driver a LCD via LVDS, so this encoder code > should be reusable here. I'm not talking about the custom encoder or anything like that, or code reuse. I'm talking about doing piles of separate .ko when you could have just one (with a bunch of component drivers contained within). At least this is how the really big drivers all work. Of course shared ip (like the dw-hdmi bridge driver) need to be in separate .ko, that part completely makes sense. > > Note: bridge drivers as separate .ko makes sense, but separate .ko for > > every single functional unit in your vendor IP imo totally doesn't. > > Actually I added a global ko for the "DRM" driver with crtc, planes and CVBS, > and another ko for the HDMI bridge wrapping. Or maybe I just misunderstood things, but meson_venc_hdmi_mode_set looks like it could be pulled into this module? -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: [PATCH v2 07/13] drm/meson: Add support for HDMI encoder and DW-HDMI bridge + PHY Date: Tue, 4 Apr 2017 15:50:56 +0200 Message-ID: <20170404134957.my2p7hocjolvjqbl@phenom.ffwll.local> References: <1490109950-21421-1-git-send-email-narmstrong@baylibre.com> <1490109950-21421-8-git-send-email-narmstrong@baylibre.com> <20170404085752.jg246ilczhgmyyhy@phenom.ffwll.local> <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id EC1626E420 for ; Tue, 4 Apr 2017 13:51:06 +0000 (UTC) Received: by mail-wm0-x243.google.com with SMTP id z133so6079810wmb.2 for ; Tue, 04 Apr 2017 06:51:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: <3b6333c4-2fe0-8331-e54b-01d956e97797@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: linux-amlogic@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org T24gVHVlLCBBcHIgMDQsIDIwMTcgYXQgMTE6MTY6MjNBTSArMDIwMCwgTmVpbCBBcm1zdHJvbmcg d3JvdGU6Cj4gT24gMDQvMDQvMjAxNyAxMDo1NyBBTSwgRGFuaWVsIFZldHRlciB3cm90ZToKPiA+ IE9uIFR1ZSwgTWFyIDIxLCAyMDE3IGF0IDA0OjI1OjQ0UE0gKzAxMDAsIE5laWwgQXJtc3Ryb25n IHdyb3RlOgo+ID4+IFRoZSBBbWxvZ2ljIE1lc29uIEdYQkIvR1hML0dYTSBTb0NzIGVtYmVkcyBh IFN5bm9wc3lzIERlc2lnbldhcmUgSERNSSBUWAo+ID4+IENvbnRyb2xsZXIgd2l0aCBhIGN1c3Rv bSBCcmlkZ2UgKyBQSFkgYXJvdW5kIHRoZSBDb250cm9sbGVyLgo+ID4+Cj4gPj4gVGhpcyBkcml2 ZXIgbWFrZXMgdXNlcyBvZiBhbGwgdGhlIGN1c3RvbSBQSFkgcGxhdCBkYXRhIGNhbGxiYWNrcyBh bmQgZW5hYmxlcwo+ID4+IHRoZSBjb21wYXRpYmxlIEhETUkgbW9kZXMgdG8gYmUgY29uZmlndXJl ZCBhcyBhIGRybV9lbmNvZGVyIGluc3RhbmNlLgo+ID4+Cj4gPj4gU2lnbmVkLW9mZi1ieTogTmVp bCBBcm1zdHJvbmcgPG5hcm1zdHJvbmdAYmF5bGlicmUuY29tPgo+ID4gCj4gPiBbc25pcF0KPiA+ IAo+ID4+ICtzdGF0aWMgaW50IG1lc29uX3ZlbmNfaGRtaV9lbmNvZGVyX2F0b21pY19jaGVjayhz dHJ1Y3QgZHJtX2VuY29kZXIgKmVuY29kZXIsCj4gPj4gKwkJCQkJc3RydWN0IGRybV9jcnRjX3N0 YXRlICpjcnRjX3N0YXRlLAo+ID4+ICsJCQkJCXN0cnVjdCBkcm1fY29ubmVjdG9yX3N0YXRlICpj b25uX3N0YXRlKQo+ID4+ICt7Cj4gPj4gKwlyZXR1cm4gMDsKPiA+PiArfQo+ID4gCj4gPiBHaXZl biB0aGUgb3Zlci10aGUtdG9wIGNvbXBsaWNhdGVkIG1vZGUgZW5jb2RpbmcgeW91IHNlZW0gdG8g aGF2ZSwgdGhpcwo+ID4gZmVlbHMgbGlrZSBpdCdzIGEgYml0IHRvbyBzaW1wbHkuCj4gCj4gSW5k ZWVkLCBidXQgdGhlIEhXIGlzIHJlYWxseSB3ZWlyZCwgZXZlcnkgc3VwcG9ydGVkIG1vZGVzIGhh dmUgdmVyeSBzcGVjaWZpYwo+IHRpbWluZ3MvcGFyYW1ldGVycyBzbyBtb3ZpbmcgdGhlIG1vZGUg dmFsaWRhdGlvbiBjb2RlIGZyb20gdGhlIGR3LWhkbWkgbW9kZV92YWxpZAo+IHRvIGhlcmUgd291 bGQgb25seSBtYWtlIHNlbnNlIGlmIHdlIG5lZWQgdG8gc3VwcG9ydCBhIGRpZmZlcmVudCBIRE1J IGNvbnRyb2xsZXIuCj4gCj4gQnV0IHlvdSBhcmUgcmlnaHQsIGJ1dCBJIHdvdWxkIGhhdmUgcHJl ZmVycmVkIHRvIGhhdmUgYSBiZXR0ZXIgSFcgZm9yIHN1cmUuLi4KCk9oLCBpZiB5b3VyIGNvbnN0 cmFpbnRzIG9uIHRoZSBtZXNvbiBlbmNvZGVyIG1hdGNoIHdoYXQgZHctaGRtaSBuZWVkcywKdGhl biB0aGUgbW9kZV92YWxpZCBjaGVja3MgaW4gdGhlcmUgYXJlIGdvb2QgZW5vdWdoLiBBIGNvbW1l bnQgbWlnaHQgYmUKZ29vZCBpbiB0aGF0IGNhc2UuCgpCdXQgaXQgbG9va2VkIHRvIG1lIChhdCBh IHZlcnkgY3Vyc29yeSBnbGFuY2UpIHRoYXQgdGhlIG1lc29uIGVuY29kZXIgaGFzCnNvbWUgYWRk aXRpb25hbCBmdW4gcmVzdHJpY3Rpb25zIG9uIHRvcC4KCj4gPj4gKwo+ID4+ICtzdGF0aWMgdm9p ZCBtZXNvbl92ZW5jX2hkbWlfZW5jb2Rlcl9kaXNhYmxlKHN0cnVjdCBkcm1fZW5jb2RlciAqZW5j b2RlcikKPiA+PiArewo+ID4+ICsJc3RydWN0IG1lc29uX2R3X2hkbWkgKmR3X2hkbWkgPSBlbmNv ZGVyX3RvX21lc29uX2R3X2hkbWkoZW5jb2Rlcik7Cj4gPj4gKwlzdHJ1Y3QgbWVzb25fZHJtICpw cml2ID0gZHdfaGRtaS0+cHJpdjsKPiA+PiArCj4gPj4gKwlEUk1fREVCVUdfRFJJVkVSKCJcbiIp Owo+ID4+ICsKPiA+PiArCXdyaXRlbF9iaXRzX3JlbGF4ZWQoMHgzLCAwLAo+ID4+ICsJCQkgICAg cHJpdi0+aW9fYmFzZSArIF9SRUcoVlBVX0hETUlfU0VUVElORykpOwo+ID4+ICsKPiA+PiArCXdy aXRlbF9yZWxheGVkKDAsIHByaXYtPmlvX2Jhc2UgKyBfUkVHKEVOQ0lfVklERU9fRU4pKTsKPiA+ PiArCXdyaXRlbF9yZWxheGVkKDAsIHByaXYtPmlvX2Jhc2UgKyBfUkVHKEVOQ1BfVklERU9fRU4p KTsKPiA+PiArfQo+ID4+ICsKPiA+PiArc3RhdGljIHZvaWQgbWVzb25fdmVuY19oZG1pX2VuY29k ZXJfZW5hYmxlKHN0cnVjdCBkcm1fZW5jb2RlciAqZW5jb2RlcikKPiA+PiArewo+ID4+ICsJc3Ry dWN0IG1lc29uX2R3X2hkbWkgKmR3X2hkbWkgPSBlbmNvZGVyX3RvX21lc29uX2R3X2hkbWkoZW5j b2Rlcik7Cj4gPj4gKwlzdHJ1Y3QgbWVzb25fZHJtICpwcml2ID0gZHdfaGRtaS0+cHJpdjsKPiA+ PiArCj4gPj4gKwlEUk1fREVCVUdfRFJJVkVSKCIlc1xuIiwgcHJpdi0+dmVuYy5oZG1pX3VzZV9l bmNpID8gIlZFTkNJIiA6ICJWRU5DUCIpOwo+ID4+ICsKPiA+PiArCWlmIChwcml2LT52ZW5jLmhk bWlfdXNlX2VuY2kpCj4gPj4gKwkJd3JpdGVsX3JlbGF4ZWQoMSwgcHJpdi0+aW9fYmFzZSArIF9S RUcoRU5DSV9WSURFT19FTikpOwo+ID4+ICsJZWxzZQo+ID4+ICsJCXdyaXRlbF9yZWxheGVkKDEs IHByaXYtPmlvX2Jhc2UgKyBfUkVHKEVOQ1BfVklERU9fRU4pKTsKPiA+PiArfQo+ID4+ICsKPiA+ PiArc3RhdGljIHZvaWQgbWVzb25fdmVuY19oZG1pX2VuY29kZXJfbW9kZV9zZXQoc3RydWN0IGRy bV9lbmNvZGVyICplbmNvZGVyLAo+ID4+ICsJCQkJICAgc3RydWN0IGRybV9kaXNwbGF5X21vZGUg Km1vZGUsCj4gPj4gKwkJCQkgICBzdHJ1Y3QgZHJtX2Rpc3BsYXlfbW9kZSAqYWRqdXN0ZWRfbW9k ZSkKPiA+PiArewo+ID4+ICsJc3RydWN0IG1lc29uX2R3X2hkbWkgKmR3X2hkbWkgPSBlbmNvZGVy X3RvX21lc29uX2R3X2hkbWkoZW5jb2Rlcik7Cj4gPj4gKwlzdHJ1Y3QgbWVzb25fZHJtICpwcml2 ID0gZHdfaGRtaS0+cHJpdjsKPiA+PiArCWludCB2aWMgPSBkcm1fbWF0Y2hfY2VhX21vZGUobW9k ZSk7Cj4gPj4gKwo+ID4+ICsJRFJNX0RFQlVHX0RSSVZFUigiJWQ6XCIlc1wiIHZpYyAlZFxuIiwK PiA+PiArCQkJIG1vZGUtPmJhc2UuaWQsIG1vZGUtPm5hbWUsIHZpYyk7Cj4gPj4gKwo+ID4+ICsJ LyogU2hvdWxkIGhhdmUgYmVlbiBmaWx0ZXJlZCAqLwo+ID4+ICsJaWYgKCF2aWMpCj4gPj4gKwkJ cmV0dXJuOwo+ID4+ICsKPiA+PiArCS8qIFZFTkMgKyBWRU5DLURWSSBNb2RlIHNldHVwICovCj4g Pj4gKwltZXNvbl92ZW5jX2hkbWlfbW9kZV9zZXQocHJpdiwgdmljLCBtb2RlKTsKPiA+IAo+ID4g U28gdGhpcyBjYWxscyBhIGRpZmZlcmVudCBtb2R1bGUgd2hpY2ggZXhwb3J0X3N5bWJvbF9ncGxz IHRoYXQgdGhpbmcuIEkKPiA+IGhhdmUgbm8gaWRlYSB3aHkgYXJtLXNvYyBwZW9wbGUgbG92ZSBt b2R1bGFyaXplZC10by10aGUtZnVuY3Rpb24gbGV2ZWwKPiA+IGRyaXZlcnMsIGJ1dCBpdCBmZWVs cyBvdmVyIHRoZSB0b3AuIGFtZC9ub3V2ZWF1L2k5MTUgYWxsIHNtYXNoIGV2ZXJ5dGhpbmcKPiA+ IGludG8gb25lIGRyaXZlciwgbWFrZXMgbGlmZSBzbyBtdWNoIGVhc2llci4KPiAKPiBJIGtub3cs IHdlIGFyZSBkb29tZWQgb24gdGhhdCAhCj4gQnV0IGhlcmUsIHNpbmNlIHRoZSB3cmFwcGluZyBh cm91bmQgdGhlIGR3LWhkbWkgY29udHJvbGxlciBpcyBjb21wbGV0ZWx5IGN1c3RvbQo+IGlmIHdh cyBuZWNlc3NhcnkgdG8gYWRkIGEgc2VwYXJhdGUgZW5jb2RlciB0aWVkIHRvIEhETUkgYW5kIGhh dmUgdGhlIHBoeXNpY2FsCj4gZW5jb2RpbmcgY29kZSBpbiB0aGUgY29tbW9uIGRyaXZlci4KPiBO b3RlIHRoYXQgdGhlIHBsYXRmb3JtIGlzIGFsc28gYWJsZSB0byBkcml2ZXIgYSBMQ0QgdmlhIExW RFMsIHNvIHRoaXMgZW5jb2RlciBjb2RlCj4gc2hvdWxkIGJlIHJldXNhYmxlIGhlcmUuCgpJJ20g bm90IHRhbGtpbmcgYWJvdXQgdGhlIGN1c3RvbSBlbmNvZGVyIG9yIGFueXRoaW5nIGxpa2UgdGhh dCwgb3IgY29kZQpyZXVzZS4gSSdtIHRhbGtpbmcgYWJvdXQgZG9pbmcgcGlsZXMgb2Ygc2VwYXJh dGUgLmtvIHdoZW4geW91IGNvdWxkIGhhdmUKanVzdCBvbmUgKHdpdGggYSBidW5jaCBvZiBjb21w b25lbnQgZHJpdmVycyBjb250YWluZWQgd2l0aGluKS4gQXQgbGVhc3QKdGhpcyBpcyBob3cgdGhl IHJlYWxseSBiaWcgZHJpdmVycyBhbGwgd29yay4KCk9mIGNvdXJzZSBzaGFyZWQgaXAgKGxpa2Ug dGhlIGR3LWhkbWkgYnJpZGdlIGRyaXZlcikgbmVlZCB0byBiZSBpbgpzZXBhcmF0ZSAua28sIHRo YXQgcGFydCBjb21wbGV0ZWx5IG1ha2VzIHNlbnNlLgoKPiA+IE5vdGU6IGJyaWRnZSBkcml2ZXJz IGFzIHNlcGFyYXRlIC5rbyBtYWtlcyBzZW5zZSwgYnV0IHNlcGFyYXRlIC5rbyBmb3IKPiA+IGV2 ZXJ5IHNpbmdsZSBmdW5jdGlvbmFsIHVuaXQgaW4geW91ciB2ZW5kb3IgSVAgaW1vIHRvdGFsbHkg ZG9lc24ndC4KPiAKPiBBY3R1YWxseSBJIGFkZGVkIGEgZ2xvYmFsIGtvIGZvciB0aGUgIkRSTSIg ZHJpdmVyIHdpdGggY3J0YywgcGxhbmVzIGFuZCBDVkJTLAo+IGFuZCBhbm90aGVyIGtvIGZvciB0 aGUgSERNSSBicmlkZ2Ugd3JhcHBpbmcuCgpPciBtYXliZSBJIGp1c3QgbWlzdW5kZXJzdG9vZCB0 aGluZ3MsIGJ1dCBtZXNvbl92ZW5jX2hkbWlfbW9kZV9zZXQgbG9va3MKbGlrZSBpdCBjb3VsZCBi ZSBwdWxsZWQgaW50byB0aGlzIG1vZHVsZT8KLURhbmllbAotLSAKRGFuaWVsIFZldHRlcgpTb2Z0 d2FyZSBFbmdpbmVlciwgSW50ZWwgQ29ycG9yYXRpb24KaHR0cDovL2Jsb2cuZmZ3bGwuY2gKX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1h aWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMu ZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Tue, 4 Apr 2017 15:50:56 +0200 Subject: [PATCH v2 07/13] drm/meson: Add support for HDMI encoder and DW-HDMI bridge + PHY In-Reply-To: <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> References: <1490109950-21421-1-git-send-email-narmstrong@baylibre.com> <1490109950-21421-8-git-send-email-narmstrong@baylibre.com> <20170404085752.jg246ilczhgmyyhy@phenom.ffwll.local> <3b6333c4-2fe0-8331-e54b-01d956e97797@baylibre.com> Message-ID: <20170404134957.my2p7hocjolvjqbl@phenom.ffwll.local> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On Tue, Apr 04, 2017 at 11:16:23AM +0200, Neil Armstrong wrote: > On 04/04/2017 10:57 AM, Daniel Vetter wrote: > > On Tue, Mar 21, 2017 at 04:25:44PM +0100, Neil Armstrong wrote: > >> The Amlogic Meson GXBB/GXL/GXM SoCs embeds a Synopsys DesignWare HDMI TX > >> Controller with a custom Bridge + PHY around the Controller. > >> > >> This driver makes uses of all the custom PHY plat data callbacks and enables > >> the compatible HDMI modes to be configured as a drm_encoder instance. > >> > >> Signed-off-by: Neil Armstrong > > > > [snip] > > > >> +static int meson_venc_hdmi_encoder_atomic_check(struct drm_encoder *encoder, > >> + struct drm_crtc_state *crtc_state, > >> + struct drm_connector_state *conn_state) > >> +{ > >> + return 0; > >> +} > > > > Given the over-the-top complicated mode encoding you seem to have, this > > feels like it's a bit too simply. > > Indeed, but the HW is really weird, every supported modes have very specific > timings/parameters so moving the mode validation code from the dw-hdmi mode_valid > to here would only make sense if we need to support a different HDMI controller. > > But you are right, but I would have preferred to have a better HW for sure... Oh, if your constraints on the meson encoder match what dw-hdmi needs, then the mode_valid checks in there are good enough. A comment might be good in that case. But it looked to me (at a very cursory glance) that the meson encoder has some additional fun restrictions on top. > >> + > >> +static void meson_venc_hdmi_encoder_disable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + > >> + DRM_DEBUG_DRIVER("\n"); > >> + > >> + writel_bits_relaxed(0x3, 0, > >> + priv->io_base + _REG(VPU_HDMI_SETTING)); > >> + > >> + writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN)); > >> + writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN)); > >> +} > >> + > >> +static void meson_venc_hdmi_encoder_enable(struct drm_encoder *encoder) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + > >> + DRM_DEBUG_DRIVER("%s\n", priv->venc.hdmi_use_enci ? "VENCI" : "VENCP"); > >> + > >> + if (priv->venc.hdmi_use_enci) > >> + writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN)); > >> + else > >> + writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN)); > >> +} > >> + > >> +static void meson_venc_hdmi_encoder_mode_set(struct drm_encoder *encoder, > >> + struct drm_display_mode *mode, > >> + struct drm_display_mode *adjusted_mode) > >> +{ > >> + struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder); > >> + struct meson_drm *priv = dw_hdmi->priv; > >> + int vic = drm_match_cea_mode(mode); > >> + > >> + DRM_DEBUG_DRIVER("%d:\"%s\" vic %d\n", > >> + mode->base.id, mode->name, vic); > >> + > >> + /* Should have been filtered */ > >> + if (!vic) > >> + return; > >> + > >> + /* VENC + VENC-DVI Mode setup */ > >> + meson_venc_hdmi_mode_set(priv, vic, mode); > > > > So this calls a different module which export_symbol_gpls that thing. I > > have no idea why arm-soc people love modularized-to-the-function level > > drivers, but it feels over the top. amd/nouveau/i915 all smash everything > > into one driver, makes life so much easier. > > I know, we are doomed on that ! > But here, since the wrapping around the dw-hdmi controller is completely custom > if was necessary to add a separate encoder tied to HDMI and have the physical > encoding code in the common driver. > Note that the platform is also able to driver a LCD via LVDS, so this encoder code > should be reusable here. I'm not talking about the custom encoder or anything like that, or code reuse. I'm talking about doing piles of separate .ko when you could have just one (with a bunch of component drivers contained within). At least this is how the really big drivers all work. Of course shared ip (like the dw-hdmi bridge driver) need to be in separate .ko, that part completely makes sense. > > Note: bridge drivers as separate .ko makes sense, but separate .ko for > > every single functional unit in your vendor IP imo totally doesn't. > > Actually I added a global ko for the "DRM" driver with crtc, planes and CVBS, > and another ko for the HDMI bridge wrapping. Or maybe I just misunderstood things, but meson_venc_hdmi_mode_set looks like it could be pulled into this module? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch