From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch Date: Tue, 3 Jul 2012 21:10:03 +0200 Message-ID: <20120703191002.GC5103@phenom.ffwll.local> References: <1341341853-4092-1-git-send-email-przanoni@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f169.google.com (mail-wi0-f169.google.com [209.85.212.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 8DA03A0872 for ; Tue, 3 Jul 2012 12:10:06 -0700 (PDT) Received: by wibhm2 with SMTP id hm2so3742720wib.0 for ; Tue, 03 Jul 2012 12:10:05 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1341341853-4092-1-git-send-email-przanoni@gmail.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: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Tue, Jul 03, 2012 at 03:57:31PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > And rely on the fact that it's 0 to assume that machines without a PCH > will have PCH_NONE as dev_priv->pch_type. > > Just today I finally realized that HAS_PCH_IBX is true for machines > without a PCH. IMHO this is totally counter-intuitive and I don't > think it's a good idea to assume that we're going to check for > HAS_PCH_IBX only after we check for HAS_PCH_SPLIT. > > I believe that in the future we'll have more PCH types and checks > like: > > if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) > > will become more and more common. There's a good chance that we may > break non-PCH machines by adding these checks in code that runs on all > machines. I also believe that the HAS_PCH_SPLIT check will become less > common as we add more and more different PCH types. > > Also: are we sure we don't already have any bugs triggered by checking > for HAS_PCH_IBX on non-PCH machines? I think most of the HAS_PCH_xxx are implicitly guarded because we've split up the pch modeset into it's own functions. I think there might only be a few issues in the encoder functions maybe. Have your checked all the HAS_PCH_IBX checks there? If you want, I can go through the code, too. Otherwise I really like this. -Daniel > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > 1 file changed, 1 insertion(+) > > Another alternative would have been to change HAS_PCH_IBX to also > check for HAS_PCH_SPLIT, but I'm not exactly in favor of adding more > conditionals... > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b7a1eaa..b12e79a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -333,6 +333,7 @@ enum no_fbc_reason { > }; > > enum intel_pch { > + PCH_NONE = 0, /* No PCH present */ > PCH_IBX, /* Ibexpeak PCH */ > PCH_CPT, /* Cougarpoint PCH */ > PCH_LPT, /* Lynxpoint PCH */ > -- > 1.7.10.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48