| Message ID | 1362489888-32651-1-git-send-email-patrik.r.jakobsson@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> 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
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.
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
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
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.
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
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 >
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); }
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(-)