From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 0/7] modeset rework prep patches Date: Sun, 12 Aug 2012 21:12:14 +0100 Message-ID: <1344802342_81385@CP5-2952> References: <1344792434-1316-1-git-send-email-daniel.vetter@ffwll.ch> <1344800880_81295@CP5-2952> <20120812200102.GJ5575@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (smtp.fireflyinternet.com [109.228.6.236]) by gabe.freedesktop.org (Postfix) with ESMTP id E4F569E933 for ; Sun, 12 Aug 2012 13:12:25 -0700 (PDT) In-Reply-To: <20120812200102.GJ5575@phenom.ffwll.local> 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: Daniel Vetter Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Sun, 12 Aug 2012 22:01:02 +0200, Daniel Vetter wrote: > On Sun, Aug 12, 2012 at 08:47:56PM +0100, Chris Wilson wrote: > > On Sun, 12 Aug 2012 19:27:07 +0200, Daniel Vetter wrote: > > > drm/i915: prepare load-detect pipe code for dpms changes > > > > It is not immediately obvious from the function that there is a > > relationship between the connector and intel_encoder. If we derived the > > encoder from the connector in that function, the reviewer's life gets a > > little easier. As it stands the code looks correct and rightly removes > > some internal details. > > Hm, good point. I'll add a patch on top that drops the intel_encoder > argument (since it's redudant, all callers get it with > intel_attached_encoder). Or better if I squash it together with this one? I'll buy it either way, with a slight preference to having it as two steps. > > > drm/i915: simplify dvo dpms interface > > > > This just looks like churn for churn's sake? The changes look correct. > > We don't bother with anything else than dpms on/off states in most of the > modeset code (even for crt newer hw drops the intermediate states). Hence > the new interfaces have only enable/disable functions at the encoder/crtc > level. I've figured it looks odd if we keep the full dpms interface for > dvo. But since it's rather independant churn I've moved it into this > odds bits series. The full fledged dpms mode isn't going to completely disappear thanks to the CRT dinosaur. I just wonder if we can achieve the same simplification by recognising that all non-zero dpms modes are off, e.g. s/dpms_mode/powersave/ if (powersave) switch_off() else switch_on() -Chris -- Chris Wilson, Intel Open Source Technology Centre