[PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Paulo Zanoni]

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

Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
state doesn't affect the result of reading the current HW state.

This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
HSW and then suspend/resume.

Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
Tested-by: Qingshuai Tian <qingshuai.tian@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 4ec1665..0def5ef 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
 	enum pipe pipe;
 	struct intel_crtc *intel_crtc;
 
+	dev_priv->ddi_plls.spll_refcount = 0;
+	dev_priv->ddi_plls.wrpll1_refcount = 0;
+	dev_priv->ddi_plls.wrpll2_refcount = 0;
+
 	for_each_pipe(pipe) {
 		intel_crtc =
 			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 
-		if (!intel_crtc->active)
+		if (!intel_crtc->active) {
+			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_NONE;
 			continue;
+		}
 
 		intel_crtc->ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
 								 pipe);
-- 
1.8.4.2

[PATCH 2/2] drm/i915: fix wrong PLL debug messages.

[Paulo Zanoni]

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

LPT does have PCH refclk, but it's different form the IBX/CPT/PPT one
and doesn't use the same structs. It is wrong to have a message saying
that "LPT does not has PCH refclk" (sic). While at it, signal that we
only want this function on IBX/CPT/PPT by renaming it and adding a
WARN.

On HSW we also print "0 shared PLLs initialized", but we *do* have
shared PLLs on HSW (LCPLL, WRPLL, SPLL) and we *do* initialize them.
We just don't use "struct intel_shared_dpll". So remove the debug
message.

In the future we may want to rename all that "intel shared pll" code
to "ibx shared pll", but I'll leave this to another patch.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 034952c..df3cf69 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1211,15 +1211,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
+static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
 {
 	u32 val;
 	bool enabled;
 
-	if (HAS_PCH_LPT(dev_priv->dev)) {
-		DRM_DEBUG_DRIVER("LPT does not has PCH refclk, skipping check\n");
-		return;
-	}
+	WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev)));
 
 	val = I915_READ(PCH_DREF_CONTROL);
 	enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK |
@@ -10067,7 +10064,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll)
 {
 	/* PCH refclock must be enabled first */
-	assert_pch_refclk_enabled(dev_priv);
+	ibx_assert_pch_refclk_enabled(dev_priv);
 
 	I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
 
@@ -10135,8 +10132,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 		dev_priv->num_shared_dpll = 0;
 
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
-	DRM_DEBUG_KMS("%i shared PLLs initialized\n",
-		      dev_priv->num_shared_dpll);
 }
 
 static void intel_crtc_init(struct drm_device *dev, int pipe)
-- 
1.8.4.2

Re: [PATCH 2/2] drm/i915: fix wrong PLL debug messages.

[Damien Lespiau]

On Wed, Jan 08, 2014 at 11:12:28AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> LPT does have PCH refclk, but it's different form the IBX/CPT/PPT one
> and doesn't use the same structs. It is wrong to have a message saying
> that "LPT does not has PCH refclk" (sic). While at it, signal that we
> only want this function on IBX/CPT/PPT by renaming it and adding a
> WARN.
> 
> On HSW we also print "0 shared PLLs initialized", but we *do* have
> shared PLLs on HSW (LCPLL, WRPLL, SPLL) and we *do* initialize them.
> We just don't use "struct intel_shared_dpll". So remove the debug
> message.
> 
> In the future we may want to rename all that "intel shared pll" code
> to "ibx shared pll", but I'll leave this to another patch.

I remember Daniel saying that his plan was to make the DDI clocks use
the shared PLL struct. I guess that'd take care of the debug message
correctness.

So I defer this one to higher authorities, you have my r-b tag as the
patch makes sense, not sure if Daniel will take it though :)

> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 034952c..df3cf69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1211,15 +1211,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static void assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
> +static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
>  {
>  	u32 val;
>  	bool enabled;
>  
> -	if (HAS_PCH_LPT(dev_priv->dev)) {
> -		DRM_DEBUG_DRIVER("LPT does not has PCH refclk, skipping check\n");
> -		return;
> -	}
> +	WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev)));
>  
>  	val = I915_READ(PCH_DREF_CONTROL);
>  	enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK |
> @@ -10067,7 +10064,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
>  	/* PCH refclock must be enabled first */
> -	assert_pch_refclk_enabled(dev_priv);
> +	ibx_assert_pch_refclk_enabled(dev_priv);
>  
>  	I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
>  
> @@ -10135,8 +10132,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  		dev_priv->num_shared_dpll = 0;
>  
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
> -	DRM_DEBUG_KMS("%i shared PLLs initialized\n",
> -		      dev_priv->num_shared_dpll);
>  }
>  
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: fix wrong PLL debug messages.

[Paulo Zanoni]

2014/1/8 Damien Lespiau <damien.lespiau@intel.com>:
> On Wed, Jan 08, 2014 at 11:12:28AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> LPT does have PCH refclk, but it's different form the IBX/CPT/PPT one
>> and doesn't use the same structs. It is wrong to have a message saying
>> that "LPT does not has PCH refclk" (sic). While at it, signal that we
>> only want this function on IBX/CPT/PPT by renaming it and adding a
>> WARN.
>>
>> On HSW we also print "0 shared PLLs initialized", but we *do* have
>> shared PLLs on HSW (LCPLL, WRPLL, SPLL) and we *do* initialize them.
>> We just don't use "struct intel_shared_dpll". So remove the debug
>> message.
>>
>> In the future we may want to rename all that "intel shared pll" code
>> to "ibx shared pll", but I'll leave this to another patch.
>
> I remember Daniel saying that his plan was to make the DDI clocks use
> the shared PLL struct. I guess that'd take care of the debug message
> correctness.

Yes, but there's currently no plan actually do that conversion, so
we'll stay with wrong messages for an undefined amount of time. The
first message I removed is not even ever print, and the second message
always prints "2" for IBX/CPT/PPT and 0 for LPT+: you can easily check
that by looking at the code, so the messages are not even useful.

Thanks for the reviews!
Paulo

>
> So I defer this one to higher authorities, you have my r-b tag as the
> patch makes sense, not sure if Daniel will take it though :)
>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 034952c..df3cf69 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1211,15 +1211,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
>>       }
>>  }
>>
>> -static void assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
>> +static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
>>  {
>>       u32 val;
>>       bool enabled;
>>
>> -     if (HAS_PCH_LPT(dev_priv->dev)) {
>> -             DRM_DEBUG_DRIVER("LPT does not has PCH refclk, skipping check\n");
>> -             return;
>> -     }
>> +     WARN_ON(!(HAS_PCH_IBX(dev_priv->dev) || HAS_PCH_CPT(dev_priv->dev)));
>>
>>       val = I915_READ(PCH_DREF_CONTROL);
>>       enabled = !!(val & (DREF_SSC_SOURCE_MASK | DREF_NONSPREAD_SOURCE_MASK |
>> @@ -10067,7 +10064,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>>                               struct intel_shared_dpll *pll)
>>  {
>>       /* PCH refclock must be enabled first */
>> -     assert_pch_refclk_enabled(dev_priv);
>> +     ibx_assert_pch_refclk_enabled(dev_priv);
>>
>>       I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
>>
>> @@ -10135,8 +10132,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>>               dev_priv->num_shared_dpll = 0;
>>
>>       BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>> -     DRM_DEBUG_KMS("%i shared PLLs initialized\n",
>> -                   dev_priv->num_shared_dpll);
>>  }
>>
>>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>> --
>> 1.8.4.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [Intel-gfx] [PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Damien Lespiau]

On Wed, Jan 08, 2014 at 11:12:27AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
> state doesn't affect the result of reading the current HW state.
> 
> This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
> HSW and then suspend/resume.
> 
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
> Tested-by: Qingshuai Tian <qingshuai.tian@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 4ec1665..0def5ef 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>  	enum pipe pipe;
>  	struct intel_crtc *intel_crtc;
>  
> +	dev_priv->ddi_plls.spll_refcount = 0;
> +	dev_priv->ddi_plls.wrpll1_refcount = 0;
> +	dev_priv->ddi_plls.wrpll2_refcount = 0;
> +
>  	for_each_pipe(pipe) {
>  		intel_crtc =
>  			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->active) {
> +			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_NONE;
>  			continue;
> +		}
>  
>  		intel_crtc->ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
>  								 pipe);
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Daniel Vetter]

