From patchwork Thu Apr 18 08:46:09 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 2457951 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork1.kernel.org (Postfix) with ESMTP id 57D0D3FD8C for ; Thu, 18 Apr 2013 08:45:11 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7373DE630A for ; Thu, 18 Apr 2013 01:45:11 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-ee0-f42.google.com (mail-ee0-f42.google.com [74.125.83.42]) by gabe.freedesktop.org (Postfix) with ESMTP id 69020E62EE for ; Thu, 18 Apr 2013 01:43:21 -0700 (PDT) Received: by mail-ee0-f42.google.com with SMTP id e49so435395eek.15 for ; Thu, 18 Apr 2013 01:43:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=x-received:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=xpZ27zgDwaF4t87nUKLI6t6Pa72Rni4U7YzEoEqyybU=; b=QFS0WfyLoaU5a26EKPAGSBtzluHhbP70yUKq76QHcuVyL6oHV55EoMfY1HipnTTMIC R+yWZr+WiMTV72mturMVJSQ7U9nOuKwi6p7NVFF7OMZxTlTQA6m7fv+OgxBohg9w4KFm SVUeomn6AUl/WuuglCn92gilWxFN5+R09hRSA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references:x-gm-message-state; bh=xpZ27zgDwaF4t87nUKLI6t6Pa72Rni4U7YzEoEqyybU=; b=bUT2jZ1+rabUR7lOkQLGGrFZSRYwNSu9ZhNhdltGh3lqOaHDR2zIHWmi2wCxu7tT5k D/pduxuZq+Obi8dBvYTNGfvDvUGBRQeQ34pHc9mN6ILgJkiaD6I3otvYWUFryuIiobMo G2z2iNMAqIpyVnqecwCxf4kipKfuD5sMm3YHJH/CXZEeXlKnQgm3zcwHuVxTu+Zjt7M8 bZx0ZPe4gus783pEcIVANV+5qbUKwyrAGDf4H0s4p/i5e9VuFuCSvxrLmehAddwfq27Y 6yy7tdy2qyNV3AKUYR/5sCh4BohNP87Ndhozm5e7QEIfQKpuv3JzSLEjfyNvnqKOOyZW +0gA== X-Received: by 10.14.110.198 with SMTP id u46mr27919648eeg.41.1366274600332; Thu, 18 Apr 2013 01:43:20 -0700 (PDT) Received: from phenom.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id u44sm14093490eel.7.2013.04.18.01.43.18 (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 18 Apr 2013 01:43:19 -0700 (PDT) From: Daniel Vetter To: Intel Graphics Development Date: Thu, 18 Apr 2013 10:46:09 +0200 Message-Id: <1366274775-10733-2-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1366274775-10733-1-git-send-email-daniel.vetter@ffwll.ch> References: <20130418081740.GE6169@phenom.ffwll.local> <1366274775-10733-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQlemEt02Vwpdp2R9XKcmwBy8259gY/yW5jBRgLpDl7DkMKwZQfOdz665/P1iHtOuMjX0ZvD Cc: Daniel Vetter Subject: [Intel-gfx] [PATCH 2/8] Review comments. X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Imo extracting vlv_enable_pll should be done now, the other stuff is imo ok as follow-up (you could keep the comments as FIXME sections). --- drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 26ef98d..c124dba 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3701,6 +3701,14 @@ static void vlv_pll_pre_enable(struct drm_crtc *crtc) WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock)); /* Assume eDP on port C and HDMI/DP on port B */ + + /* + * Presuming the port macro argument below is indeed the same port the + * above comment claims, the code does not match up with reality. + * + * Second, if you move this into encoder->pre_pll_enable you'll have + * access to the port number directly and can dtrt. + */ if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) { /* Program Tx lane resets to default */ intel_dpio_write(dev_priv, DPIO_PCS_TX(0), @@ -3751,6 +3759,11 @@ static void vlv_pll_post_enable(struct drm_crtc *crtc) WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock)); /* Assume eDP on port C and HDMI/DP on port B */ + + /* + * Same comment as for pll_pre_enable about the port confusion. Again I + * think this can be moved to the encoder->pre_enable. + */ if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) { /* Enable clock channels for this port */ u32 val; @@ -3822,6 +3835,19 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_crtc->active = true; intel_update_watermarks(dev); + /* + * Two issues with these hunks here: + * - it screams for a vlv_enable_pll function + * - you call vlv_pll_post/pre_enable twice. On latest dinq we don't + * enable anything anymore in i9xx_crtc_mode_set, so there's no reason + * any longer to duplicate the pll setup, at least for vlv. i9xx/i8xx + * is a different story, since there we still need to deal with the + * lvds pre_pll_enable hook. So I'm thinking of moving all the crap in + * vlv_update_pll to vlv_enable_pll and only call it from here, plus + * adding a call to encoder->pre_pll_enable to vlv_enable_pll, which + * would do the vlv_pll_pre_enable stuff (but with the right port + * numbers) + */ if (IS_VALLEYVIEW(dev)) { mutex_lock(&dev_priv->dpio_lock); vlv_pll_pre_enable(crtc); @@ -4450,6 +4476,8 @@ static void vlv_update_pll(struct intel_crtc *crtc) mdiv |= ((bestp1 << DPIO_P1_SHIFT) | (bestp2 << DPIO_P2_SHIFT)); mdiv |= ((bestn << DPIO_N_SHIFT)); mdiv |= (1 << DPIO_K_SHIFT); + /* You could replace the EDP || DISPLAYPORT checks here and below with + * crtc->config.has_dp_encoder. */ if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI) || intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_EDP) || intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))