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

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

[Paulo Zanoni]

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?

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

[PATCH 2/3] drm/i915: get rid of dev_priv->info->has_pch_split

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Previously we had has_pch_split to tell us whether we had a PCH or not
and we also had dev_priv->pch_type to tell us which kind of PCH it
was, but it could only be used if we were 100% sure we did have a PCH.
Now that PCH_NONE was added to dev_priv->pch_type we don't need
has_pch_split anymore: we can just check for pch_type != PCH_NONE.

The HAS_PCH_{IBX,CPT,LPT} macros use dev_priv->pch_type, so they can
only be called after intel_detect_pch. The HAS_PCH_SPLIT macro looks
at dev_priv->info->has_pch_split, which is available earlier.

Since the goal is to implement HAS_PCH_SPLIT using dev_priv->pch_type
instead of dev_priv->info->has_pch_split, we need to make sure that
intel_detect_pch is called before any calls to HAS_PCH_SPLIT are made.
So we moved the intel_detect_pch call to an earlier stage.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |    5 +++--
 drivers/gpu/drm/i915/i915_drv.c |    8 --------
 drivers/gpu/drm/i915/i915_drv.h |    3 +--
 3 files changed, 4 insertions(+), 12 deletions(-)

This patch does not solve any real problem: it's just a "suggestion"
of something we could do after the previous patch. Some people may
argue that looking at the has_pch_split variable might make it easier
for us to find out which machines actually have a pch split without
running the machine. So I really won't complain if we don't accept
this patch: patch 01 is the important one.

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2166519..f8bc9ea 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1547,6 +1547,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto out_mtrrfree;
 	}
 
+	/* This must be called before any calls to HAS_PCH_* */
+	intel_detect_pch(dev);
+
 	intel_irq_init(dev);
 
 	/* Try to make sure MCHBAR is enabled before poking at it */
@@ -1599,8 +1602,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	/* Start out suspended */
 	dev_priv->mm.suspended = 1;
 
