From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 4/5] drm/i915: take power well refs when needed Date: Tue, 15 Oct 2013 13:57:00 -0700 Message-ID: <20131015135700.3aaba36c@jbarnes-desktop> References: <1381792069-27800-1-git-send-email-jbarnes@virtuousgeek.org> <1381792069-27800-5-git-send-email-jbarnes@virtuousgeek.org> <20131015134044.04cc49de@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy9-pub.mail.unifiedlayer.com (oproxy9-pub.mail.unifiedlayer.com [69.89.24.6]) by gabe.freedesktop.org (Postfix) with SMTP id 53B19E64F7 for ; Tue, 15 Oct 2013 13:56:51 -0700 (PDT) In-Reply-To: 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: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Tue, 15 Oct 2013 17:47:20 -0300 Paulo Zanoni wrote: > 2013/10/15 Jesse Barnes : > > On Tue, 15 Oct 2013 16:54:00 -0300 > > Paulo Zanoni wrote: > > > >> 2013/10/14 Jesse Barnes : > >> > When accessing the display regs for hw state readout or cross check, we > >> > need to make sure the power well is enabled so we can read valid > >> > register state. > >> > >> On the current code (HSW) we already check for the power wells in the > >> HW state readout code: if the power well is disabled, then transcoders > >> A/B/C are disabled. The current code takes care to not touch registers > >> on a disabled power well. > > > > Yeah and we should probably preserve that behavior, for the readout > > code at least. > > > >> > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); > >> > + > >> > >> Why is this here? It certainly deserves a comment in the code. > > > > It probably needs to be removed. The refcounting for the initial load > > is still screwy due to the unconditional enable later on. > > > >> So now HSW uses global_resources while VLV uses crtc enable/disable. I > >> really think both platforms should try to do the same thing. > > > > Definitely agreed. > > > >> By the way, at least on Haswell, if we do an equivalent change we'll > >> have problems, because when you disable the power well at > >> crtc_disable, then everything you did at crtc_mode_set will be undone, > >> and it won't be redone at crtc_enable. When you reenable the power > >> well, the registers go back to their default values, not the values > >> that were previously there. Did you check if VLV behaves the same? > > > > No that's taken into account here. In __intel_set_mode we take a > > private ref on the appropriate power well so that we'll preserve state > > until we do the first crtc_enable. From then on, the ref is tracked > > there and we drop the private one in __intel_set_mode > > Yeah, but then after all this is done, at some point we'll call > crtc_disable, then we'll put the last ref back and then the power well > will be disabled. Then the user moves the mouse and we call > crtc_enable, so we enable the power well, all its registers to back to > their reset state, then crtc_enable sets some of the registers, but > that won't really be enough since no one called crtc_mode_set again, > and all the state set by crtc_mode_set (not touched by crtc_enable) is > back to the default values. No? What am I missing? That's where the save/restore state of the later patch comes in. We need that if we're going to support DPMS and runtime suspend w/o a mode set. -- Jesse Barnes, Intel Open Source Technology Center