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

Commit Message

Paulo Zanoni July 3, 2012, 9:48 p.m. UTC

From: Paulo Zanoni <[e-mail]>

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. We'll probably
start replacing checks like:

    if (HAS_PCH_SPLIT(dev))
        foo();
    else
        bar();

with:

    if (HAS_PCH_NEW(dev))
        baz();
    else if (HAS_PCH_OLD(dev) || HAS_PCH_IBX(dev))
        foo();
    else
        bar();

and this may break gen 2/3/4.

As far as we have investigated, this patch will affect the behavior of
intel_hdmi_dpms and intel_dp_link_down on gen 4. In both functions the
code inside the HAS_PCH_IBX check is for IBX-specific workarounds, so
we should be safe. If we start bisecting gen 2/3/4 bugs to this commit
we should consider replacing the HAS_PCH_IBX checks with something
else.

V2: Improve commit message, list possible side effects and solution.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 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...

Comments

Daniel Vetter July 4, 2012, 7:49 a.m. UTC | #1

On Tue, Jul 03, 2012 at 06:48:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> 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. We'll probably
> start replacing checks like:
> 
>     if (HAS_PCH_SPLIT(dev))
>         foo();
>     else
>         bar();
> 
> with:
> 
>     if (HAS_PCH_NEW(dev))
>         baz();
>     else if (HAS_PCH_OLD(dev) || HAS_PCH_IBX(dev))
>         foo();
>     else
>         bar();
> 
> and this may break gen 2/3/4.
> 
> As far as we have investigated, this patch will affect the behavior of
> intel_hdmi_dpms and intel_dp_link_down on gen 4. In both functions the
> code inside the HAS_PCH_IBX check is for IBX-specific workarounds, so
> we should be safe. If we start bisecting gen 2/3/4 bugs to this commit
> we should consider replacing the HAS_PCH_IBX checks with something
> else.
> 
> V2: Improve commit message, list possible side effects and solution.
> 
> Signed-off-by: Paulo Zanoni <[e-mail]>

All three patches queued for -next, thanks.
-Daniel

Patch

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 */