[5/9] drm/i915: Clear out DPLL state from pipe config in DSI get config
diff mbox

Message ID 1435580756-20154-6-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä June 29, 2015, 12:25 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
from the pipe_config in intel_dsi_get_config(). This avoids spurious
state checker warnings. We already did it this way for DPLL_MD, but do
it for DPLL too.

Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
are enabled however. Supposedly they have some meaning to DSI too.
We now keep the ref clocks always enabled while the disp2d well is
enabled.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Daniel Vetter June 29, 2015, 4:42 p.m. UTC | #1
On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> from the pipe_config in intel_dsi_get_config(). This avoids spurious
> state checker warnings. We already did it this way for DPLL_MD, but do
> it for DPLL too.
> 
> Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> are enabled however. Supposedly they have some meaning to DSI too.
> We now keep the ref clocks always enabled while the disp2d well is
> enabled.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 36e2148..92bb252 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>  
>  	/* Disable DPOunit clock gating, can stall pipe
>  	 * and we need DPLL REFA always enabled */
> -	tmp = I915_READ(DPLL(pipe));
> -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> -	I915_WRITE(DPLL(pipe), tmp);
> -
> -	/* update the hw state for DPLL */
> -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> -
>  	tmp = I915_READ(DSPCLK_GATE_D);
>  	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
>  	I915_WRITE(DSPCLK_GATE_D, tmp);
>  
> +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> +
>  	/* put device in ready state */
>  	intel_dsi_device_ready(encoder);
>  
> @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
>  	DRM_DEBUG_KMS("\n");
>  
>  	/*
> -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> -	 * set dpll_md = 0
> +	 * DPLL is not used in case of DSI, reading will getsome default value.
> +	 * Clear the state to keep the state checker happy.
>  	 */
> +	pipe_config->dpll_hw_state.dpll = 0;
>  	pipe_config->dpll_hw_state.dpll_md = 0;

State configs are supposed to be kzallocated. Needing this indicates a
pretty serious bug - I'd vote to instead also ditch the dpll_md line and
fix the offender.
-Daniel

>  
>  	pclk = vlv_get_dsi_pclk(encoder, pipe_config->pipe_bpp);
> -- 
> 2.3.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä June 29, 2015, 4:56 p.m. UTC | #2
On Mon, Jun 29, 2015 at 06:42:11PM +0200, Daniel Vetter wrote:
> On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> > from the pipe_config in intel_dsi_get_config(). This avoids spurious
> > state checker warnings. We already did it this way for DPLL_MD, but do
> > it for DPLL too.
> > 
> > Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> > are enabled however. Supposedly they have some meaning to DSI too.
> > We now keep the ref clocks always enabled while the disp2d well is
> > enabled.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 36e2148..92bb252 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> >  
> >  	/* Disable DPOunit clock gating, can stall pipe
> >  	 * and we need DPLL REFA always enabled */
> > -	tmp = I915_READ(DPLL(pipe));
> > -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> > -	I915_WRITE(DPLL(pipe), tmp);
> > -
> > -	/* update the hw state for DPLL */
> > -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> > -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> > -
> >  	tmp = I915_READ(DSPCLK_GATE_D);
> >  	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
> >  	I915_WRITE(DSPCLK_GATE_D, tmp);
> >  
> > +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> > +
> >  	/* put device in ready state */
> >  	intel_dsi_device_ready(encoder);
> >  
> > @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
> >  	DRM_DEBUG_KMS("\n");
> >  
> >  	/*
> > -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> > -	 * set dpll_md = 0
> > +	 * DPLL is not used in case of DSI, reading will getsome default value.
> > +	 * Clear the state to keep the state checker happy.
> >  	 */
> > +	pipe_config->dpll_hw_state.dpll = 0;
> >  	pipe_config->dpll_hw_state.dpll_md = 0;
> 
> State configs are supposed to be kzallocated. Needing this indicates a
> pretty serious bug - I'd vote to instead also ditch the dpll_md line and
> fix the offender.

There is no offender really. We read out the DPLL state before we know
which ports are active and hence can't tell at that point if the
information is really relevant.

