[RFC] drm/i915: always set positive sync in the sdvo register

[RFC] drm/i915: always set positive sync in the sdvo register

[przanoni]

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

We use struct intel_sdvo_dtd for that too.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=15766
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42174
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43333

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


Hi

We have at least 3 reports of wrong sdvo modes that get fixed when we set the
sdvo register to PVSync + PHSync. According to the reporters, modes that contain
NVSync or NHSync don't work without this patch. If you look at the sdvo commnads
(from intel_sdvo_regs.h) you'll see that we also inform the sdvo device about
the positive/negative syncs when we call SDVO_CMD_SET_*_TIMINGS (using the
dtd_flags field of struct intel_sdvo_dtd).

I couldn't find in our documentation any evidence that this patch is actually
right for every sdvo device, but if anyone knows about this or know anyone I
could ask about this, please tell me.

If you have an sdvo device and want to help, please test:
 - use "xrandr --verbose" to check which modes contain -VSync or -HSync or both
 - without this patch, check if modes with -VSync or -HSync or both work (they
   should not be working)
 - apply this patch, and check if all the modes work (they should)
 - if you have newer hardware, be sure to have this patch too before testing
   anything:
   http://lists.freedesktop.org/archives/intel-gfx/2011-October/012726.html

I didn't ask our reporters to test this specific patch, but they're all using
equivalent changes.

Paulo

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 3003fb2..4a6ba9c 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1070,10 +1070,6 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 			sdvox |= intel_sdvo->color_range;
 		if (INTEL_INFO(dev)->gen < 5)
 			sdvox |= SDVO_BORDER_ENABLE;
-		if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
-			sdvox |= SDVO_VSYNC_ACTIVE_HIGH;
-		if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
-			sdvox |= SDVO_HSYNC_ACTIVE_HIGH;
 	} else {
 		sdvox = I915_READ(intel_sdvo->sdvo_reg);
 		switch (intel_sdvo->sdvo_reg) {
@@ -1091,6 +1087,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	if (intel_sdvo->has_hdmi_audio)
 		sdvox |= SDVO_AUDIO_ENABLE;
 
+	/* We already set the {P,N}{V,H}Sync using struct intel_sdvo_dtd */
+	sdvox |= SDVO_VSYNC_ACTIVE_HIGH | SDVO_HSYNC_ACTIVE_HIGH;
+
 	if (INTEL_INFO(dev)->gen >= 4) {
 		/* done in crtc_mode_set as the dpll_md reg must be written early */
 	} else if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) {
-- 
1.7.7.3

Re: [RFC] drm/i915: always set positive sync in the sdvo register

[Chris Wilson]

On Thu,  8 Dec 2011 13:41:53 -0200, przanoni@gmail.com wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We use struct intel_sdvo_dtd for that too.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=15766
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42174
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43333
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

This is an almost-revert of 81a14b46846fea0741902e8d8dfcc6c6c78154c8, so
you should mention that and copy Adam.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC] drm/i915: always set positive sync in the sdvo register

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2065 bytes --]

On Thu,  8 Dec 2011 13:41:53 -0200, przanoni@gmail.com wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We use struct intel_sdvo_dtd for that too.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=15766
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42174
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43333
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I think the patch this is sort-of-reverting was wrong.  Here's what I
found:

It was non-obvious to me what vblank high/low would mean on a digital
link.  It turns out that there are 3 magic characters sent down instead
of pixel data: blank, hsync, and vsync (and syncs can overlap).  The
sync polarity field for SDVOB in the PRM (looking at G35 here) says:

    "Indicates the polarity of Hsync and Vsync. Inverted polarity is
     transmitted as SYNC-BLANK-SYNC and standard polarity is transmitted
     as BLANK-SYNC-BLANK. For example, if Vsync is not inverted and
     Hsync is inverted, an Hsync period transmitted during Vsync would
     be transmitted as BLANK+VS+HS – BLANK+VS – BLANK+VS+HS."

Looking at my SDVO EDS, there's an example of "Invalid HSync Blank
Periods", which includes a sample sequence of "active blank+ hsync blank
hsync blank+ active" being invalid, which matches that description for
hsync low.  The valid hsync examples all match "active blank+ hsync+
blank+ active".  (+ here is the regexp meaning of "at least one of the
character").  I don't see equivalent examples banning vertical low,
but I'd expect so (the valid vertical example is vertical high).

However, I think setting these flags pre-gen4 is also wrong -- bit 4 is
reserved and bit 3 is RO and has a different meaning.  I think the pure
revert is the right answer, instead.

A couple of interesting notes for people debugging SDVO that aren't
reflected in our code:

vblank must be at least 3 lines
vsync must be at least one line.
hblank must be at least hsync characters
hsync must be at least 16 characters.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915/sdvo: Enforce more timing requirements

[Adam Jackson]

Signed-off-by: Adam Jackson <ajax@redhat.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 3003fb2..82de0b0 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1174,6 +1174,18 @@ static int intel_sdvo_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
+	if (mode->vtotal - mode->vdisplay < 3)
+		return MODE_VBLANK_NARROW;
+
+	if (mode->vsync_end - mode->vsync_start < 1)
+		return MODE_VSYNC_NARROW;
+
+	if (mode->htotal - mode->hdisplay < 16)
+		return MODE_HBLANK_NARROW;
+
+	if (mode->hsync_end - mode->hsync_start < 16)
+		return MODE_HSYNC_NARROW;
+
 	return MODE_OK;
 }
 
-- 
1.7.6.4

Re: [PATCH] drm/i915/sdvo: Enforce more timing requirements

[Paulo Zanoni]

Hi

2011/12/13 Adam Jackson <ajax@redhat.com>:
> +       if (mode->vtotal - mode->vdisplay < 3)
> +               return MODE_VBLANK_NARROW;
> +
> +       if (mode->vsync_end - mode->vsync_start < 1)
> +               return MODE_VSYNC_NARROW;
> +
> +       if (mode->htotal - mode->hdisplay < 16)
> +               return MODE_HBLANK_NARROW;
> +
> +       if (mode->hsync_end - mode->hsync_start < 16)

I believe in this line above it should be 2 instead of 16.

> +               return MODE_HSYNC_NARROW;
> +
>        return MODE_OK;
>  }
>
> --
> 1.7.6.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915/sdvo: Enforce more timing requirements

[Daniel Vetter]

On Tue, Dec 13, 2011 at 02:09:28PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2011/12/13 Adam Jackson <ajax@redhat.com>:
> > +       if (mode->vtotal - mode->vdisplay < 3)
> > +               return MODE_VBLANK_NARROW;
> > +
> > +       if (mode->vsync_end - mode->vsync_start < 1)
> > +               return MODE_VSYNC_NARROW;
> > +
> > +       if (mode->htotal - mode->hdisplay < 16)
> > +               return MODE_HBLANK_NARROW;
> > +
> > +       if (mode->hsync_end - mode->hsync_start < 16)
> 
> I believe in this line above it should be 2 instead of 16.
> 
> > +               return MODE_HSYNC_NARROW;
> > +
> >        return MODE_OK;
> >  }

Anything up with this patch or is this one for the bitbucket in the binary
heavens?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48