-	intel_detect_pch(dev);
-
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
 		if (ret < 0) {
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7d0eb82..1794833 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -225,7 +225,6 @@ static const struct intel_device_info intel_ironlake_d_info = {
 	.gen = 5,
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_bsd_ring = 1,
-	.has_pch_split = 1,
 };
 
 static const struct intel_device_info intel_ironlake_m_info = {
@@ -233,7 +232,6 @@ static const struct intel_device_info intel_ironlake_m_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_fbc = 1,
 	.has_bsd_ring = 1,
-	.has_pch_split = 1,
 };
 
 static const struct intel_device_info intel_sandybridge_d_info = {
@@ -242,7 +240,6 @@ static const struct intel_device_info intel_sandybridge_d_info = {
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
 	.has_llc = 1,
-	.has_pch_split = 1,
 	.has_force_wake = 1,
 };
 
@@ -253,7 +250,6 @@ static const struct intel_device_info intel_sandybridge_m_info = {
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
 	.has_llc = 1,
-	.has_pch_split = 1,
 	.has_force_wake = 1,
 };
 
@@ -263,7 +259,6 @@ static const struct intel_device_info intel_ivybridge_d_info = {
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
 	.has_llc = 1,
-	.has_pch_split = 1,
 	.has_force_wake = 1,
 };
 
@@ -274,7 +269,6 @@ static const struct intel_device_info intel_ivybridge_m_info = {
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
 	.has_llc = 1,
-	.has_pch_split = 1,
 	.has_force_wake = 1,
 };
 
@@ -302,7 +296,6 @@ static const struct intel_device_info intel_haswell_d_info = {
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
 	.has_llc = 1,
-	.has_pch_split = 1,
 	.has_force_wake = 1,
 };
 
@@ -312,7 +305,6 @@ static const struct intel_device_info intel_haswell_m_info = {
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
 	.has_llc = 1,
-	.has_pch_split = 1,
 	.has_force_wake = 1,
 };
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b12e79a..89025ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -285,7 +285,6 @@ struct intel_device_info {
 	u8 is_crestline:1;
 	u8 is_ivybridge:1;
 	u8 is_valleyview:1;
-	u8 has_pch_split:1;
 	u8 has_force_wake:1;
 	u8 is_haswell:1;
 	u8 has_fbc:1;
@@ -1113,13 +1112,13 @@ struct drm_i915_file_private {
 #define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr)
 #define I915_HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc)
 
-#define HAS_PCH_SPLIT(dev) (INTEL_INFO(dev)->has_pch_split)
 #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5)
 
 #define INTEL_PCH_TYPE(dev) (((struct drm_i915_private *)(dev)->dev_private)->pch_type)
 #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
 #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
 #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
+#define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
 
 #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
 
-- 
1.7.10.2

[PATCH 3/3] drm/i915: don't ironlake_init_pch_refclk() on LPT

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This function is used to set the PCH_DREF_CONTROL register, which does
not exist on LPT anymore.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

My crystal ball tells me patches like this will become more and more
common in the future... That's why patch 01 was proposed.

This patch depends on patch 01, otherwise we'll start calling
ironlake_init_pch_refclk on non-pch machines.

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1794833..0630471 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -687,7 +687,7 @@ static int i915_drm_thaw(struct drm_device *dev)
 
 	/* KMS EnterVT equivalent */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		if (HAS_PCH_SPLIT(dev))
+		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
 			ironlake_init_pch_refclk(dev);
 
 		mutex_lock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8cd5032..da33911 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6855,7 +6855,7 @@ static void intel_setup_outputs(struct drm_device *dev)
 	/* disable all the possible outputs/crtcs before entering KMS mode */
 	drm_helper_disable_unused_functions(dev);
 
-	if (HAS_PCH_SPLIT(dev))
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
 		ironlake_init_pch_refclk(dev);
 }
 
-- 
1.7.10.2

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

[Daniel Vetter]

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

Re: [PATCH 2/3] drm/i915: get rid of dev_priv->info->has_pch_split

[Daniel Vetter]

On Tue, Jul 03, 2012 at 03:57:32PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Previously we had has_pch_split to tell us whether we had a PCH or not
> and we also had dev_priv->pch_type to tell us which kind of PCH it
> was, but it could only be used if we were 100% sure we did have a PCH.
> Now that PCH_NONE was added to dev_priv->pch_type we don't need
> has_pch_split anymore: we can just check for pch_type != PCH_NONE.
> 
> The HAS_PCH_{IBX,CPT,LPT} macros use dev_priv->pch_type, so they can
> only be called after intel_detect_pch. The HAS_PCH_SPLIT macro looks
> at dev_priv->info->has_pch_split, which is available earlier.
> 
> Since the goal is to implement HAS_PCH_SPLIT using dev_priv->pch_type
> instead of dev_priv->info->has_pch_split, we need to make sure that
> intel_detect_pch is called before any calls to HAS_PCH_SPLIT are made.
> So we moved the intel_detect_pch call to an earlier stage.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    5 +++--
>  drivers/gpu/drm/i915/i915_drv.c |    8 --------
>  drivers/gpu/drm/i915/i915_drv.h |    3 +--
>  3 files changed, 4 insertions(+), 12 deletions(-)
> 
> This patch does not solve any real problem: it's just a "suggestion"
> of something we could do after the previous patch. Some people may
> argue that looking at the has_pch_split variable might make it easier
> for us to find out which machines actually have a pch split without
> running the machine. So I really won't complain if we don't accept
> this patch: patch 01 is the important one.
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2166519..f8bc9ea 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1547,6 +1547,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto out_mtrrfree;
>  	}
>  
> +	/* This must be called before any calls to HAS_PCH_* */
> +	intel_detect_pch(dev);
> +

Hm, what about defining PCH_NONE as -1, PCH_RESERVED as 0 and then adding
a WARN_ON(dev_priv->pch_type == PCH_RESERVED)? detect_pch is called
unconditionally, and that way we would catch this. Might be overkill otoh,
so if you think this is not worth it, np.
-Daniel

>  	intel_irq_init(dev);
>  
>  	/* Try to make sure MCHBAR is enabled before poking at it */
> @@ -1599,8 +1602,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	/* Start out suspended */
>  	dev_priv->mm.suspended = 1;
>  
> -	intel_detect_pch(dev);
> -
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		ret = i915_load_modeset_init(dev);
>  		if (ret < 0) {
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 7d0eb82..1794833 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -225,7 +225,6 @@ static const struct intel_device_info intel_ironlake_d_info = {
>  	.gen = 5,
>  	.need_gfx_hws = 1, .has_hotplug = 1,
>  	.has_bsd_ring = 1,
> -	.has_pch_split = 1,
>  };
>  
>  static const struct intel_device_info intel_ironlake_m_info = {
> @@ -233,7 +232,6 @@ static const struct intel_device_info intel_ironlake_m_info = {
>  	.need_gfx_hws = 1, .has_hotplug = 1,
>  	.has_fbc = 1,
>  	.has_bsd_ring = 1,
> -	.has_pch_split = 1,
>  };
>  
>  static const struct intel_device_info intel_sandybridge_d_info = {
> @@ -242,7 +240,6 @@ static const struct intel_device_info intel_sandybridge_d_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
> -	.has_pch_split = 1,
>  	.has_force_wake = 1,
>  };
>  
> @@ -253,7 +250,6 @@ static const struct intel_device_info intel_sandybridge_m_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
> -	.has_pch_split = 1,
>  	.has_force_wake = 1,
>  };
>  
> @@ -263,7 +259,6 @@ static const struct intel_device_info intel_ivybridge_d_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
> -	.has_pch_split = 1,
>  	.has_force_wake = 1,
>  };
>  
> @@ -274,7 +269,6 @@ static const struct intel_device_info intel_ivybridge_m_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
> -	.has_pch_split = 1,
>  	.has_force_wake = 1,
>  };
>  
> @@ -302,7 +296,6 @@ static const struct intel_device_info intel_haswell_d_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
> -	.has_pch_split = 1,
>  	.has_force_wake = 1,
>  };
>  
> @@ -312,7 +305,6 @@ static const struct intel_device_info intel_haswell_m_info = {
>  	.has_bsd_ring = 1,
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
> -	.has_pch_split = 1,
>  	.has_force_wake = 1,
>  };
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b12e79a..89025ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -285,7 +285,6 @@ struct intel_device_info {
>  	u8 is_crestline:1;
>  	u8 is_ivybridge:1;
>  	u8 is_valleyview:1;
> -	u8 has_pch_split:1;
>  	u8 has_force_wake:1;
>  	u8 is_haswell:1;
>  	u8 has_fbc:1;
> @@ -1113,13 +1112,13 @@ struct drm_i915_file_private {
>  #define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr)
>  #define I915_HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc)
>  
> -#define HAS_PCH_SPLIT(dev) (INTEL_INFO(dev)->has_pch_split)
>  #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5)
>  
>  #define INTEL_PCH_TYPE(dev) (((struct drm_i915_private *)(dev)->dev_private)->pch_type)
>  #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
>  #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
>  #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
> +#define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
>  
>  #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
>  
> -- 
> 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

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

[Paulo Zanoni]

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(); }".

Thanks,
Paulo

> 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



-- 
Paulo Zanoni

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

[Daniel Vetter]

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

Re: [PATCH 2/3] drm/i915: get rid of dev_priv->info->has_pch_split

[Paulo Zanoni]

2012/7/3 Daniel Vetter <daniel@ffwll.ch>:
> Hm, what about defining PCH_NONE as -1, PCH_RESERVED as 0 and then adding
> a WARN_ON(dev_priv->pch_type == PCH_RESERVED)? detect_pch is called
> unconditionally, and that way we would catch this. Might be overkill otoh,
> so if you think this is not worth it, np.

I actually thought about this idea before. But it would only make
sense if we add these WARNs to the HAS_PCH_FOO macros. But these
macros are supposed to be simple and cheap and fast... Do we really
want to start adding assertions inside them? If we think the price is
worth paying, then we might do as you suggested. Or maybe this could
be inside some #ifdef DEBUG...

On my local machines, I changed the HAS_PCH_FOO macros to print some
stuff so I could check whether any of them was called before
intel_detect_pch. At least on these machines (SNB, HSW and a netbook),
everything was fine.

> -Daniel
>
>>       intel_irq_init(dev);
>>
>>       /* Try to make sure MCHBAR is enabled before poking at it */
>> @@ -1599,8 +1602,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>       /* Start out suspended */
>>       dev_priv->mm.suspended = 1;
>>
>> -     intel_detect_pch(dev);
>> -
>>       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>               ret = i915_load_modeset_init(dev);
>>               if (ret < 0) {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 7d0eb82..1794833 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -225,7 +225,6 @@ static const struct intel_device_info intel_ironlake_d_info = {
>>       .gen = 5,
>>       .need_gfx_hws = 1, .has_hotplug = 1,
>>       .has_bsd_ring = 1,
>> -     .has_pch_split = 1,
>>  };
>>
>>  static const struct intel_device_info intel_ironlake_m_info = {
>> @@ -233,7 +232,6 @@ static const struct intel_device_info intel_ironlake_m_info = {
>>       .need_gfx_hws = 1, .has_hotplug = 1,
>>       .has_fbc = 1,
>>       .has_bsd_ring = 1,
>> -     .has_pch_split = 1,
>>  };
>>
>>  static const struct intel_device_info intel_sandybridge_d_info = {
>> @@ -242,7 +240,6 @@ static const struct intel_device_info intel_sandybridge_d_info = {
>>       .has_bsd_ring = 1,
>>       .has_blt_ring = 1,
>>       .has_llc = 1,
>> -     .has_pch_split = 1,
>>       .has_force_wake = 1,
>>  };
>>
>> @@ -253,7 +250,6 @@ static const struct intel_device_info intel_sandybridge_m_info = {
>>       .has_bsd_ring = 1,
>>       .has_blt_ring = 1,
>>       .has_llc = 1,
>> -     .has_pch_split = 1,
>>       .has_force_wake = 1,
>>  };
>>
>> @@ -263,7 +259,6 @@ static const struct intel_device_info intel_ivybridge_d_info = {
>>       .has_bsd_ring = 1,
>>       .has_blt_ring = 1,
>>       .has_llc = 1,
>> -     .has_pch_split = 1,
>>       .has_force_wake = 1,
>>  };
>>
>> @@ -274,7 +269,6 @@ static const struct intel_device_info intel_ivybridge_m_info = {
>>       .has_bsd_ring = 1,
>>       .has_blt_ring = 1,
>>       .has_llc = 1,
>> -     .has_pch_split = 1,
>>       .has_force_wake = 1,
>>  };
>>
>> @@ -302,7 +296,6 @@ static const struct intel_device_info intel_haswell_d_info = {
>>       .has_bsd_ring = 1,
>>       .has_blt_ring = 1,
>>       .has_llc = 1,
>> -     .has_pch_split = 1,
>>       .has_force_wake = 1,
>>  };
>>
>> @@ -312,7 +305,6 @@ static const struct intel_device_info intel_haswell_m_info = {
>>       .has_bsd_ring = 1,
>>       .has_blt_ring = 1,
>>       .has_llc = 1,
>> -     .has_pch_split = 1,
>>       .has_force_wake = 1,
>>  };
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index b12e79a..89025ab 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -285,7 +285,6 @@ struct intel_device_info {
>>       u8 is_crestline:1;
>>       u8 is_ivybridge:1;
>>       u8 is_valleyview:1;
>> -     u8 has_pch_split:1;
>>       u8 has_force_wake:1;
>>       u8 is_haswell:1;
>>       u8 has_fbc:1;
>> @@ -1113,13 +1112,13 @@ struct drm_i915_file_private {
>>  #define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr)
>>  #define I915_HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc)
>>
>> -#define HAS_PCH_SPLIT(dev) (INTEL_INFO(dev)->has_pch_split)
>>  #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5)
>>
>>  #define INTEL_PCH_TYPE(dev) (((struct drm_i915_private *)(dev)->dev_private)->pch_type)
>>  #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
>>  #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
>>  #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
>> +#define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
>>
>>  #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
>>
>> --
>> 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



-- 
Paulo Zanoni

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

[Paulo Zanoni]

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. 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.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

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

[Daniel Vetter]

On Tue, Jul 03, 2012 at 06:48:16PM -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. 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.zanoni@intel.com>

All three patches queued for -next, thanks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48