[PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We accidentally lost the initial DPLL register write in
1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M

The "three times for luck" hack probably saved us from a total
disaster. But anyway, bring the initial write back so that the
code actually makes some sense.

Cc: stable@vger.kernel.org
Cc: Nick Bowler <nbowler@draconx.ca>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 147e700..f4fdff9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
 			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
 	}
 
+	I915_WRITE(reg, dpll);
+
 	/* Wait for the clocks to stabilize. */
 	POSTING_READ(reg);
 	udelay(150);
-- 
2.4.9

[PATCH 2/2] drm/i915: Enable DPLL VGA mode before P1/P2 divider write

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently writing the DPLL register P1/P2 divider fields won't trigger
an actual change in the DPLL output unless VGA mode is enabled for
prior to the register write that changes the P1/P2 dividers. The write
with the new P1/P2 divider can itself disable VGA mode again without
problems.

I tested the behaviour on my 946GZ, and when manually frobbing the
register with the display on, the behaviour is very clear. However I
can't explain why this machine actually works. The P1/P2 divider
changes caused by normal modesets do seem to make it through to the
hardware somehow since I get a stable picture on the monitor with
any resolution. Maybe it's the "three times for luck" stuff that
somehow masks the problem, or something.

But apparently there are machines (eg. Nick Bowler's G45) where that
isn't the case and we fail to get the correct clock from the DPLL.

Things used to work because we enabled VGA mode for disabled DPLLs,
so when re-enabling the DPLL VGA mode was enabled just prior to the
first register write, and hence the P1/P2 change went through without
a hitch. That got changed in

b8afb9113c51 drm/i915: Keep GMCH DPLL VGA mode always disabled

in the name of consistency. In order to keep the consistency part,
leave VGA mode disabled for disabled DPLLs, but turn it on just prior
to updating the P1/P2 dividers to make sure the hardware picks up
on the new values.

Cc: Nick Bowler <nbowler@draconx.ca>
Reported-by: Nick Bowler <nbowler@draconx.ca>
Tested-by: Nick Bowler <nbowler@draconx.ca>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f4fdff9..ce51dbc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1743,6 +1743,13 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
 			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
 	}
 
+	/*
+	 * Apparently we need to have VGA mode enabled prior to changing
+	 * the P1/P2 dividers. Otherwise the DPLL will keep using the old
+	 * dividers, even though the register value does change.
+	 */
+	I915_WRITE(reg, 0);
+
 	I915_WRITE(reg, dpll);
 
 	/* Wait for the clocks to stabilize. */
-- 
2.4.9

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

Re: [PATCH 2/2] drm/i915: Enable DPLL VGA mode before P1/P2 divider write

[Daniel Vetter]

On Wed, Oct 07, 2015 at 10:08:25PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apparently writing the DPLL register P1/P2 divider fields won't trigger
> an actual change in the DPLL output unless VGA mode is enabled for
> prior to the register write that changes the P1/P2 dividers. The write
> with the new P1/P2 divider can itself disable VGA mode again without
> problems.
> 
> I tested the behaviour on my 946GZ, and when manually frobbing the
> register with the display on, the behaviour is very clear. However I
> can't explain why this machine actually works. The P1/P2 divider
> changes caused by normal modesets do seem to make it through to the
> hardware somehow since I get a stable picture on the monitor with
> any resolution. Maybe it's the "three times for luck" stuff that
> somehow masks the problem, or something.
> 
> But apparently there are machines (eg. Nick Bowler's G45) where that
> isn't the case and we fail to get the correct clock from the DPLL.
> 
> Things used to work because we enabled VGA mode for disabled DPLLs,
> so when re-enabling the DPLL VGA mode was enabled just prior to the
> first register write, and hence the P1/P2 change went through without
> a hitch. That got changed in
> 
> b8afb9113c51 drm/i915: Keep GMCH DPLL VGA mode always disabled
> 
> in the name of consistency. In order to keep the consistency part,
> leave VGA mode disabled for disabled DPLLs, but turn it on just prior
> to updating the P1/P2 dividers to make sure the hardware picks up
> on the new values.
> 
> Cc: Nick Bowler <nbowler@draconx.ca>
> Reported-by: Nick Bowler <nbowler@draconx.ca>
> Tested-by: Nick Bowler <nbowler@draconx.ca>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f4fdff9..ce51dbc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1743,6 +1743,13 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
>  	}
>  
> +	/*
> +	 * Apparently we need to have VGA mode enabled prior to changing
> +	 * the P1/P2 dividers. Otherwise the DPLL will keep using the old
> +	 * dividers, even though the register value does change.
> +	 */
> +	I915_WRITE(reg, 0);

Again POSTING_READ for ordering? Otherwise Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Not that there's a lot to review since Bspec is silent ...
-Daniel
> +
>  	I915_WRITE(reg, dpll);
>  
>  	/* Wait for the clocks to stabilize. */
> -- 
> 2.4.9
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Enable DPLL VGA mode before P1/P2 divider write

[Chris Wilson]

On Thu, Oct 08, 2015 at 10:19:14AM +0200, Daniel Vetter wrote:
> On Wed, Oct 07, 2015 at 10:08:25PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Apparently writing the DPLL register P1/P2 divider fields won't trigger
> > an actual change in the DPLL output unless VGA mode is enabled for
> > prior to the register write that changes the P1/P2 dividers. The write
> > with the new P1/P2 divider can itself disable VGA mode again without
> > problems.
> > 
> > I tested the behaviour on my 946GZ, and when manually frobbing the
> > register with the display on, the behaviour is very clear. However I
> > can't explain why this machine actually works. The P1/P2 divider
> > changes caused by normal modesets do seem to make it through to the
> > hardware somehow since I get a stable picture on the monitor with
> > any resolution. Maybe it's the "three times for luck" stuff that
> > somehow masks the problem, or something.
> > 
> > But apparently there are machines (eg. Nick Bowler's G45) where that
> > isn't the case and we fail to get the correct clock from the DPLL.
> > 
> > Things used to work because we enabled VGA mode for disabled DPLLs,
> > so when re-enabling the DPLL VGA mode was enabled just prior to the
> > first register write, and hence the P1/P2 change went through without
> > a hitch. That got changed in
> > 
> > b8afb9113c51 drm/i915: Keep GMCH DPLL VGA mode always disabled
> > 
> > in the name of consistency. In order to keep the consistency part,
> > leave VGA mode disabled for disabled DPLLs, but turn it on just prior
> > to updating the P1/P2 dividers to make sure the hardware picks up
> > on the new values.
> > 
> > Cc: Nick Bowler <nbowler@draconx.ca>
> > Reported-by: Nick Bowler <nbowler@draconx.ca>
> > Tested-by: Nick Bowler <nbowler@draconx.ca>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f4fdff9..ce51dbc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1743,6 +1743,13 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
> >  	}
> >  
> > +	/*
> > +	 * Apparently we need to have VGA mode enabled prior to changing
> > +	 * the P1/P2 dividers. Otherwise the DPLL will keep using the old
> > +	 * dividers, even though the register value does change.
> > +	 */
> > +	I915_WRITE(reg, 0);
> 
> Again POSTING_READ for ordering? Otherwise Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Not that there's a lot to review since Bspec is silent ...

One option is to start using mmiowb() for documenting the write ordering
(which is a no-op on x86 and so heaven forbid we get to use WC ever.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
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: Restore lost DPLL register write on gen2-4

[Daniel Vetter]

On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We accidentally lost the initial DPLL register write in
> 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
> 
> The "three times for luck" hack probably saved us from a total
> disaster. But anyway, bring the initial write back so that the
> code actually makes some sense.
> 
> Cc: stable@vger.kernel.org
> Cc: Nick Bowler <nbowler@draconx.ca>
Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 147e700..f4fdff9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);

Don't we also need a POSTING_READ here to make sure the two-step 2x mode
sequence is still followed?

With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>  	}
>  
> +	I915_WRITE(reg, dpll);
> +
>  	/* Wait for the clocks to stabilize. */
>  	POSTING_READ(reg);
>  	udelay(150);
> -- 
> 2.4.9
> 
> _______________________________________________
> 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

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[Daniel Vetter]

On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> We accidentally lost the initial DPLL register write in
> 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
> 
> The "three times for luck" hack probably saved us from a total
> disaster. But anyway, bring the initial write back so that the
> code actually makes some sense.
> 
> Cc: stable@vger.kernel.org
> Cc: Nick Bowler <nbowler@draconx.ca>
Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html

> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 147e700..f4fdff9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);

Don't we also need a POSTING_READ here to make sure the two-step 2x mode
sequence is still followed?

With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>  	}
>  
> +	I915_WRITE(reg, dpll);
> +
>  	/* Wait for the clocks to stabilize. */
>  	POSTING_READ(reg);
>  	udelay(150);
> -- 
> 2.4.9
> 
> _______________________________________________
> 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

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We accidentally lost the initial DPLL register write in
> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
> > 
> > The "three times for luck" hack probably saved us from a total
> > disaster. But anyway, bring the initial write back so that the
> > code actually makes some sense.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Nick Bowler <nbowler@draconx.ca>
> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 147e700..f4fdff9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
> 
> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
> sequence is still followed?

We don't do write combining on registers, and there are no shadow
register type of things to consider in this case either.

> 
> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >  	}
> >  
> > +	I915_WRITE(reg, dpll);
> > +
> >  	/* Wait for the clocks to stabilize. */
> >  	POSTING_READ(reg);
> >  	udelay(150);
> > -- 
> > 2.4.9
> > 
> > _______________________________________________
> > 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ä
Intel OTC

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > We accidentally lost the initial DPLL register write in
> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
> > 
> > The "three times for luck" hack probably saved us from a total
> > disaster. But anyway, bring the initial write back so that the
> > code actually makes some sense.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Nick Bowler <nbowler@draconx.ca>
> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
> 
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 147e700..f4fdff9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
> 
> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
> sequence is still followed?

We don't do write combining on registers, and there are no shadow
register type of things to consider in this case either.

> 
> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >  	}
> >  
> > +	I915_WRITE(reg, dpll);
> > +
> >  	/* Wait for the clocks to stabilize. */
> >  	POSTING_READ(reg);
> >  	udelay(150);
> > -- 
> > 2.4.9
> > 
> > _______________________________________________
> > 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�
Intel OTC

Re: [PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[Jani Nikula]

On Thu, 08 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
>> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > 
>> > We accidentally lost the initial DPLL register write in
>> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
>> > 
>> > The "three times for luck" hack probably saved us from a total
>> > disaster. But anyway, bring the initial write back so that the
>> > code actually makes some sense.
>> > 
>> > Cc: stable@vger.kernel.org
>> > Cc: Nick Bowler <nbowler@draconx.ca>
>> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
>> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
>> 
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 147e700..f4fdff9 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
>> 
>> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
>> sequence is still followed?
>
> We don't do write combining on registers, and there are no shadow
> register type of things to consider in this case either.
>
>> 
>> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


Daniel, are you happy with the responses about posting reads, for both
patches?

BR,
Jani.





>> >  	}
>> >  
>> > +	I915_WRITE(reg, dpll);
>> > +
>> >  	/* Wait for the clocks to stabilize. */
>> >  	POSTING_READ(reg);
>> >  	udelay(150);
>> > -- 
>> > 2.4.9
>> > 
>> > _______________________________________________
>> > 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ä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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: Restore lost DPLL register write on gen2-4

[Jani Nikula]

On Thu, 08 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
>> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > 
>> > We accidentally lost the initial DPLL register write in
>> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
>> > 
>> > The "three times for luck" hack probably saved us from a total
>> > disaster. But anyway, bring the initial write back so that the
>> > code actually makes some sense.
>> > 
>> > Cc: stable@vger.kernel.org
>> > Cc: Nick Bowler <nbowler@draconx.ca>
>> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
>> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
>> 
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 147e700..f4fdff9 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
>> 
>> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
>> sequence is still followed?
>
> We don't do write combining on registers, and there are no shadow
> register type of things to consider in this case either.
>
>> 
>> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


Daniel, are you happy with the responses about posting reads, for both
patches?

BR,
Jani.





>> >  	}
>> >  
>> > +	I915_WRITE(reg, dpll);
>> > +
>> >  	/* Wait for the clocks to stabilize. */
>> >  	POSTING_READ(reg);
>> >  	udelay(150);
>> > -- 
>> > 2.4.9
>> > 
>> > _______________________________________________
>> > 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ä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[Daniel Vetter]

On Tue, Oct 13, 2015 at 04:10:19PM +0300, Jani Nikula wrote:
> On Thu, 08 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
> >> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > 
> >> > We accidentally lost the initial DPLL register write in
> >> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
> >> > 
> >> > The "three times for luck" hack probably saved us from a total
> >> > disaster. But anyway, bring the initial write back so that the
> >> > code actually makes some sense.
> >> > 
> >> > Cc: stable@vger.kernel.org
> >> > Cc: Nick Bowler <nbowler@draconx.ca>
> >> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
> >> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
> >> 
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index 147e700..f4fdff9 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
> >> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
> >> 
> >> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
> >> sequence is still followed?
> >
> > We don't do write combining on registers, and there are no shadow
> > register type of things to consider in this case either.
> >
> >> 
> >> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> Daniel, are you happy with the responses about posting reads, for both
> patches?

Yeah, acked on irc but forgot to follow up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
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: Restore lost DPLL register write on gen2-4

[Daniel Vetter]

On Tue, Oct 13, 2015 at 04:10:19PM +0300, Jani Nikula wrote:
> On Thu, 08 Oct 2015, Ville Syrj�l� <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
> >> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
> >> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >> > 
> >> > We accidentally lost the initial DPLL register write in
> >> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
> >> > 
> >> > The "three times for luck" hack probably saved us from a total
> >> > disaster. But anyway, bring the initial write back so that the
> >> > code actually makes some sense.
> >> > 
> >> > Cc: stable@vger.kernel.org
> >> > Cc: Nick Bowler <nbowler@draconx.ca>
> >> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
> >> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
> >> 
> >> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index 147e700..f4fdff9 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
> >> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
> >> 
> >> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
> >> sequence is still followed?
> >
> > We don't do write combining on registers, and there are no shadow
> > register type of things to consider in this case either.
> >
> >> 
> >> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> Daniel, are you happy with the responses about posting reads, for both
> patches?

Yeah, acked on irc but forgot to follow up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Restore lost DPLL register write on gen2-4

[Jani Nikula]

On Tue, 13 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Oct 13, 2015 at 04:10:19PM +0300, Jani Nikula wrote:
>> On Thu, 08 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
>> >> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
>> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > 
>> >> > We accidentally lost the initial DPLL register write in
>> >> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
>> >> > 
>> >> > The "three times for luck" hack probably saved us from a total
>> >> > disaster. But anyway, bring the initial write back so that the
>> >> > code actually makes some sense.
>> >> > 
>> >> > Cc: stable@vger.kernel.org
>> >> > Cc: Nick Bowler <nbowler@draconx.ca>
>> >> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
>> >> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
>> >> 
>> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
>> >> >  1 file changed, 2 insertions(+)
>> >> > 
>> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> > index 147e700..f4fdff9 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_display.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>> >> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
>> >> 
>> >> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
>> >> sequence is still followed?
>> >
>> > We don't do write combining on registers, and there are no shadow
>> > register type of things to consider in this case either.
>> >
>> >> 
>> >> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> 
>> 
>> Daniel, are you happy with the responses about posting reads, for both
>> patches?
>
> Yeah, acked on irc but forgot to follow up.

Both pushed to drm-intel-fixes, thanks for the patches and review.

BR,
Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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: Restore lost DPLL register write on gen2-4

[Jani Nikula]

On Tue, 13 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Oct 13, 2015 at 04:10:19PM +0300, Jani Nikula wrote:
>> On Thu, 08 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Oct 08, 2015 at 10:17:30AM +0200, Daniel Vetter wrote:
>> >> On Wed, Oct 07, 2015 at 10:08:24PM +0300, ville.syrjala@linux.intel.com wrote:
>> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > 
>> >> > We accidentally lost the initial DPLL register write in
>> >> > 1c4e02746147 drm/i915: Fix DVO 2x clock enable on 830M
>> >> > 
>> >> > The "three times for luck" hack probably saved us from a total
>> >> > disaster. But anyway, bring the initial write back so that the
>> >> > code actually makes some sense.
>> >> > 
>> >> > Cc: stable@vger.kernel.org
>> >> > Cc: Nick Bowler <nbowler@draconx.ca>
>> >> Reported-and-tested-by: Nick Bowler <nbowler@draconx.ca>
>> >> References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077463.html
>> >> 
>> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
>> >> >  1 file changed, 2 insertions(+)
>> >> > 
>> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> > index 147e700..f4fdff9 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_display.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> > @@ -1743,6 +1743,8 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
>> >> >  			   I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
>> >> 
>> >> Don't we also need a POSTING_READ here to make sure the two-step 2x mode
>> >> sequence is still followed?
>> >
>> > We don't do write combining on registers, and there are no shadow
>> > register type of things to consider in this case either.
>> >
>> >> 
>> >> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> 
>> 
>> Daniel, are you happy with the responses about posting reads, for both
>> patches?
>
> Yeah, acked on irc but forgot to follow up.

Both pushed to drm-intel-fixes, thanks for the patches and review.

BR,
Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center