The alternative would be to explicitly program the DPLL to a specific
state for DSI. That is, we could just call {vlv,chv}_disable_pll()
from  crtc_enable when the pipe is driving a DSI port), and have
intel_dsi_compute_config() fill in an identical state.

> -Daniel
> 
> >  
> >  	pclk = vlv_get_dsi_pclk(encoder, pipe_config->pipe_bpp);
> > -- 
> > 2.3.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Ville Syrjälä June 29, 2015, 5:08 p.m. UTC | #3
On Mon, Jun 29, 2015 at 07:56:05PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 29, 2015 at 06:42:11PM +0200, Daniel Vetter wrote:
> > On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> > > from the pipe_config in intel_dsi_get_config(). This avoids spurious
> > > state checker warnings. We already did it this way for DPLL_MD, but do
> > > it for DPLL too.
> > > 
> > > Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> > > are enabled however. Supposedly they have some meaning to DSI too.
> > > We now keep the ref clocks always enabled while the disp2d well is
> > > enabled.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > > index 36e2148..92bb252 100644
> > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> > >  
> > >  	/* Disable DPOunit clock gating, can stall pipe
> > >  	 * and we need DPLL REFA always enabled */
> > > -	tmp = I915_READ(DPLL(pipe));
> > > -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> > > -	I915_WRITE(DPLL(pipe), tmp);
> > > -
> > > -	/* update the hw state for DPLL */
> > > -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> > > -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> > > -
> > >  	tmp = I915_READ(DSPCLK_GATE_D);
> > >  	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
> > >  	I915_WRITE(DSPCLK_GATE_D, tmp);
> > >  
> > > +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> > > +
> > >  	/* put device in ready state */
> > >  	intel_dsi_device_ready(encoder);
> > >  
> > > @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
> > >  	DRM_DEBUG_KMS("\n");
> > >  
> > >  	/*
> > > -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> > > -	 * set dpll_md = 0
> > > +	 * DPLL is not used in case of DSI, reading will getsome default value.
> > > +	 * Clear the state to keep the state checker happy.
> > >  	 */
> > > +	pipe_config->dpll_hw_state.dpll = 0;
> > >  	pipe_config->dpll_hw_state.dpll_md = 0;
> > 
> > State configs are supposed to be kzallocated. Needing this indicates a
> > pretty serious bug - I'd vote to instead also ditch the dpll_md line and
> > fix the offender.
> 
> There is no offender really. We read out the DPLL state before we know
> which ports are active and hence can't tell at that point if the
> information is really relevant.
> 
> The alternative would be to explicitly program the DPLL to a specific
> state for DSI. That is, we could just call {vlv,chv}_disable_pll()
> from  crtc_enable when the pipe is driving a DSI port), and have
> intel_dsi_compute_config() fill in an identical state.

Or I suppose we could move clock readout to the encoder
.get_config() entirely.
Daniel Vetter June 30, 2015, 10:13 a.m. UTC | #4
On Mon, Jun 29, 2015 at 08:08:27PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 29, 2015 at 07:56:05PM +0300, Ville Syrjälä wrote:
> > On Mon, Jun 29, 2015 at 06:42:11PM +0200, Daniel Vetter wrote:
> > > On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> > > > from the pipe_config in intel_dsi_get_config(). This avoids spurious
> > > > state checker warnings. We already did it this way for DPLL_MD, but do
> > > > it for DPLL too.
> > > > 
> > > > Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> > > > are enabled however. Supposedly they have some meaning to DSI too.
> > > > We now keep the ref clocks always enabled while the disp2d well is
> > > > enabled.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
> > > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > > > index 36e2148..92bb252 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> > > >  
> > > >  	/* Disable DPOunit clock gating, can stall pipe
> > > >  	 * and we need DPLL REFA always enabled */
> > > > -	tmp = I915_READ(DPLL(pipe));
> > > > -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> > > > -	I915_WRITE(DPLL(pipe), tmp);
> > > > -
> > > > -	/* update the hw state for DPLL */
> > > > -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> > > > -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> > > > -
> > > >  	tmp = I915_READ(DSPCLK_GATE_D);
> > > >  	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
> > > >  	I915_WRITE(DSPCLK_GATE_D, tmp);
> > > >  
> > > > +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> > > > +
> > > >  	/* put device in ready state */
> > > >  	intel_dsi_device_ready(encoder);
> > > >  
> > > > @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
> > > >  	DRM_DEBUG_KMS("\n");
> > > >  
> > > >  	/*
> > > > -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> > > > -	 * set dpll_md = 0
> > > > +	 * DPLL is not used in case of DSI, reading will getsome default value.
> > > > +	 * Clear the state to keep the state checker happy.
> > > >  	 */
> > > > +	pipe_config->dpll_hw_state.dpll = 0;
> > > >  	pipe_config->dpll_hw_state.dpll_md = 0;
> > > 
> > > State configs are supposed to be kzallocated. Needing this indicates a
> > > pretty serious bug - I'd vote to instead also ditch the dpll_md line and
> > > fix the offender.
> > 
> > There is no offender really. We read out the DPLL state before we know
> > which ports are active and hence can't tell at that point if the
> > information is really relevant.

