From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt Date: Tue, 5 Mar 2013 16:48:55 +0100 Message-ID: <20130305154855.GJ9021@phenom.ffwll.local> References: <1362489888-32651-1-git-send-email-patrik.r.jakobsson@gmail.com> <20130305134530.GU4469@intel.com> <20130305145912.GW4469@intel.com> <20130305152359.GH9021@phenom.ffwll.local> <20130305154103.GX4469@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wg0-f45.google.com (mail-wg0-f45.google.com [74.125.82.45]) by gabe.freedesktop.org (Postfix) with ESMTP id 06ABAE5DB8 for ; Tue, 5 Mar 2013 07:46:25 -0800 (PST) Received: by mail-wg0-f45.google.com with SMTP id dq12so6025127wgb.12 for ; Tue, 05 Mar 2013 07:46:25 -0800 (PST) Content-Disposition: inline In-Reply-To: <20130305154103.GX4469@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Mar 05, 2013 at 05:41:04PM +0200, Ville Syrj=E4l=E4 wrote: > On Tue, Mar 05, 2013 at 04:23:59PM +0100, Daniel Vetter wrote: > > On Tue, Mar 05, 2013 at 04:59:12PM +0200, Ville Syrj=E4l=E4 wrote: > > > On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote: > > > > > Accroding to the docs these bits don't exist on PCH platforms. > > > > > intel_crt_dpms() already has a check for this, so I suppose > > > > > intel_disable_crt() should have one too. > > > > > > > > > > Also I noticed that we seem to have the hsync and vsync disable > > > > > bits reversed. At least that's what the docs are telling me. > > > > = > > > > The PCH check just forces suspend and standby to off and we're only= doing dpms > > > > off in intel_disable_crt() so no need to check it there. > > > = > > > You're right. I assumed that the check would somehow avoid setting > > > these bits too, but it doesn't. So I guess we don't really care > > > that they don't exist. > > = > > The dpms state gets clamped to the values support by the hw in > > intel_crt_dpms. So I think we should care also in intel_crt_disable. > = > The point was that in intel_crt_dpms() we don't care whether the > hsync/vsync disable bits actually exist. We just set them whenever > the dpms mode warrants it. So for "off" we always set both bits, and > "off" is always supported. And intel_crt_disable() is equal to > intel_crt_dpms(DRM_MODE_DPMS_OFF) so the behaviour is consistent > across the board. > = > Whether or not there could be side effects from setting those bits > on PCH plaforms is another matter. If there are, then the clamping > stuff is not enough and we need to add PCH checks to both functions. Oh right, I've missed that when reworking that code - we set the bits in the OFF case ... I guess that'd be worthwile to amend, if just to keep out any future surprises. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch