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

Commit Message

Paulo Zanoni July 3, 2012, 6:57 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.

Also: are we sure we don't already have any bugs triggered by checking
for HAS_PCH_IBX on non-PCH machines?

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 3, 2012, 7:10 p.m. UTC | #1

On Tue, Jul 03, 2012 at 03:57:31PM -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.
> 
> 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 <[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...
> 
> 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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni July 3, 2012, 8:29 p.m. UTC | #2

2012/7/3 Daniel Vetter <[e-mail]>:
> 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(); }".

Thanks,
Paulo

> Otherwise I really like this.
> -Daniel
>>
>> 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...
>>
>> 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
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Mail: [e-mail]
> Mobile: [phone]

Daniel Vetter July 3, 2012, 8:39 p.m. UTC | #3

On Tue, Jul 3, 2012 at 10:29 PM, Paulo Zanoni <[e-mail]> wrote:
> 2012/7/3 Daniel Vetter <[e-mail]>:
>> 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

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