So the bios leaves the DPLL enabled even when using a DSI port? Or do we
miss to check some routing bits in get_clock?

> > The alternative would be to explicitly program the DPLL to a specific
> > state for DSI. That is, we could just call {vlv,chv}_disable_pll()
> > from  crtc_enable when the pipe is driving a DSI port), and have
> > intel_dsi_compute_config() fill in an identical state.
> 
> Or I suppose we could move clock readout to the encoder
> .get_config() entirely.

Yeah either that or do a crtc sanitize to fixup the DPLL that's somehow
enabled but not needed. Either option is imo clearer than cleaning out
state that we've read.
-Daniel
Ville Syrjälä June 30, 2015, 11:50 a.m. UTC | #5
On Tue, Jun 30, 2015 at 12:13:37PM +0200, Daniel Vetter wrote:
> On Mon, Jun 29, 2015 at 08:08:27PM +0300, Ville Syrjälä wrote:
> > On Mon, Jun 29, 2015 at 07:56:05PM +0300, Ville Syrjälä wrote:
> > > On Mon, Jun 29, 2015 at 06:42:11PM +0200, Daniel Vetter wrote:
> > > > On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> > > > > from the pipe_config in intel_dsi_get_config(). This avoids spurious
> > > > > state checker warnings. We already did it this way for DPLL_MD, but do
> > > > > it for DPLL too.
> > > > > 
> > > > > Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> > > > > are enabled however. Supposedly they have some meaning to DSI too.
> > > > > We now keep the ref clocks always enabled while the disp2d well is
> > > > > enabled.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
> > > > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > > > > index 36e2148..92bb252 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > > @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> > > > >  
> > > > >  	/* Disable DPOunit clock gating, can stall pipe
> > > > >  	 * and we need DPLL REFA always enabled */
> > > > > -	tmp = I915_READ(DPLL(pipe));
> > > > > -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> > > > > -	I915_WRITE(DPLL(pipe), tmp);
> > > > > -
> > > > > -	/* update the hw state for DPLL */
> > > > > -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> > > > > -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> > > > > -
> > > > >  	tmp = I915_READ(DSPCLK_GATE_D);
> > > > >  	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
> > > > >  	I915_WRITE(DSPCLK_GATE_D, tmp);
> > > > >  
> > > > > +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> > > > > +
> > > > >  	/* put device in ready state */
> > > > >  	intel_dsi_device_ready(encoder);
> > > > >  
> > > > > @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
> > > > >  	DRM_DEBUG_KMS("\n");
> > > > >  
> > > > >  	/*
> > > > > -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> > > > > -	 * set dpll_md = 0
> > > > > +	 * DPLL is not used in case of DSI, reading will getsome default value.
> > > > > +	 * Clear the state to keep the state checker happy.
> > > > >  	 */
> > > > > +	pipe_config->dpll_hw_state.dpll = 0;
> > > > >  	pipe_config->dpll_hw_state.dpll_md = 0;
> > > > 
> > > > State configs are supposed to be kzallocated. Needing this indicates a
> > > > pretty serious bug - I'd vote to instead also ditch the dpll_md line and
> > > > fix the offender.
> > > 
> > > There is no offender really. We read out the DPLL state before we know
> > > which ports are active and hence can't tell at that point if the
> > > information is really relevant.
> 
> So the bios leaves the DPLL enabled even when using a DSI port? Or do we
> miss to check some routing bits in get_clock?

Not necessarily enabled, but there are other bits in there that could be
left in any state basically. The DSI port simply doesn't care.

> > > The alternative would be to explicitly program the DPLL to a specific
> > > state for DSI. That is, we could just call {vlv,chv}_disable_pll()
> > > from  crtc_enable when the pipe is driving a DSI port), and have
> > > intel_dsi_compute_config() fill in an identical state.
> > 
> > Or I suppose we could move clock readout to the encoder
> > .get_config() entirely.
> 
> Yeah either that or do a crtc sanitize to fixup the DPLL that's somehow
> enabled but not needed.

To do that properly we should just expose all the PLLs as "shared" PLLs.
I don't really want to add a VLV/CHV specific hack for that.

> Either option is imo clearer than cleaning out
> state that we've read.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter July 1, 2015, 12:42 p.m. UTC | #6
On Tue, Jun 30, 2015 at 02:50:33PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 30, 2015 at 12:13:37PM +0200, Daniel Vetter wrote:
> > On Mon, Jun 29, 2015 at 08:08:27PM +0300, Ville Syrjälä wrote:
> > > On Mon, Jun 29, 2015 at 07:56:05PM +0300, Ville Syrjälä wrote:
> > > > On Mon, Jun 29, 2015 at 06:42:11PM +0200, Daniel Vetter wrote:
> > > > > On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> > > > > > from the pipe_config in intel_dsi_get_config(). This avoids spurious
> > > > > > state checker warnings. We already did it this way for DPLL_MD, but do
> > > > > > it for DPLL too.
> > > > > > 
> > > > > > Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> > > > > > are enabled however. Supposedly they have some meaning to DSI too.
> > > > > > We now keep the ref clocks always enabled while the disp2d well is
> > > > > > enabled.
> > > > > > 
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
> > > > > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > > > > > index 36e2148..92bb252 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > > > @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> > > > > >  
> > > > > >  	/* Disable DPOunit clock gating, can stall pipe
> > > > > >  	 * and we need DPLL REFA always enabled */
> > > > > > -	tmp = I915_READ(DPLL(pipe));
> > > > > > -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> > > > > > -	I915_WRITE(DPLL(pipe), tmp);
> > > > > > -
> > > > > > -	/* update the hw state for DPLL */
> > > > > > -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> > > > > > -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> > > > > > -
> > > > > >  	tmp = I915_READ(DSPCLK_GATE_D);
> > > > > >  	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
> > > > > >  	I915_WRITE(DSPCLK_GATE_D, tmp);
> > > > > >  
> > > > > > +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> > > > > > +
> > > > > >  	/* put device in ready state */
> > > > > >  	intel_dsi_device_ready(encoder);
> > > > > >  
> > > > > > @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
> > > > > >  	DRM_DEBUG_KMS("\n");
> > > > > >  
> > > > > >  	/*
> > > > > > -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> > > > > > -	 * set dpll_md = 0
> > > > > > +	 * DPLL is not used in case of DSI, reading will getsome default value.
> > > > > > +	 * Clear the state to keep the state checker happy.
> > > > > >  	 */
> > > > > > +	pipe_config->dpll_hw_state.dpll = 0;
> > > > > >  	pipe_config->dpll_hw_state.dpll_md = 0;
> > > > > 
> > > > > State configs are supposed to be kzallocated. Needing this indicates a
> > > > > pretty serious bug - I'd vote to instead also ditch the dpll_md line and
> > > > > fix the offender.
> > > > 
> > > > There is no offender really. We read out the DPLL state before we know
> > > > which ports are active and hence can't tell at that point if the
> > > > information is really relevant.
> > 
> > So the bios leaves the DPLL enabled even when using a DSI port? Or do we
> > miss to check some routing bits in get_clock?
> 
> Not necessarily enabled, but there are other bits in there that could be
> left in any state basically. The DSI port simply doesn't care.

If the enable bit is what's gating things here then we should just forgo
reading out any dpll register state if that's not set. Looking at the
vlv/chv state readout code that seems to be the trouble - there's nothing
guarding the register reads into the pipe_config at all. Didn't matter
pre-vlv without dsi since enable pipe should imply enabled dpll, but
obviously won't work correctly with dsi. Can you please spin such a patch
and remove the hacks here from dsi_get_config?
-Daniel
Sivakumar Thulasimani July 10, 2015, 11:45 a.m. UTC | #7
Reviewed-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>


On 6/29/2015 5:55 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
> from the pipe_config in intel_dsi_get_config(). This avoids spurious
> state checker warnings. We already did it this way for DPLL_MD, but do
> it for DPLL too.
>
> Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
> are enabled however. Supposedly they have some meaning to DSI too.
> We now keep the ref clocks always enabled while the disp2d well is
> enabled.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 36e2148..92bb252 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>   
>   	/* Disable DPOunit clock gating, can stall pipe
>   	 * and we need DPLL REFA always enabled */
> -	tmp = I915_READ(DPLL(pipe));
> -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
> -	I915_WRITE(DPLL(pipe), tmp);
> -
> -	/* update the hw state for DPLL */
> -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> -
>   	tmp = I915_READ(DSPCLK_GATE_D);
>   	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
>   	I915_WRITE(DSPCLK_GATE_D, tmp);
>   
> +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
> +
>   	/* put device in ready state */
>   	intel_dsi_device_ready(encoder);
>   
> @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
>   	DRM_DEBUG_KMS("\n");
>   
>   	/*
> -	 * DPLL_MD is not used in case of DSI, reading will get some default value
> -	 * set dpll_md = 0
> +	 * DPLL is not used in case of DSI, reading will getsome default value.
> +	 * Clear the state to keep the state checker happy.
>   	 */
> +	pipe_config->dpll_hw_state.dpll = 0;
>   	pipe_config->dpll_hw_state.dpll_md = 0;
>   
>   	pclk = vlv_get_dsi_pclk(encoder, pipe_config->pipe_bpp);
Sivakumar Thulasimani July 10, 2015, 12:07 p.m. UTC | #8
On 7/1/2015 6:12 PM, Daniel Vetter wrote:
> On Tue, Jun 30, 2015 at 02:50:33PM +0300, Ville Syrjälä wrote:
>> On Tue, Jun 30, 2015 at 12:13:37PM +0200, Daniel Vetter wrote:
>>> On Mon, Jun 29, 2015 at 08:08:27PM +0300, Ville Syrjälä wrote:
>>>> On Mon, Jun 29, 2015 at 07:56:05PM +0300, Ville Syrjälä wrote:
>>>>> On Mon, Jun 29, 2015 at 06:42:11PM +0200, Daniel Vetter wrote:
>>>>>> On Mon, Jun 29, 2015 at 03:25:52PM +0300, ville.syrjala@linux.intel.com wrote:
>>>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>>
>>>>>>> VLV/CHV don't use the DPLL with DSI, so just clear out the DPLL state
>>>>>>> from the pipe_config in intel_dsi_get_config(). This avoids spurious
>>>>>>> state checker warnings. We already did it this way for DPLL_MD, but do
>>>>>>> it for DPLL too.
>>>>>>>
>>>>>>> Toss in a WARN to intel_dsi_pre_enable() to make sure the ref clocks
>>>>>>> are enabled however. Supposedly they have some meaning to DSI too.
>>>>>>> We now keep the ref clocks always enabled while the disp2d well is
>>>>>>> enabled.
>>>>>>>
>>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/intel_dsi.c | 15 +++++----------
>>>>>>>   1 file changed, 5 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>>>>>> index 36e2148..92bb252 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>>>>>> @@ -421,18 +421,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>>>>>>>   
>>>>>>>   	/* Disable DPOunit clock gating, can stall pipe
>>>>>>>   	 * and we need DPLL REFA always enabled */
>>>>>>> -	tmp = I915_READ(DPLL(pipe));
>>>>>>> -	tmp |= DPLL_REF_CLK_ENABLE_VLV;
>>>>>>> -	I915_WRITE(DPLL(pipe), tmp);
>>>>>>> -
>>>>>>> -	/* update the hw state for DPLL */
>>>>>>> -	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
>>>>>>> -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
>>>>>>> -
>>>>>>>   	tmp = I915_READ(DSPCLK_GATE_D);
>>>>>>>   	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
>>>>>>>   	I915_WRITE(DSPCLK_GATE_D, tmp);
>>>>>>>   
>>>>>>> +	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
>>>>>>> +
>>>>>>>   	/* put device in ready state */
>>>>>>>   	intel_dsi_device_ready(encoder);
>>>>>>>   
>>>>>>> @@ -635,9 +629,10 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
>>>>>>>   	DRM_DEBUG_KMS("\n");
>>>>>>>   
>>>>>>>   	/*
>>>>>>> -	 * DPLL_MD is not used in case of DSI, reading will get some default value
>>>>>>> -	 * set dpll_md = 0
>>>>>>> +	 * DPLL is not used in case of DSI, reading will getsome default value.
>>>>>>> +	 * Clear the state to keep the state checker happy.
>>>>>>>   	 */
>>>>>>> +	pipe_config->dpll_hw_state.dpll = 0;
>>>>>>>   	pipe_config->dpll_hw_state.dpll_md = 0;
>>>>>> State configs are supposed to be kzallocated. Needing this indicates a
>>>>>> pretty serious bug - I'd vote to instead also ditch the dpll_md line and
>>>>>> fix the offender.
>>>>> There is no offender really. We read out the DPLL state before we know
>>>>> which ports are active and hence can't tell at that point if the
>>>>> information is really relevant.
>>> So the bios leaves the DPLL enabled even when using a DSI port? Or do we
>>> miss to check some routing bits in get_clock?
>> Not necessarily enabled, but there are other bits in there that could be
>> left in any state basically. The DSI port simply doesn't care.
> If the enable bit is what's gating things here then we should just forgo
> reading out any dpll register state if that's not set. Looking at the
> vlv/chv state readout code that seems to be the trouble - there's nothing
> guarding the register reads into the pipe_config at all. Didn't matter
> pre-vlv without dsi since enable pipe should imply enabled dpll, but
> obviously won't work correctly with dsi. Can you please spin such a patch
> and remove the hacks here from dsi_get_config?
> -Daniel
Not sure i understand the point of contention here, just noticed this 
after i gave my RB :)
so my justification on why this is proper is that DSI is not supposed to 
touch DPLL register
any place we access dpll_hw_state.dpll is under !is_dsi check so that 
ensures that we dont
program DPLL register for dsi panel. it was wrong to have originally 
modfied DPLL register
inside intel_dsi_pre_enable so removal is fine. setting it to zero in 
intel_dsi_get_config
is of no impact since any place we write back the contents of 
dpll_hw_state is past the
!is_dsi check is never consumed by anyone as long as the CRTC uses DSI.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 36e2148..92bb252 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -421,18 +421,12 @@  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
 
 	/* Disable DPOunit clock gating, can stall pipe
 	 * and we need DPLL REFA always enabled */
