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

2012-07-04 Thread Daniel Vetter 
On Tue, Jul 03, 2012 at 06:48:16PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@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. 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 paulo.r.zan...@intel.com

All three patches queued for -next, thanks.
-Daniel
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch

2012-07-03 Thread Paulo Zanoni 
From: Paulo Zanoni paulo.r.zan...@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?

Signed-off-by: Paulo Zanoni paulo.r.zan...@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

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

2012-07-03 Thread Daniel Vetter 
On Tue, Jul 03, 2012 at 03:57:31PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@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.zan...@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: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2012-07-03 Thread Paulo Zanoni 
2012/7/3 Daniel Vetter dan...@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(); }.

Thanks,
Paulo

 Otherwise I really like this.
 -Daniel

 Signed-off-by: Paulo Zanoni paulo.r.zan...@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: dan...@ffwll.ch
 Mobile: +41 (0)79 365 57 48



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2012-07-03 Thread Daniel Vetter 
On Tue, Jul 3, 2012 at 10:29 PM, Paulo Zanoni przan...@gmail.com wrote:
 2012/7/3 Daniel Vetter dan...@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.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch

2012-07-03 Thread Paulo Zanoni 
From: Paulo Zanoni paulo.r.zan...@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. 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 paulo.r.zan...@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