From patchwork Wed Nov 11 09:25:50 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ander Conselvan de Oliveira X-Patchwork-Id: 7594561 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 46BFFBF90C for ; Wed, 11 Nov 2015 09:26:00 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 375B620454 for ; Wed, 11 Nov 2015 09:25:59 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id DBAE920450 for ; Wed, 11 Nov 2015 09:25:57 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B8676E13E; Wed, 11 Nov 2015 01:25:56 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-pa0-f52.google.com (mail-pa0-f52.google.com [209.85.220.52]) by gabe.freedesktop.org (Postfix) with ESMTPS id CE87F6E13E for ; Wed, 11 Nov 2015 01:25:54 -0800 (PST) Received: by pasz6 with SMTP id z6so27290536pas.2 for ; Wed, 11 Nov 2015 01:25:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version:content-transfer-encoding; bh=FO46Rm2yPqBBl20Yv7AGBy/QczaBvIJvJ+OPbipf5zU=; b=wQKHM7o9pNFdKeN/mUkUWl2rc4oq3U1miKXkPhzP9oNQVgaWJ3idLrfmZF5PMD7pL8 MCg7Qh3hn7mw3sf3EM6LpozeZu/u3/2EtDhBDuq+YPhA93JoqM5bxCI5WwnUQ6nhVPsN mVOV/n0leVJdRuSVFTDUBjrAbNnV0UurDYmHBtKTKcljphHkab3KoMCYQSscyOvoTqCX 51LOTmqZN2+PA6FYSF9BdgfTUL5Y9bb1Srv5thiV6Mj+qZisKU8/ImH+kby3F4CP8gHy yNxNCxCHv6ODkupkKNx6lXzuf6bB/S95FKrcAuB20+177bqBaPUoJWObO1Dc/cGIXGqH 2oEg== X-Received: by 10.68.104.2 with SMTP id ga2mr13031416pbb.56.1447233954385; Wed, 11 Nov 2015 01:25:54 -0800 (PST) Received: from aconselv-mobl3.ger.corp.intel.com (jfdmzpr02-ext.jf.intel.com. [134.134.137.71]) by smtp.googlemail.com with ESMTPSA id ij3sm8450229pbb.62.2015.11.11.01.25.52 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Nov 2015 01:25:53 -0800 (PST) Message-ID: <1447233950.3406.22.camel@gmail.com> From: Ander Conselvan De Oliveira To: Jani Nikula , Daniel Vetter , "Lankhorst, Maarten" Date: Wed, 11 Nov 2015 11:25:50 +0200 In-Reply-To: <87si4ehv7c.fsf@intel.com> References: <1443022487-6259-1-git-send-email-gabriel.feceoru@intel.com> <561D0498.7090307@linux.intel.com> <20151013133501.GX26718@phenom.ffwll.local> <561D0A80.7050503@linux.intel.com> <20151013135830.GF26718@phenom.ffwll.local> <561D0E85.2020809@linux.intel.com> <20151013140759.GG26718@phenom.ffwll.local> <1444810906.2636.4.camel@gmail.com> <20151014124401.GZ26718@phenom.ffwll.local> <1444831135.2636.27.camel@gmail.com> <20151014150302.GH26718@phenom.ffwll.local> <87si4ehv7c.fsf@intel.com> X-Mailer: Evolution 3.16.5 (3.16.5-3.fc22) Mime-Version: 1.0 Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable 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, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: > On Wed, 14 Oct 2015, Daniel Vetter wrote: > > On Wed, Oct 14, 2015 at 04:58:55PM +0300, Ander Conselvan De Oliveira wrote: > > > On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote: > > > > On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira > > > > wrote: > > > > > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > > > > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > > > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst > > > > > > > > wrote: > > > > > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst > > > > > > > > > > wrote: > > > > > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be > > > > > > > > > > > > set for > > > > > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to > > > > > > > > > > > > INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch > > > > > > > > > > > > in dpll_hw_state.wrpll > > > > > > > > > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > > > > > Except that we still have a bunch of cases where we > > > > > > > > > > recompute clock state > > > > > > > > > > but only partially. Can we just move them all up into a > > > > > > > > > > common place > > > > > > > > > > please? That would also catch cases where we simply forget > > > > > > > > > > to fill this > > > > > > > > > > out at all. > > > > > > > > > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's > > > > > > > > > > probably > > > > > > > > > > more. > > > > > > > > > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state > > > > > > > > > removed? > > > > > > > > I think this will blow up since we recompute clock state only > > > > > > > > when > > > > > > > > needs_modeset is true. So needs a bit more intelligence in > > > > > > > > deciding when > > > > > > > > to clear it I think. > > > > > > > Oops you're right. Maybe intel_modeset_clear_plls because that's > > > > > > > where all the clock state > > > > > > > belongs? > > > > > > > > > > > > Yeah that might be an even better place, in the loop after the > > > > > > continue; > > > > > > statement. > > > > > > > > > > The reason I didn't put the memset there in the first place was the > > > > > way we calculate plls for DP > > > > > with DDI platforms. In that case, ddi_pll_sel is setup from the > > > > > encoder_config instead of > > > > > compute_clock, so a memset ends up clearing the new pll config. > > > > > > > > Hm, I forgot about this split totally. And there seems to be a giant > > > > mess > > > > going on here: > > > > > > > > In our top-level intel_atomic_check we have 4 parts to compute state: > > > > 1. drm_atomic_helper_check_modeset > > > > 2. intel_modeset_pipe_config > > > > 3. intel_modeset_checks > > > > 4. drm_atomic_helper_check_planes > > > > > > > > We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) > > > > in 1., way ahead of anything else in intel_crtc_atomic_check. That looks > > > > very suspcious since it means only very later on (in the loop that does > > > > 2.) do we even decide whether we need to do a full modeset or not. > > > > > > > > So what I had in mind is that we clear clocks in > > > > intel_modeset_pipe_config, before we call any of the callbacks. That > > > > makes > > > > sure that when we decided to do a modeset, we do recompute the clocks > > > > correctly. > > > > > > I had a suspicion this would interact badly with how we "cancel" the > > > modeset if the pipe config > > > didn't changed, just after the call to intel_modeset_pipe_config(). It > > > turns out there's an issue > > > there already. > > > > > > There are two possibilities for the dpll_hw_state value after the new > > > pipe_config is calculated. It > > > may have the new values already for DP in HSW/BDW and eDP in SKL or it may > > > still have the old value. > > > In the latter case the new value is only calculated in .crtc_clock(), > > > after we already compared the > > > old and new configs and may have decided to skip the modeset. > > > > > > But doing the memset() in intel_modeset_pipe_config() would be find as > > > long as we don't change our > > > minds about doing a modeset later. > > > > It's more annoying since my analysis is all wrong: intel_crtc_atomic_check > > is called from drm_atomic_helper_check_planes, i.e. step 4 not step 1. > > It'll all work out I think if we memset it in intel_modeset_pipe_config. > > The caveat is that we need to move the clock recomputation into > > intel_modeset_pipe_config too (which is better, since then we'll have more > > accurate state to decided whether we'll fastboot or not). > > > > And then intel_modeset_clear_plls would really just update the global pll > > setup (and would be really good to rename it to > > intel_modeset_compute_shared_dpll or whatever). > > > > Thoughts? > > Ander, Maarten, where are we with this? Is it horribly wrong to merge > the original patch in this ever-growing and diverging thread? I think the patch as is will cause problems with DP, since we might clear the pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix disregarding the discussion in this thread is to drop another memset in intel_crt_compute_config(). Like this } Ander diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index b84aaa0..ad099f3 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -278,6 +278,9 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder, /* FDI must always be 2.7 GHz */ if (HAS_DDI(dev)) { + memset(&pipe_config->dpll_hw_state, 0, + sizeof(pipe_config->dpll_hw_state)); + pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; pipe_config->port_clock = 135000 * 2;