On Wed, Jan 08, 2014 at 11:12:27AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
> state doesn't affect the result of reading the current HW state.
> 
> This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
> HSW and then suspend/resume.
> 
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
> Tested-by: Qingshuai Tian <qingshuai.tian@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 4ec1665..0def5ef 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>  	enum pipe pipe;
>  	struct intel_crtc *intel_crtc;
>  
> +	dev_priv->ddi_plls.spll_refcount = 0;
> +	dev_priv->ddi_plls.wrpll1_refcount = 0;
> +	dev_priv->ddi_plls.wrpll2_refcount = 0;

One idea I have for the longer-term is to unify the ddi pll
refcounting/readout stuff with the logic I've created for shared pch plls.
The pch pll sharing checks and refcount logic is now really solid and
completely paranoid with self-checks, and it took about 10 iterations to
get there in a mostly bug-free manner. It looks a bit like ddi pll sharing
is on track to duplicate that, so merging them would be benificial. It
might also help the state pre-computation stuff we still need to do for
plls.

Anyway, patch merged to -fixes (but I'll probably only get in after 3.14
is out).
-Daniel

> +
>  	for_each_pipe(pipe) {
>  		intel_crtc =
>  			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->active) {
> +			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_NONE;
>  			continue;
> +		}
>  
>  		intel_crtc->ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
>  								 pipe);
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Ville Syrjälä]

On Wed, Jan 08, 2014 at 03:53:28PM +0100, Daniel Vetter wrote:
> On Wed, Jan 08, 2014 at 11:12:27AM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
> > state doesn't affect the result of reading the current HW state.
> > 
> > This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
> > HSW and then suspend/resume.
> > 
> > Cc: stable@vger.kernel.org
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
> > Tested-by: Qingshuai Tian <qingshuai.tian@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 4ec1665..0def5ef 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
> >  	enum pipe pipe;
> >  	struct intel_crtc *intel_crtc;
> >  
> > +	dev_priv->ddi_plls.spll_refcount = 0;
> > +	dev_priv->ddi_plls.wrpll1_refcount = 0;
> > +	dev_priv->ddi_plls.wrpll2_refcount = 0;
> 
> One idea I have for the longer-term is to unify the ddi pll
> refcounting/readout stuff with the logic I've created for shared pch plls.
> The pch pll sharing checks and refcount logic is now really solid and
> completely paranoid with self-checks, and it took about 10 iterations to
> get there in a mostly bug-free manner. It looks a bit like ddi pll sharing
> is on track to duplicate that, so merging them would be benificial. It
> might also help the state pre-computation stuff we still need to do for
> plls.

We might also want to look into PLL sharing on VLV as well.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Paulo Zanoni]

2014/1/8 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jan 08, 2014 at 11:12:27AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
>> state doesn't affect the result of reading the current HW state.
>>
>> This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
>> HSW and then suspend/resume.
>>
>> Cc: stable@vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
>> Tested-by: Qingshuai Tian <qingshuai.tian@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 4ec1665..0def5ef 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>>       enum pipe pipe;
>>       struct intel_crtc *intel_crtc;
>>
>> +     dev_priv->ddi_plls.spll_refcount = 0;
>> +     dev_priv->ddi_plls.wrpll1_refcount = 0;
>> +     dev_priv->ddi_plls.wrpll2_refcount = 0;
>
> One idea I have for the longer-term is to unify the ddi pll
> refcounting/readout stuff with the logic I've created for shared pch plls.
> The pch pll sharing checks and refcount logic is now really solid and
> completely paranoid with self-checks, and it took about 10 iterations to
> get there in a mostly bug-free manner. It looks a bit like ddi pll sharing
> is on track to duplicate that, so merging them would be benificial. It
> might also help the state pre-computation stuff we still need to do for
> plls.

I'm aware that's your plan, but I'm not really sure if the advantages
beat the disadvantages. Also, I remember you said you were going to
implement that, so I was waiting.

I had just written a huge list of reasons explaining why I think we
shouldn't do what you suggested, but I erased it because I know I'm
going to get flamed: merging code like this is always better in
theory.

I also think we should consider doing the other way around: making
IBX/CPT/PPT code follow the HSW implementation model, because the HSW
implementation IMHO looks much simpler and easier to understand.

>
> Anyway, patch merged to -fixes (but I'll probably only get in after 3.14
> is out).
> -Daniel
>
>> +
>>       for_each_pipe(pipe) {
>>               intel_crtc =
>>                       to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>>
>> -             if (!intel_crtc->active)
>> +             if (!intel_crtc->active) {
>> +                     intel_crtc->ddi_pll_sel = PORT_CLK_SEL_NONE;
>>                       continue;
>> +             }
>>
>>               intel_crtc->ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
>>                                                                pipe);
>> --
>> 1.8.4.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/2] drm/i915: fix DDI PLLs HW state readout code

[Daniel Vetter]

On Wed, Jan 08, 2014 at 01:55:19PM -0200, Paulo Zanoni wrote:
> 2014/1/8 Daniel Vetter <daniel@ffwll.ch>:
> > On Wed, Jan 08, 2014 at 11:12:27AM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Properly zero the refcounts and crtc->ddi_pll_set so the previous HW
> >> state doesn't affect the result of reading the current HW state.
> >>
> >> This fixes WARNs about WRPLL refcount if we have an HDMI monitor on
> >> HSW and then suspend/resume.
> >>
> >> Cc: stable@vger.kernel.org
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=64379
> >> Tested-by: Qingshuai Tian <qingshuai.tian@intel.com>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 4ec1665..0def5ef 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1136,12 +1136,18 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
> >>       enum pipe pipe;
> >>       struct intel_crtc *intel_crtc;
> >>
> >> +     dev_priv->ddi_plls.spll_refcount = 0;
> >> +     dev_priv->ddi_plls.wrpll1_refcount = 0;
> >> +     dev_priv->ddi_plls.wrpll2_refcount = 0;
> >
> > One idea I have for the longer-term is to unify the ddi pll
> > refcounting/readout stuff with the logic I've created for shared pch plls.
> > The pch pll sharing checks and refcount logic is now really solid and
> > completely paranoid with self-checks, and it took about 10 iterations to
> > get there in a mostly bug-free manner. It looks a bit like ddi pll sharing
> > is on track to duplicate that, so merging them would be benificial. It
> > might also help the state pre-computation stuff we still need to do for
> > plls.
> 
> I'm aware that's your plan, but I'm not really sure if the advantages
> beat the disadvantages. Also, I remember you said you were going to
> implement that, so I was waiting.

Well there's lots of stuff somewhere on my todo continually falling off
the end. So if you have some spare bandwidth just ping to make sure I
don't start at the same time. Otherwise I don't mind at all if people
steal my todo items ;-)

> I had just written a huge list of reasons explaining why I think we
> shouldn't do what you suggested, but I erased it because I know I'm
> going to get flamed: merging code like this is always better in
> theory.
> 
> I also think we should consider doing the other way around: making
> IBX/CPT/PPT code follow the HSW implementation model, because the HSW
> implementation IMHO looks much simpler and easier to understand.

Yeah, it's fairly complex and abstracted since the older code was imo too
fragile. Imo the separation between the hw enable/disable code and the
refcounting/checking is fairly nice.

But it sounds like you disagree, so I really want to hear your reasons and
objections. Can you please dig them out again?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch