[PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Patrik Jakobsson]

According to PRM we need to disable hsync and vsync even though ADPA is
disabled. The previous code did infact do the opposite so we fix it.

Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
---
 drivers/gpu/drm/i915/intel_crt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 969d08c..32a3693 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -88,7 +88,7 @@ static void intel_disable_crt(struct intel_encoder *encoder)
 	u32 temp;
 
 	temp = I915_READ(crt->adpa_reg);
-	temp &= ~(ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE);
+	temp |= ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE;
 	temp &= ~ADPA_DAC_ENABLE;
 	I915_WRITE(crt->adpa_reg, temp);
 }
-- 
1.7.10.4

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Ville Syrjälä]

On Tue, Mar 05, 2013 at 02:24:48PM +0100, Patrik Jakobsson wrote:
> According to PRM we need to disable hsync and vsync even though ADPA is
> disabled. The previous code did infact do the opposite so we fix it.
> 
> Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>
> ---
>  drivers/gpu/drm/i915/intel_crt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 969d08c..32a3693 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -88,7 +88,7 @@ static void intel_disable_crt(struct intel_encoder *encoder)
>  	u32 temp;
>  
>  	temp = I915_READ(crt->adpa_reg);
> -	temp &= ~(ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE);
> +	temp |= ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE;

Accroding to the docs these bits don't exist on PCH platforms.
intel_crt_dpms() already has a check for this, so I suppose
intel_disable_crt() should have one too.

Also I noticed that we seem to have the hsync and vsync disable
bits reversed. At least that's what the docs are telling me.

>
>  	temp &= ~ADPA_DAC_ENABLE;
>  	I915_WRITE(crt->adpa_reg, temp);
>  }
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Patrik Jakobsson]

> Accroding to the docs these bits don't exist on PCH platforms.
> intel_crt_dpms() already has a check for this, so I suppose
> intel_disable_crt() should have one too.
>
> Also I noticed that we seem to have the hsync and vsync disable
> bits reversed. At least that's what the docs are telling me.

The PCH check just forces suspend and standby to off and we're only doing dpms
off in intel_disable_crt() so no need to check it there.

I'm looking at the 965/G35 PRM and the "sync disable" are defined correctly but
used incorrectly in intel_disable_crt(). That's what my patch fixes. I haven't
checked the other PRMs. Is it different on newer hardware?

I'm thinking that this is related to the bug 56359. The thing that triggers it
seems to be that ADPA sometimes get connected to the same pipe as LVDS and
somehow VSYNC and HSYNC trickles through. This might be fixed by setting the
ADPA pipe again in intel_enable_crt() and/or intel_crt_dpms().

Thanks
Patrik

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Ville Syrjälä]

On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote:
> > Accroding to the docs these bits don't exist on PCH platforms.
> > intel_crt_dpms() already has a check for this, so I suppose
> > intel_disable_crt() should have one too.
> >
> > Also I noticed that we seem to have the hsync and vsync disable
> > bits reversed. At least that's what the docs are telling me.
> 
> The PCH check just forces suspend and standby to off and we're only doing dpms
> off in intel_disable_crt() so no need to check it there.

You're right. I assumed that the check would somehow avoid setting
these bits too, but it doesn't. So I guess we don't really care
that they don't exist.

> I'm looking at the 965/G35 PRM and the "sync disable" are defined correctly but
> used incorrectly in intel_disable_crt(). That's what my patch fixes. I haven't
> checked the other PRMs. Is it different on newer hardware?

This is what the docs say:

11:10 Monitor DPMS: (for CRT port) ...
...
00 = ... (will not affect sync pulses)
01 = ... (HSYNC pulses, VSYNC does not)
10 = ... (VSYNC pulses, HSYNC does not)
11 = ... (Neither HSYNC nor VSYNC pulses)

These are our definintions:

#define   ADPA_VSYNC_CNTL_DISABLE (1<<11)
#define   ADPA_HSYNC_CNTL_DISABLE (1<<10)

As you can see they don't match.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Patrik Jakobsson]

On Tue, Mar 5, 2013 at 3:59 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote:
>> > Accroding to the docs these bits don't exist on PCH platforms.
>> > intel_crt_dpms() already has a check for this, so I suppose
>> > intel_disable_crt() should have one too.
>> >
>> > Also I noticed that we seem to have the hsync and vsync disable
>> > bits reversed. At least that's what the docs are telling me.
>>
>> The PCH check just forces suspend and standby to off and we're only doing dpms
>> off in intel_disable_crt() so no need to check it there.
>
> You're right. I assumed that the check would somehow avoid setting
> these bits too, but it doesn't. So I guess we don't really care
> that they don't exist.
>
>> I'm looking at the 965/G35 PRM and the "sync disable" are defined correctly but
>> used incorrectly in intel_disable_crt(). That's what my patch fixes. I haven't
>> checked the other PRMs. Is it different on newer hardware?
>
> This is what the docs say:
>
> 11:10 Monitor DPMS: (for CRT port) ...
> ...
> 00 = ... (will not affect sync pulses)
> 01 = ... (HSYNC pulses, VSYNC does not)
> 10 = ... (VSYNC pulses, HSYNC does not)
> 11 = ... (Neither HSYNC nor VSYNC pulses)
>
> These are our definintions:
>
> #define   ADPA_VSYNC_CNTL_DISABLE (1<<11)
> #define   ADPA_HSYNC_CNTL_DISABLE (1<<10)
>
> As you can see they don't match.

Aha, you're right. I thought you meant disable = 0 and enable = 1.
So when we're doing suspend we are in fact doing standby and vice versa.

-Patrik

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Daniel Vetter]

On Tue, Mar 05, 2013 at 04:59:12PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote:
> > > Accroding to the docs these bits don't exist on PCH platforms.
> > > intel_crt_dpms() already has a check for this, so I suppose
> > > intel_disable_crt() should have one too.
> > >
> > > Also I noticed that we seem to have the hsync and vsync disable
> > > bits reversed. At least that's what the docs are telling me.
> > 
> > The PCH check just forces suspend and standby to off and we're only doing dpms
> > off in intel_disable_crt() so no need to check it there.
> 
> You're right. I assumed that the check would somehow avoid setting
> these bits too, but it doesn't. So I guess we don't really care
> that they don't exist.

The dpms state gets clamped to the values support by the hw in
intel_crt_dpms. So I think we should care also in intel_crt_disable.

> > I'm looking at the 965/G35 PRM and the "sync disable" are defined correctly but
> > used incorrectly in intel_disable_crt(). That's what my patch fixes. I haven't
> > checked the other PRMs. Is it different on newer hardware?
> 
> This is what the docs say:
> 
> 11:10 Monitor DPMS: (for CRT port) ...
> ...
> 00 = ... (will not affect sync pulses)
> 01 = ... (HSYNC pulses, VSYNC does not)
> 10 = ... (VSYNC pulses, HSYNC does not)
> 11 = ... (Neither HSYNC nor VSYNC pulses)
> 
> These are our definintions:
> 
> #define   ADPA_VSYNC_CNTL_DISABLE (1<<11)
> #define   ADPA_HSYNC_CNTL_DISABLE (1<<10)
> 
> As you can see they don't match.

I've checked gen2/3 docs and they agree with this, so we need to update
the #define.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Ville Syrjälä]

On Tue, Mar 05, 2013 at 04:23:59PM +0100, Daniel Vetter wrote:
> On Tue, Mar 05, 2013 at 04:59:12PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote:
> > > > Accroding to the docs these bits don't exist on PCH platforms.
> > > > intel_crt_dpms() already has a check for this, so I suppose
> > > > intel_disable_crt() should have one too.
> > > >
> > > > Also I noticed that we seem to have the hsync and vsync disable
> > > > bits reversed. At least that's what the docs are telling me.
> > > 
> > > The PCH check just forces suspend and standby to off and we're only doing dpms
> > > off in intel_disable_crt() so no need to check it there.
> > 
> > You're right. I assumed that the check would somehow avoid setting
> > these bits too, but it doesn't. So I guess we don't really care
> > that they don't exist.
> 
> The dpms state gets clamped to the values support by the hw in
> intel_crt_dpms. So I think we should care also in intel_crt_disable.

The point was that in intel_crt_dpms() we don't care whether the
hsync/vsync disable bits actually exist. We just set them whenever
the dpms mode warrants it. So for "off" we always set both bits, and
"off" is always supported. And intel_crt_disable() is equal to
intel_crt_dpms(DRM_MODE_DPMS_OFF) so the behaviour is consistent
across the board.

Whether or not there could be side effects from setting those bits
on PCH plaforms is another matter. If there are, then the clamping
stuff is not enough and we need to add PCH checks to both functions.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Daniel Vetter]

On Tue, Mar 05, 2013 at 05:41:04PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 05, 2013 at 04:23:59PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 05, 2013 at 04:59:12PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 05, 2013 at 03:33:26PM +0100, Patrik Jakobsson wrote:
> > > > > Accroding to the docs these bits don't exist on PCH platforms.
> > > > > intel_crt_dpms() already has a check for this, so I suppose
> > > > > intel_disable_crt() should have one too.
> > > > >
> > > > > Also I noticed that we seem to have the hsync and vsync disable
> > > > > bits reversed. At least that's what the docs are telling me.
> > > > 
> > > > The PCH check just forces suspend and standby to off and we're only doing dpms
> > > > off in intel_disable_crt() so no need to check it there.
> > > 
> > > You're right. I assumed that the check would somehow avoid setting
> > > these bits too, but it doesn't. So I guess we don't really care
> > > that they don't exist.
> > 
> > The dpms state gets clamped to the values support by the hw in
> > intel_crt_dpms. So I think we should care also in intel_crt_disable.
> 
> The point was that in intel_crt_dpms() we don't care whether the
> hsync/vsync disable bits actually exist. We just set them whenever
> the dpms mode warrants it. So for "off" we always set both bits, and
> "off" is always supported. And intel_crt_disable() is equal to
> intel_crt_dpms(DRM_MODE_DPMS_OFF) so the behaviour is consistent
> across the board.
> 
> Whether or not there could be side effects from setting those bits
> on PCH plaforms is another matter. If there are, then the clamping
> stuff is not enough and we need to add PCH checks to both functions.

Oh right, I've missed that when reworking that code - we set the bits in
the OFF case ... I guess that'd be worthwile to amend, if just to keep out
any future surprises.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Turn off hsync and vsync on ADPA when disabling crt

[Daniel Vetter]

On Tue, Mar 05, 2013 at 02:24:48PM +0100, Patrik Jakobsson wrote:
> According to PRM we need to disable hsync and vsync even though ADPA is
> disabled. The previous code did infact do the opposite so we fix it.
> 
> Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

Bugreporter confirmed that it works, so merged to -fixes with cc: stable.
Thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_crt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 969d08c..32a3693 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -88,7 +88,7 @@ static void intel_disable_crt(struct intel_encoder *encoder)
>  	u32 temp;
>  
>  	temp = I915_READ(crt->adpa_reg);
> -	temp &= ~(ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE);
> +	temp |= ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE;
>  	temp &= ~ADPA_DAC_ENABLE;
>  	I915_WRITE(crt->adpa_reg, temp);
>  }
> -- 
> 1.7.10.4
> 

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