From patchwork Tue Jun 10 15:13:06 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Russell King - ARM Linux X-Patchwork-Id: 4329651 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 95C589F1D6 for ; Tue, 10 Jun 2014 15:13:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9B61E202C8 for ; Tue, 10 Jun 2014 15:13:22 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id C5401202BE for ; Tue, 10 Jun 2014 15:13:18 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D50A36E6D8; Tue, 10 Jun 2014 08:13:17 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from pandora.arm.linux.org.uk (gw-1.arm.linux.org.uk [78.32.30.217]) by gabe.freedesktop.org (Postfix) with ESMTP id C343B6E6D8 for ; Tue, 10 Jun 2014 08:13:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=arm.linux.org.uk; s=pandora; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=7gzAMr9yGvFaDux9gFWkvfkveVrhHQmb7SpcBPeelf8=; b=Qj2y9UvWwnzffd3fhud6T+C42QALHamGiWKeuH+TD00ooz9yGdatiHyZX6lkTTYhc6dlwA8zVArKuTuv70phhsjYntjJMzuWjHZsLkSjmuAp9Gl7H+y+CWwcRoNp93hJU8Bm38TNI0PXmX5FB96K74vbezaP1JSgLrc810OKBqI=; Received: from n2100.arm.linux.org.uk ([2002:4e20:1eda:1:214:fdff:fe10:4f86]:35640) by pandora.arm.linux.org.uk with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.76) (envelope-from ) id 1WuNjY-0006BB-9v; Tue, 10 Jun 2014 16:13:08 +0100 Received: from linux by n2100.arm.linux.org.uk with local (Exim 4.76) (envelope-from ) id 1WuNjW-0001L0-W8; Tue, 10 Jun 2014 16:13:07 +0100 Date: Tue, 10 Jun 2014 16:13:06 +0100 From: Russell King - ARM Linux To: Fabio Estevam Subject: Re: [PATCH] imx-drm: imx-hdmi: fix hdmi hotplug detection initial state Message-ID: <20140610151306.GZ23430@n2100.arm.linux.org.uk> References: <20140609140639.GR23430@n2100.arm.linux.org.uk> <20140609174941.GS23430@n2100.arm.linux.org.uk> <20140609200943.GW23430@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.19 (2009-01-05) Cc: devel@driverdev.osuosl.org, Sascha Hauer , Greg Kroah-Hartman , DRI mailing list , Shawn Guo X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jun 10, 2014 at 09:58:54AM -0300, Fabio Estevam wrote: > Booting the kernel with the HDMI cable connected (no image is seen on > HDMI, only on LVDS): Reformatting a bit: > disp 0: panel size = 1920 x 1080 > Clocks: IPU 264000000Hz DI 24000000Hz Needed 138500000Hz > IPU clock can give 132000000 with divider 2, error -4.3% > Want 138500000Hz IPU 264000000Hz DI 138500000Hz using DI, 138500000Hz > disp 1: panel size = 1024 x 768 > Clocks: IPU 264000000Hz DI 64999999Hz Needed 65000000Hz > Want 65000000Hz IPU 264000000Hz DI 64999999Hz using DI, 64999999Hz > > After cable removal: > disp 0: panel size = 1024 x 768 > Clocks: IPU 264000000Hz DI 64999999Hz Needed 65000000Hz > Want 65000000Hz IPU 264000000Hz DI 64999999Hz using DI, 64999999Hz So here we can see that the clock for DI0 got changed beneath it from 138.5MHz to 65MHz, probably when the DI1 clock was changed. > After cable re-insertion (image is seen on both HDMI and LVDS): > disp 0: panel size = 1920 x 1080 > Clocks: IPU 264000000Hz DI 64999999Hz Needed 138500000Hz > IPU clock can give 132000000 with divider 2, error -4.3% > Want 138500000Hz IPU 264000000Hz DI 129999997Hz using DI, 129999997Hz > disp 1: panel size = 1024 x 768 > Clocks: IPU 264000000Hz DI 64999999Hz Needed 65000000Hz > Want 65000000Hz IPU 264000000Hz DI 64999999Hz using DI, 64999999Hz The strange thing here is that DI0, when asking for 138.5MHz, only gets 130MHz (a multiple of DI1's frequency) yet when DI1 reduced it to 65MHz, the CCF just obliged. Let's go back to the basic requirements for LDB, because I think there's a bug here. 1. the LDB serial clock must be 7x the LDB DI clock. 2. for single/split/separate mode, the IPU DI clock must be the same and synchronous with the LDB DI clock. 3. for dual mode, the IPU DI clock must be twice the LDB DI clock, and must be synchronised with the LDB DI clock. (1) is provided for us via the CCM clock tree - there is already a /7 divider between the LDB serial clock and the LDB DI clock: LM --+-------- LDB serial `- /7 -- LDB DI clock where LM is the LDB clock muxer. I'm going to leave case (3) for the time being, because at the moment, I can't see how to achieve it given the clock tree structure that the iMX6 CPUs offer - I'm going to make the assumption at the present time that this mode is not supported. The easiest way to achieve (2) is to set the IPU DI clock mux to select the LDB DI clock directly: LM --+---------------- LDB serial `- /7 -+-------- LDB DI clock `- IM -- IPU DI clock where 'M' is the IPU DI clock muxer. However, we're currently setting this up as: LM --+---------------- LDB serial `- /7 -+-------- LDB DI clock IPM --- /N ---- IM --- IPU DI clock and hoping that the LDB and IPU DI clocks are appropriately synchronised. This is the bug I refer to above. Finally, for HDMI, we need: PLL5 -- IPM -- IM -- IPU DI clock What this implies is that we need control of the IPU DI clock muxer (IM) and each DI needs to know what kind of display bridge(s) are connected to it, so that the appropriate parent(s) can be selected. We can get a little closer to that (and clean up the code) with the patch below. It gives us some of the information we have really been wanting to know all along in ipu_crtc_mode_set(), that is which kinds of bridges are connected to the DI. What we /really/ need to solve the above problem is exactly which bridges are connected as well - whether it's LDB0 or LDB1, and also how to get at the clocks to be able to control that mux. I'm at a loss how best to do that given the complexity of iMX6 DT, and I don't think we want to add these to the IPU/DI node as the IPU/DI isn't really connected to these clocks - it's connected to the IM mux which is internal to the CCM. I think this patch also should help Denis Carikli, as the polarity of the clock signal (etc) also depends on the type(s) of encoders attached to the DI. We really should fail if we end up with encoders with incompatible requirements trying to be bound to a single DI. (side note: I'd really like to get rid of imx_drm_panel_format*()...) In any case, to fix this I think we're looking at changing the DT stuff again, and as the IPU code is moving out of drivers/staging, this is going to become increasing difficult... drivers/staging/imx-drm/imx-drm-core.c | 3 +-- drivers/staging/imx-drm/imx-drm.h | 2 +- drivers/staging/imx-drm/ipuv3-crtc.c | 40 +++++++++++++++++++--------------- 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index 7da0cad27b49..c538c82f8a32 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -115,8 +115,7 @@ int imx_drm_panel_format_pins(struct drm_encoder *encoder, helper = &imx_crtc->imx_drm_helper_funcs; if (helper->set_interface_pix_fmt) return helper->set_interface_pix_fmt(encoder->crtc, - encoder->encoder_type, interface_pix_fmt, - hsync_pin, vsync_pin); + interface_pix_fmt, hsync_pin, vsync_pin); return 0; } EXPORT_SYMBOL_GPL(imx_drm_panel_format_pins); diff --git a/drivers/staging/imx-drm/imx-drm.h b/drivers/staging/imx-drm/imx-drm.h index a322bac55414..0bf8e5eb76e3 100644 --- a/drivers/staging/imx-drm/imx-drm.h +++ b/drivers/staging/imx-drm/imx-drm.h @@ -17,7 +17,7 @@ int imx_drm_crtc_id(struct imx_drm_crtc *crtc); struct imx_drm_crtc_helper_funcs { int (*enable_vblank)(struct drm_crtc *crtc); void (*disable_vblank)(struct drm_crtc *crtc); - int (*set_interface_pix_fmt)(struct drm_crtc *crtc, u32 encoder_type, + int (*set_interface_pix_fmt)(struct drm_crtc *crtc, u32 pix_fmt, int hsync_pin, int vsync_pin); const struct drm_crtc_helper_funcs *crtc_helper_funcs; const struct drm_crtc_funcs *crtc_funcs; diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c index 47bec5e17358..796e9bef15f0 100644 --- a/drivers/staging/imx-drm/ipuv3-crtc.c +++ b/drivers/staging/imx-drm/ipuv3-crtc.c @@ -51,7 +51,6 @@ struct ipu_crtc { struct drm_framebuffer *newfb; int irq; u32 interface_pix_fmt; - unsigned long di_clkflags; int di_hsync_pin; int di_vsync_pin; }; @@ -146,10 +145,13 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) { + struct drm_device *dev = crtc->dev; + struct drm_encoder *encoder; struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); - int ret; struct ipu_di_signal_cfg sig_cfg = {}; + unsigned long encoder_types = 0; u32 out_pixel_fmt; + int ret; dev_dbg(ipu_crtc->dev, "%s: mode->hdisplay: %d\n", __func__, mode->hdisplay); @@ -165,6 +167,24 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, if (mode->flags & DRM_MODE_FLAG_PVSYNC) sig_cfg.Vsync_pol = 1; + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + if (encoder->crtc == crtc) + encoder_types |= BIT(encoder->encoder_type); + + dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%x\n", + __func__, encoder_types); + + /* + * If we have DAC, TVDAC or LDB, then we need the IPU DI clock + * to be the same as the LDB DI clock. + */ + if (encoder_types & (BIT(DRM_MODE_ENCODER_DAC) | + BIT(DRM_MODE_ENCODER_TVDAC) | + BIT(DRM_MODE_ENCODER_LVDS))) + sig_cfg.clkflags = IPU_DI_CLKMODE_SYNC | IPU_DI_CLKMODE_EXT; + else + sig_cfg.clkflags = 0; + sig_cfg.enable_pol = 1; sig_cfg.clk_pol = 0; sig_cfg.width = mode->hdisplay; @@ -178,7 +198,6 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, sig_cfg.v_sync_width = mode->vsync_end - mode->vsync_start; sig_cfg.v_end_width = mode->vsync_start - mode->vdisplay; sig_cfg.pixelclock = mode->clock * 1000; - sig_cfg.clkflags = ipu_crtc->di_clkflags; sig_cfg.v_to_h_sync = 0; @@ -277,7 +296,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc) ipu_crtc->newfb = NULL; } -static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc, u32 encoder_type, +static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc, u32 pixfmt, int hsync_pin, int vsync_pin) { struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); @@ -286,19 +305,6 @@ static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc, u32 encoder_type, ipu_crtc->di_hsync_pin = hsync_pin; ipu_crtc->di_vsync_pin = vsync_pin; - switch (encoder_type) { - case DRM_MODE_ENCODER_DAC: - case DRM_MODE_ENCODER_TVDAC: - case DRM_MODE_ENCODER_LVDS: - ipu_crtc->di_clkflags = IPU_DI_CLKMODE_SYNC | - IPU_DI_CLKMODE_EXT; - break; - case DRM_MODE_ENCODER_TMDS: - case DRM_MODE_ENCODER_NONE: - ipu_crtc->di_clkflags = 0; - break; - } - return 0; }