-	tmp = I915_READ(DPLL(pipe));
-	tmp |= DPLL_REF_CLK_ENABLE_VLV;
-	I915_WRITE(DPLL(pipe), tmp);
-
-	/* update the hw state for DPLL */
-	intel_crtc->config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
-		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
-
 	tmp = I915_READ(DSPCLK_GATE_D);
 	tmp |= DPOUNIT_CLOCK_GATE_DISABLE;
 	I915_WRITE(DSPCLK_GATE_D, tmp);
 
+	WARN_ON((I915_READ(DPLL(pipe)) & DPLL_REF_CLK_ENABLE_VLV) == 0);
+
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
@@ -635,9 +629,10 @@  static void intel_dsi_get_config(struct intel_encoder *encoder,
 	DRM_DEBUG_KMS("\n");
 
 	/*
-	 * DPLL_MD is not used in case of DSI, reading will get some default value
-	 * set dpll_md = 0
+	 * DPLL is not used in case of DSI, reading will getsome default value.
+	 * Clear the state to keep the state checker happy.
 	 */
+	pipe_config->dpll_hw_state.dpll = 0;
 	pipe_config->dpll_hw_state.dpll_md = 0;
 
 	pclk = vlv_get_dsi_pclk(encoder, pipe_config->pipe_bpp);