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
Subject: Re: [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch
Date: Tue, 3 Jul 2012 22:39:18 +0200	[thread overview]
Message-ID: <CAKMK7uEGQMnhda-h8rKZor5x0usmTZFLRj_tTTM8xpnPM3LkWw@mail.gmail.com> (raw)
In-Reply-To: <CA+gsUGSjuJv-3+-+AXDYck8gx6SYYzG2WzmgZh6bb0H-MDyFHQ@mail.gmail.com>

On Tue, Jul 3, 2012 at 10:29 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/7/3 Daniel Vetter <daniel@ffwll.ch>:
>> 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.
>>
>
> I did check. At the moment we have just a few HAS_PCH_IBX calls in our
> driver. The only possible issues may be inside intel_hdmi.c and
> intel_dp.c (and they need more investigation).
>
> My biggest worry here is being "future-proof": are we sure whenever
> someone suggests adding HAS_PCH_IBX we'll remember that machines
> without a PCH return true for HAS_PCH_IBX? This is highly
> counter-intuitive. I really think that in future hardware enablement
> code we'll replace a lot of the "if (HAS_PCH_SPLIT) { foo(); } else {
> bar(); }" code for "if (HAS_PCH_NEW) { baz(); } else if (HAS_PCH_OLD)
> { foo(); } else { bar(); }".

Ok, I've quickly checked them. The one in intel_dp.c isn't an issue,
because DP is a gen5+ feature. So the only thing accidentally affected
is vlv, which isn't such a big deal ;-)

The only other check that isn't guarded with a HAS_PCH_SPLIT check is
in intel_hdmi.c for a ibx-only w/a. That one will also leak out into
gm45 platforms (which support hdmi, too).

Otherwise I haven't found anything. Can you please amend the commit
message detailing the effects on these two places? Just in case a
bisect hits this patch and someone is totally confused what's going on
here ...
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch