Re: [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch

From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch
Date: Tue, 3 Jul 2012 21:10:03 +0200	[thread overview]
Message-ID: <20120703191002.GC5103@phenom.ffwll.local> (raw)
In-Reply-To: <1341341853-4092-1-git-send-email-przanoni@gmail.com>

On Tue, Jul 03, 2012 at 03:57:31PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>
> ---
>  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