[PATCH 0/4] sdvo hdmi regression fix and related cleanups

[PATCH 0/4] sdvo hdmi regression fix and related cleanups

[Daniel Vetter]

Hi all,

The first patch in this series fixes a long-standing hdmi-on-sdvo regression -
we apparently have not set up the pixel doubling (or quadrupling) correctly.
This regression was introduced in 2.6.36. Now if this patch is indeed correct,
hdmi on sdvo (and _only_ hdmi) for any pixelclock below 100 MHz should have been
broken since then. But we seem to have a decent lack of regression reports,
which is why I'm a bit uneasy with this patch.

So review, comments and especially testing (and tested-by gathering) on the
first patch highly welcome. I'd like to include this into -fixes for 3.4, but if
the testing/review on that patch is lacking, I'll postpone it for -next - it's
been broken for way too long anyway :(

The other patches are just clarifications and paranoia-checks for -next.

Yours, Daniel

Daniel Vetter (4):
  drm/i915: handle input/output sdvo timings separately in mode_set
  drm/i915: clarify preferred sdvo input mode code
  drm/i915: don't silently ignore sdvo mode_set failures
  drm/i915: debug messge for lossy sdvo dtd -> drm mode conversion

 drivers/gpu/drm/i915/intel_sdvo.c |   74 +++++++++++++++++++++---------------
 1 files changed, 43 insertions(+), 31 deletions(-)

-- 
1.7.9.1

[PATCH 1/4] drm/i915: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

We seem to have a decent confusion between the output timings and the
input timings of the sdvo encoder. If I understand the code correctly,
we use the original mode unchanged for the output timings, safe for
the lvds case. And we should use the adjusted mode for input timings.

Clarify the situation by adding an explicit output_dtd to the sdvo
mode_set function and streamline the code-flow by moving the input and
output mode setting in the sdvo encode together.

Furthermore testing showed that the sdvo input timing needs the
unadjusted dotclock, the sdvo chip will automatically compute the
required pixel multiplier to get a dotclock above 100 MHz.

Fix this up when converting a drm mode to an sdvo dtd.

This regression was introduced in

commit c74696b9c890074c1e1ee3d7496fc71eb3680ced
Author: Pavel Roskin <proski@gnu.org>
Date:   Thu Sep 2 14:46:34 2010 -0400

    i915: revert some checks added by commit 32aad86f

particularly the following hunk:

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
b/drivers/gpu/drm/i915/intel_sdvo.c
index 093e914..62d22ae 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1122,11 +1123,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,

     /* We have tried to get input timing in mode_fixup, and filled into
        adjusted_mode */
-    if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
-        intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
+    intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
+    if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
         input_dtd.part2.sdvo_flags = intel_sdvo->sdvo_flags;
-    } else
-        intel_sdvo_get_dtd_from_mode(&input_dtd, mode);

     /* If it's a TV, we already set the output timing in mode_fixup.
      * Otherwise, the output timing is equal to the input timing.

Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 6898145..ab47c1e 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	uint16_t width, height;
 	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
 	uint16_t h_sync_offset, v_sync_offset;
+	int mode_clock;
 
 	width = mode->crtc_hdisplay;
 	height = mode->crtc_vdisplay;
@@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
 	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
 
-	dtd->part1.clock = mode->clock / 10;
+	mode_clock = mode->clock;
+	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
+	mode_clock /= 10;
+	dtd->part1.clock = mode_clock;
+
 	dtd->part1.h_active = width & 0xff;
 	dtd->part1.h_blank = h_blank_len & 0xff;
 	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |
@@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
 	u32 sdvox;
 	struct intel_sdvo_in_out_map in_out;
-	struct intel_sdvo_dtd input_dtd;
+	struct intel_sdvo_dtd input_dtd, output_dtd;
 	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
 	int rate;
 
@@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 					  intel_sdvo->attached_output))
 		return;
 
-	/* We have tried to get input timing in mode_fixup, and filled into
-	 * adjusted_mode.
-	 */
-	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
-		input_dtd = intel_sdvo->input_dtd;
-	} else {
-		/* Set the output timing to the screen */
-		if (!intel_sdvo_set_target_output(intel_sdvo,
-						  intel_sdvo->attached_output))
-			return;
-
-		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
-		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
-	}
+	/* lvds has a special fixed output timing. */
+	if (intel_sdvo->is_lvds)
+		intel_sdvo_get_dtd_from_mode(&output_dtd,
+					     intel_sdvo->sdvo_lvds_fixed_mode);
+	else
+		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
+	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
 
 	/* Set the input timing to the screen. Assume always input 0. */
 	if (!intel_sdvo_set_target_input(intel_sdvo))
@@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	    !intel_sdvo_set_tv_format(intel_sdvo))
 		return;
 
+	/* We have tried to get input timing in mode_fixup, and filled into
+	 * adjusted_mode.
+	 */
+	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
 	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
 
 	switch (pixel_multiplier) {
-- 
1.7.9.1

Re: [PATCH 1/4] drm/i915: handle input/output sdvo timings separately in mode_set

[Jesse Barnes]

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

On Tue, 10 Apr 2012 13:55:46 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We seem to have a decent confusion between the output timings and the
> input timings of the sdvo encoder. If I understand the code correctly,
> we use the original mode unchanged for the output timings, safe for
> the lvds case. And we should use the adjusted mode for input timings.
> 
> Clarify the situation by adding an explicit output_dtd to the sdvo
> mode_set function and streamline the code-flow by moving the input and
> output mode setting in the sdvo encode together.
> 
> Furthermore testing showed that the sdvo input timing needs the
> unadjusted dotclock, the sdvo chip will automatically compute the
> required pixel multiplier to get a dotclock above 100 MHz.
> 
> Fix this up when converting a drm mode to an sdvo dtd.
> 
> This regression was introduced in
> 
> commit c74696b9c890074c1e1ee3d7496fc71eb3680ced
> Author: Pavel Roskin <proski@gnu.org>
> Date:   Thu Sep 2 14:46:34 2010 -0400
> 
>     i915: revert some checks added by commit 32aad86f
> 
> particularly the following hunk:
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> b/drivers/gpu/drm/i915/intel_sdvo.c
> index 093e914..62d22ae 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1122,11 +1123,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> 
>      /* We have tried to get input timing in mode_fixup, and filled into
>         adjusted_mode */
> -    if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -        intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> +    intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> +    if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
>          input_dtd.part2.sdvo_flags = intel_sdvo->sdvo_flags;
> -    } else
> -        intel_sdvo_get_dtd_from_mode(&input_dtd, mode);
> 
>      /* If it's a TV, we already set the output timing in mode_fixup.
>       * Otherwise, the output timing is equal to the input timing.
> 
> Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
>  1 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 6898145..ab47c1e 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	uint16_t width, height;
>  	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
>  	uint16_t h_sync_offset, v_sync_offset;
> +	int mode_clock;
>  
>  	width = mode->crtc_hdisplay;
>  	height = mode->crtc_vdisplay;
> @@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
>  	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
>  
> -	dtd->part1.clock = mode->clock / 10;
> +	mode_clock = mode->clock;
> +	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
> +	mode_clock /= 10;
> +	dtd->part1.clock = mode_clock;
> +
>  	dtd->part1.h_active = width & 0xff;
>  	dtd->part1.h_blank = h_blank_len & 0xff;
>  	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |

This hunk looks good.

> @@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
>  	u32 sdvox;
>  	struct intel_sdvo_in_out_map in_out;
> -	struct intel_sdvo_dtd input_dtd;
> +	struct intel_sdvo_dtd input_dtd, output_dtd;
>  	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
>  	int rate;
>  
> @@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  					  intel_sdvo->attached_output))
>  		return;
>  
> -	/* We have tried to get input timing in mode_fixup, and filled into
> -	 * adjusted_mode.
> -	 */
> -	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -		input_dtd = intel_sdvo->input_dtd;
> -	} else {
> -		/* Set the output timing to the screen */
> -		if (!intel_sdvo_set_target_output(intel_sdvo,
> -						  intel_sdvo->attached_output))
> -			return;
> -
> -		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> -		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
> -	}
> +	/* lvds has a special fixed output timing. */
> +	if (intel_sdvo->is_lvds)
> +		intel_sdvo_get_dtd_from_mode(&output_dtd,
> +					     intel_sdvo->sdvo_lvds_fixed_mode);
> +	else
> +		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> +	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
>  
>  	/* Set the input timing to the screen. Assume always input 0. */
>  	if (!intel_sdvo_set_target_input(intel_sdvo))
> @@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	    !intel_sdvo_set_tv_format(intel_sdvo))
>  		return;
>  
> +	/* We have tried to get input timing in mode_fixup, and filled into
> +	 * adjusted_mode.
> +	 */
> +	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
>  	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
>  
>  	switch (pixel_multiplier) {

But seems mostly separate from this hunk, which I don't really
understand, not being an SDVO expert.

What happened to the tv check?  Is input_dtd already set to the right
value here after the change?


-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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

Re: [Intel-gfx] [PATCH 1/4] drm/i915: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

On Tue, Apr 10, 2012 at 09:17:37AM -0700, Jesse Barnes wrote:
> On Tue, 10 Apr 2012 13:55:46 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > We seem to have a decent confusion between the output timings and the
> > input timings of the sdvo encoder. If I understand the code correctly,
> > we use the original mode unchanged for the output timings, safe for
> > the lvds case. And we should use the adjusted mode for input timings.
> > 
> > Clarify the situation by adding an explicit output_dtd to the sdvo
> > mode_set function and streamline the code-flow by moving the input and
> > output mode setting in the sdvo encode together.
> > 
> > Furthermore testing showed that the sdvo input timing needs the
> > unadjusted dotclock, the sdvo chip will automatically compute the
> > required pixel multiplier to get a dotclock above 100 MHz.
> > 
> > Fix this up when converting a drm mode to an sdvo dtd.
> > 
> > This regression was introduced in
> > 
> > commit c74696b9c890074c1e1ee3d7496fc71eb3680ced
> > Author: Pavel Roskin <proski@gnu.org>
> > Date:   Thu Sep 2 14:46:34 2010 -0400
> > 
> >     i915: revert some checks added by commit 32aad86f
> > 
> > particularly the following hunk:
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> > b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 093e914..62d22ae 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1122,11 +1123,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> > 
> >      /* We have tried to get input timing in mode_fixup, and filled into
> >         adjusted_mode */
> > -    if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> > -        intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> > +    intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> > +    if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
> >          input_dtd.part2.sdvo_flags = intel_sdvo->sdvo_flags;
> > -    } else
> > -        intel_sdvo_get_dtd_from_mode(&input_dtd, mode);
> > 
> >      /* If it's a TV, we already set the output timing in mode_fixup.
> >       * Otherwise, the output timing is equal to the input timing.
> > 
> > Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
> >  1 files changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 6898145..ab47c1e 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
> >  	uint16_t width, height;
> >  	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
> >  	uint16_t h_sync_offset, v_sync_offset;
> > +	int mode_clock;
> >  
> >  	width = mode->crtc_hdisplay;
> >  	height = mode->crtc_vdisplay;
> > @@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
> >  	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
> >  	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
> >  
> > -	dtd->part1.clock = mode->clock / 10;
> > +	mode_clock = mode->clock;
> > +	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
> > +	mode_clock /= 10;
> > +	dtd->part1.clock = mode_clock;
> > +
> >  	dtd->part1.h_active = width & 0xff;
> >  	dtd->part1.h_blank = h_blank_len & 0xff;
> >  	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |
> 
> This hunk looks good.

We need both hunks to fix the regression, which is why I've smashed them
into the same patch.

> > @@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> >  	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
> >  	u32 sdvox;
> >  	struct intel_sdvo_in_out_map in_out;
> > -	struct intel_sdvo_dtd input_dtd;
> > +	struct intel_sdvo_dtd input_dtd, output_dtd;
> >  	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> >  	int rate;
> >  
> > @@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> >  					  intel_sdvo->attached_output))
> >  		return;
> >  
> > -	/* We have tried to get input timing in mode_fixup, and filled into
> > -	 * adjusted_mode.
> > -	 */
> > -	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> > -		input_dtd = intel_sdvo->input_dtd;
> > -	} else {
> > -		/* Set the output timing to the screen */
> > -		if (!intel_sdvo_set_target_output(intel_sdvo,
> > -						  intel_sdvo->attached_output))
> > -			return;
> > -
> > -		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> > -		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
> > -	}
> > +	/* lvds has a special fixed output timing. */
> > +	if (intel_sdvo->is_lvds)
> > +		intel_sdvo_get_dtd_from_mode(&output_dtd,
> > +					     intel_sdvo->sdvo_lvds_fixed_mode);
> > +	else
> > +		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> > +	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
> >  
> >  	/* Set the input timing to the screen. Assume always input 0. */
> >  	if (!intel_sdvo_set_target_input(intel_sdvo))
> > @@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> >  	    !intel_sdvo_set_tv_format(intel_sdvo))
> >  		return;
> >  
> > +	/* We have tried to get input timing in mode_fixup, and filled into
> > +	 * adjusted_mode.
> > +	 */
> > +	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> >  	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
> >  
> >  	switch (pixel_multiplier) {
> 
> But seems mostly separate from this hunk, which I don't really
> understand, not being an SDVO expert.
> 
> What happened to the tv check?  Is input_dtd already set to the right
> value here after the change?

Well, neither do I have a clue about sdvo, but I think I somewhat
self-consistent explanation for what's going on.

Sdvo seems to have two timings, one is the output timing which will be
sent over whatever is connected on the other side of the sdvo chip (panel,
hdmi screen, tv), the other is the input timing which will be generated by
the gmch pipe. It looks like sdvo is expected to scale between the two.

To make things slightly more complicated, we have a bunch of special
cases:
- for lvds panel we always use a fixed output timing, namely
  intel_sdvo->sdvo_lvds_fixed_mode, hence that special case.
- sdvo has an interface to generate a preferred input timing for a given
  output timing. This is the confusing thing that I've tried to clear up
  with the follow-on patches.
- a special requirement is that the input pixel clock needs to be between
  100MHz and 200MHz (likely to keep it within the electromechanical design
  range of PCIe). Lower pixel clocks are doubled/quadrupled.

The thing this patch tries to fix is that the pipe needs to be explicit
instructed to double/quadruple the pixels and needs the correspondingly
higher pixel clock, whereas the sdvo adaptor seems to do that itself and
needs the unadjusted pixel clock.

This patch tries to fix this mess by
- keeping the output mode timing in the unadjusted plain mode, safe for
  the lvds case.
- store the input timing in the adjusted_mode with the adjusted pixel
  clock. This way we don't need to frob around with the core crtc mode set
  code.
- fixup the pixelclock when constructing the sdvo dtd timing struct. This
  is why the first part of the patch is an integral part of the series.
- the is_tv special case can be dropped because input_dtd is equivalent to
  adjusted_mode after these changes. Follow-up patches clear this up
  further (by simply ripping out intel_sdvo->input_dtd because it's not
  needed).

Hopefully this clears things up a bit.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/4] drm/i915: handle input/output sdvo timings separately in mode_set

[Jesse Barnes]

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

On Tue, 10 Apr 2012 18:36:49 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:
> Well, neither do I have a clue about sdvo, but I think I somewhat
> self-consistent explanation for what's going on.
> 
> Sdvo seems to have two timings, one is the output timing which will be
> sent over whatever is connected on the other side of the sdvo chip (panel,
> hdmi screen, tv), the other is the input timing which will be generated by
> the gmch pipe. It looks like sdvo is expected to scale between the two.

Correct.  And the SDVO encoder in the display engine will always run
the clock at 100MHz+ and do data stuffing if the pixel rate is lower
than that, but we need to set the right clock multiplier in that case
(which we do).

> To make things slightly more complicated, we have a bunch of special
> cases:
> - for lvds panel we always use a fixed output timing, namely
>   intel_sdvo->sdvo_lvds_fixed_mode, hence that special case.
> - sdvo has an interface to generate a preferred input timing for a given
>   output timing. This is the confusing thing that I've tried to clear up
>   with the follow-on patches.
> - a special requirement is that the input pixel clock needs to be between
>   100MHz and 200MHz (likely to keep it within the electromechanical design
>   range of PCIe). Lower pixel clocks are doubled/quadrupled.
> 
> The thing this patch tries to fix is that the pipe needs to be explicit
> instructed to double/quadruple the pixels and needs the correspondingly
> higher pixel clock, whereas the sdvo adaptor seems to do that itself and
> needs the unadjusted pixel clock.

Yeah that sounds right based on my reading of an old spec I have.

> This patch tries to fix this mess by
> - keeping the output mode timing in the unadjusted plain mode, safe for
>   the lvds case.
> - store the input timing in the adjusted_mode with the adjusted pixel
>   clock. This way we don't need to frob around with the core crtc mode set
>   code.
> - fixup the pixelclock when constructing the sdvo dtd timing struct. This
>   is why the first part of the patch is an integral part of the series.
> - the is_tv special case can be dropped because input_dtd is equivalent to
>   adjusted_mode after these changes. Follow-up patches clear this up
>   further (by simply ripping out intel_sdvo->input_dtd because it's not
>   needed).
> 
> Hopefully this clears things up a bit.

Yep, thanks.  Hopefully you'll get your SDVO spec access soon...

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 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: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

We seem to have a decent confusion between the output timings and the
input timings of the sdvo encoder. If I understand the code correctly,
we use the original mode unchanged for the output timings, safe for
the lvds case. And we should use the adjusted mode for input timings.

Clarify the situation by adding an explicit output_dtd to the sdvo
mode_set function and streamline the code-flow by moving the input and
output mode setting in the sdvo encode together.

Furthermore testing showed that the sdvo input timing needs the
unadjusted dotclock, the sdvo chip will automatically compute the
required pixel multiplier to get a dotclock above 100 MHz.

Fix this up when converting a drm mode to an sdvo dtd.

This regression was introduced in

commit c74696b9c890074c1e1ee3d7496fc71eb3680ced
Author: Pavel Roskin <proski@gnu.org>
Date:   Thu Sep 2 14:46:34 2010 -0400

    i915: revert some checks added by commit 32aad86f

particularly the following hunk:

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
b/drivers/gpu/drm/i915/intel_sdvo.c
index 093e914..62d22ae 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1122,11 +1123,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,

     /* We have tried to get input timing in mode_fixup, and filled into
        adjusted_mode */
-    if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
-        intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
+    intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
+    if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
         input_dtd.part2.sdvo_flags = intel_sdvo->sdvo_flags;
-    } else
-        intel_sdvo_get_dtd_from_mode(&input_dtd, mode);

     /* If it's a TV, we already set the output timing in mode_fixup.
      * Otherwise, the output timing is equal to the input timing.

Due to questions raised in review, below a more elaborate analysis of
the bug at hand:

Sdvo seems to have two timings, one is the output timing which will be
sent over whatever is connected on the other side of the sdvo chip (panel,
hdmi screen, tv), the other is the input timing which will be generated by
the gmch pipe. It looks like sdvo is expected to scale between the two.

To make things slightly more complicated, we have a bunch of special
cases:
- For lvds panel we always use a fixed output timing, namely
  intel_sdvo->sdvo_lvds_fixed_mode, hence that special case.
- Sdvo has an interface to generate a preferred input timing for a given
  output timing. This is the confusing thing that I've tried to clear up
  with the follow-on patches.
- A special requirement is that the input pixel clock needs to be between
  100MHz and 200MHz (likely to keep it within the electromechanical design
  range of PCIe), 270MHz on later gen4+. Lower pixel clocks are
  doubled/quadrupled.

The thing this patch tries to fix is that the pipe needs to be
explicitly instructed to double/quadruple the pixels and needs the
correspondingly higher pixel clock, whereas the sdvo adaptor seems to
do that itself and needs the unadjusted pixel clock. For the sdvo
encode side we already set the pixel mutliplier with a different
command (0x21).

This patch tries to fix this mess by:
- Keeping the output mode timing in the unadjusted plain mode, safe
  for the lvds case.
- Storing the input timing in the adjusted_mode with the adjusted
  pixel clock. This way we don't need to frob around with the core
  crtc mode set code.
- Fixing up the pixelclock when constructing the sdvo dtd timing
  struct. This is why the first hunk of the patch is an integral part
  of the series.
- Dropping the is_tv special case because input_dtd is equivalent to
  adjusted_mode after these changes. Follow-up patches clear this up
  further (by simply ripping out intel_sdvo->input_dtd because it's
  not needed).

v2: Extend commit message with an in-depth bug analysis.

Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 6898145..ab47c1e 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	uint16_t width, height;
 	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
 	uint16_t h_sync_offset, v_sync_offset;
+	int mode_clock;
 
 	width = mode->crtc_hdisplay;
 	height = mode->crtc_vdisplay;
@@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
 	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
 
-	dtd->part1.clock = mode->clock / 10;
+	mode_clock = mode->clock;
+	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
+	mode_clock /= 10;
+	dtd->part1.clock = mode_clock;
+
 	dtd->part1.h_active = width & 0xff;
 	dtd->part1.h_blank = h_blank_len & 0xff;
 	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |
@@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
 	u32 sdvox;
 	struct intel_sdvo_in_out_map in_out;
-	struct intel_sdvo_dtd input_dtd;
+	struct intel_sdvo_dtd input_dtd, output_dtd;
 	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
 	int rate;
 
@@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 					  intel_sdvo->attached_output))
 		return;
 
-	/* We have tried to get input timing in mode_fixup, and filled into
-	 * adjusted_mode.
-	 */
-	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
-		input_dtd = intel_sdvo->input_dtd;
-	} else {
-		/* Set the output timing to the screen */
-		if (!intel_sdvo_set_target_output(intel_sdvo,
-						  intel_sdvo->attached_output))
-			return;
-
-		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
-		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
-	}
+	/* lvds has a special fixed output timing. */
+	if (intel_sdvo->is_lvds)
+		intel_sdvo_get_dtd_from_mode(&output_dtd,
+					     intel_sdvo->sdvo_lvds_fixed_mode);
+	else
+		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
+	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
 
 	/* Set the input timing to the screen. Assume always input 0. */
 	if (!intel_sdvo_set_target_input(intel_sdvo))
@@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	    !intel_sdvo_set_tv_format(intel_sdvo))
 		return;
 
+	/* We have tried to get input timing in mode_fixup, and filled into
+	 * adjusted_mode.
+	 */
+	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
 	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
 
 	switch (pixel_multiplier) {
-- 
1.7.9.1

Re: [PATCH] drm/i915: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

On Tue, Apr 10, 2012 at 07:13:59PM +0200, Daniel Vetter wrote:
> Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>

This tested-by is actually a lie, I've mixed up a few bug reports. Bug
reporter is currently on vacation and will test this stuff in 2 weeks.
-Daniel

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
>  1 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 6898145..ab47c1e 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	uint16_t width, height;
>  	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
>  	uint16_t h_sync_offset, v_sync_offset;
> +	int mode_clock;
>  
>  	width = mode->crtc_hdisplay;
>  	height = mode->crtc_vdisplay;
> @@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
>  	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
>  
> -	dtd->part1.clock = mode->clock / 10;
> +	mode_clock = mode->clock;
> +	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
> +	mode_clock /= 10;
> +	dtd->part1.clock = mode_clock;
> +
>  	dtd->part1.h_active = width & 0xff;
>  	dtd->part1.h_blank = h_blank_len & 0xff;
>  	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |
> @@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
>  	u32 sdvox;
>  	struct intel_sdvo_in_out_map in_out;
> -	struct intel_sdvo_dtd input_dtd;
> +	struct intel_sdvo_dtd input_dtd, output_dtd;
>  	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
>  	int rate;
>  
> @@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  					  intel_sdvo->attached_output))
>  		return;
>  
> -	/* We have tried to get input timing in mode_fixup, and filled into
> -	 * adjusted_mode.
> -	 */
> -	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -		input_dtd = intel_sdvo->input_dtd;
> -	} else {
> -		/* Set the output timing to the screen */
> -		if (!intel_sdvo_set_target_output(intel_sdvo,
> -						  intel_sdvo->attached_output))
> -			return;
> -
> -		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> -		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
> -	}
> +	/* lvds has a special fixed output timing. */
> +	if (intel_sdvo->is_lvds)
> +		intel_sdvo_get_dtd_from_mode(&output_dtd,
> +					     intel_sdvo->sdvo_lvds_fixed_mode);
> +	else
> +		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> +	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
>  
>  	/* Set the input timing to the screen. Assume always input 0. */
>  	if (!intel_sdvo_set_target_input(intel_sdvo))
> @@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	    !intel_sdvo_set_tv_format(intel_sdvo))
>  		return;
>  
> +	/* We have tried to get input timing in mode_fixup, and filled into
> +	 * adjusted_mode.
> +	 */
> +	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
>  	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
>  
>  	switch (pixel_multiplier) {
> -- 
> 1.7.9.1
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

On Wed, Apr 11, 2012 at 12:37:24AM +0200, Daniel Vetter wrote:
> On Tue, Apr 10, 2012 at 07:13:59PM +0200, Daniel Vetter wrote:
> > Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
> 
> This tested-by is actually a lie, I've mixed up a few bug reports. Bug
> reporter is currently on vacation and will test this stuff in 2 weeks.

Ok, I've lucked out and the bug reporter just said that this does indeed
work. Now I just need someone to grill himself by reviewing this stuff ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: handle input/output sdvo timings separately in mode_set

[Daniel Vetter]

I've picked this up for -fixes, with Jesse's irc r-b added.
-Daniel

On Tue, Apr 10, 2012 at 07:13:59PM +0200, Daniel Vetter wrote:
> We seem to have a decent confusion between the output timings and the
> input timings of the sdvo encoder. If I understand the code correctly,
> we use the original mode unchanged for the output timings, safe for
> the lvds case. And we should use the adjusted mode for input timings.
> 
> Clarify the situation by adding an explicit output_dtd to the sdvo
> mode_set function and streamline the code-flow by moving the input and
> output mode setting in the sdvo encode together.
> 
> Furthermore testing showed that the sdvo input timing needs the
> unadjusted dotclock, the sdvo chip will automatically compute the
> required pixel multiplier to get a dotclock above 100 MHz.
> 
> Fix this up when converting a drm mode to an sdvo dtd.
> 
> This regression was introduced in
> 
> commit c74696b9c890074c1e1ee3d7496fc71eb3680ced
> Author: Pavel Roskin <proski@gnu.org>
> Date:   Thu Sep 2 14:46:34 2010 -0400
> 
>     i915: revert some checks added by commit 32aad86f
> 
> particularly the following hunk:
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> b/drivers/gpu/drm/i915/intel_sdvo.c
> index 093e914..62d22ae 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1122,11 +1123,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> 
>      /* We have tried to get input timing in mode_fixup, and filled into
>         adjusted_mode */
> -    if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -        intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> +    intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> +    if (intel_sdvo->is_tv || intel_sdvo->is_lvds)
>          input_dtd.part2.sdvo_flags = intel_sdvo->sdvo_flags;
> -    } else
> -        intel_sdvo_get_dtd_from_mode(&input_dtd, mode);
> 
>      /* If it's a TV, we already set the output timing in mode_fixup.
>       * Otherwise, the output timing is equal to the input timing.
> 
> Due to questions raised in review, below a more elaborate analysis of
> the bug at hand:
> 
> Sdvo seems to have two timings, one is the output timing which will be
> sent over whatever is connected on the other side of the sdvo chip (panel,
> hdmi screen, tv), the other is the input timing which will be generated by
> the gmch pipe. It looks like sdvo is expected to scale between the two.
> 
> To make things slightly more complicated, we have a bunch of special
> cases:
> - For lvds panel we always use a fixed output timing, namely
>   intel_sdvo->sdvo_lvds_fixed_mode, hence that special case.
> - Sdvo has an interface to generate a preferred input timing for a given
>   output timing. This is the confusing thing that I've tried to clear up
>   with the follow-on patches.
> - A special requirement is that the input pixel clock needs to be between
>   100MHz and 200MHz (likely to keep it within the electromechanical design
>   range of PCIe), 270MHz on later gen4+. Lower pixel clocks are
>   doubled/quadrupled.
> 
> The thing this patch tries to fix is that the pipe needs to be
> explicitly instructed to double/quadruple the pixels and needs the
> correspondingly higher pixel clock, whereas the sdvo adaptor seems to
> do that itself and needs the unadjusted pixel clock. For the sdvo
> encode side we already set the pixel mutliplier with a different
> command (0x21).
> 
> This patch tries to fix this mess by:
> - Keeping the output mode timing in the unadjusted plain mode, safe
>   for the lvds case.
> - Storing the input timing in the adjusted_mode with the adjusted
>   pixel clock. This way we don't need to frob around with the core
>   crtc mode set code.
> - Fixing up the pixelclock when constructing the sdvo dtd timing
>   struct. This is why the first hunk of the patch is an integral part
>   of the series.
> - Dropping the is_tv special case because input_dtd is equivalent to
>   adjusted_mode after these changes. Follow-up patches clear this up
>   further (by simply ripping out intel_sdvo->input_dtd because it's
>   not needed).
> 
> v2: Extend commit message with an in-depth bug analysis.
> 
> Reported-and-Tested-by: Bernard Blackham <b-linuxgit@largestprime.net>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48157
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   34 ++++++++++++++++++----------------
>  1 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 6898145..ab47c1e 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -733,6 +733,7 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	uint16_t width, height;
>  	uint16_t h_blank_len, h_sync_len, v_blank_len, v_sync_len;
>  	uint16_t h_sync_offset, v_sync_offset;
> +	int mode_clock;
>  
>  	width = mode->crtc_hdisplay;
>  	height = mode->crtc_vdisplay;
> @@ -747,7 +748,11 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	h_sync_offset = mode->crtc_hsync_start - mode->crtc_hblank_start;
>  	v_sync_offset = mode->crtc_vsync_start - mode->crtc_vblank_start;
>  
> -	dtd->part1.clock = mode->clock / 10;
> +	mode_clock = mode->clock;
> +	mode_clock /= intel_mode_get_pixel_multiplier(mode) ?: 1;
> +	mode_clock /= 10;
> +	dtd->part1.clock = mode_clock;
> +
>  	dtd->part1.h_active = width & 0xff;
>  	dtd->part1.h_blank = h_blank_len & 0xff;
>  	dtd->part1.h_high = (((width >> 8) & 0xf) << 4) |
> @@ -998,7 +1003,7 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	struct intel_sdvo *intel_sdvo = to_intel_sdvo(encoder);
>  	u32 sdvox;
>  	struct intel_sdvo_in_out_map in_out;
> -	struct intel_sdvo_dtd input_dtd;
> +	struct intel_sdvo_dtd input_dtd, output_dtd;
>  	int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
>  	int rate;
>  
> @@ -1023,20 +1028,13 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  					  intel_sdvo->attached_output))
>  		return;
>  
> -	/* We have tried to get input timing in mode_fixup, and filled into
> -	 * adjusted_mode.
> -	 */
> -	if (intel_sdvo->is_tv || intel_sdvo->is_lvds) {
> -		input_dtd = intel_sdvo->input_dtd;
> -	} else {
> -		/* Set the output timing to the screen */
> -		if (!intel_sdvo_set_target_output(intel_sdvo,
> -						  intel_sdvo->attached_output))
> -			return;
> -
> -		intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
> -		(void) intel_sdvo_set_output_timing(intel_sdvo, &input_dtd);
> -	}
> +	/* lvds has a special fixed output timing. */
> +	if (intel_sdvo->is_lvds)
> +		intel_sdvo_get_dtd_from_mode(&output_dtd,
> +					     intel_sdvo->sdvo_lvds_fixed_mode);
> +	else
> +		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
> +	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
>  
>  	/* Set the input timing to the screen. Assume always input 0. */
>  	if (!intel_sdvo_set_target_input(intel_sdvo))
> @@ -1054,6 +1052,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>  	    !intel_sdvo_set_tv_format(intel_sdvo))
>  		return;
>  
> +	/* We have tried to get input timing in mode_fixup, and filled into
> +	 * adjusted_mode.
> +	 */
> +	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
>  	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
>  
>  	switch (pixel_multiplier) {
> -- 
> 1.7.9.1
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 2/4] drm/i915: clarify preferred sdvo input mode code

[Daniel Vetter]

- kill intel_sdvo->input_dtd, it's only used as a temporary variable,
  we store the preferred input mode in the adjusted mode at mode_fixup
  time.
- rename the function to make it clear what we want it to do (get the
  preferred mode) and say in a comment what it unfortunately does as a
  side-effect (set the new output timings).

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index ab47c1e..cb2e358 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -140,9 +140,6 @@ struct intel_sdvo {
 
 	/* DDC bus used by this SDVO encoder */
 	uint8_t ddc_bus;
-
-	/* Input timings for adjusted_mode */
-	struct intel_sdvo_dtd input_dtd;
 };
 
 struct intel_sdvo_connector {
@@ -930,11 +927,15 @@ intel_sdvo_set_output_timings_from_mode(struct intel_sdvo *intel_sdvo,
 	return true;
 }
 
+/* Asks the sdvo controller for the preferred input mode given the output mode.
+ * Unfortunately we have to set up the full output mode to do that. */
 static bool
-intel_sdvo_set_input_timings_for_mode(struct intel_sdvo *intel_sdvo,
-					struct drm_display_mode *mode,
-					struct drm_display_mode *adjusted_mode)
+intel_sdvo_get_preferred_input_mode(struct intel_sdvo *intel_sdvo,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adjusted_mode)
 {
+	struct intel_sdvo_dtd input_dtd;
+
 	/* Reset the input timing to the screen. Assume always input 0. */
 	if (!intel_sdvo_set_target_input(intel_sdvo))
 		return false;
@@ -946,10 +947,10 @@ intel_sdvo_set_input_timings_for_mode(struct intel_sdvo *intel_sdvo,
 		return false;
 
 	if (!intel_sdvo_get_preferred_input_timing(intel_sdvo,
-						   &intel_sdvo->input_dtd))
+						   &input_dtd))
 		return false;
 
-	intel_sdvo_get_mode_from_dtd(adjusted_mode, &intel_sdvo->input_dtd);
+	intel_sdvo_get_mode_from_dtd(adjusted_mode, &input_dtd);
 
 	return true;
 }
@@ -970,17 +971,17 @@ static bool intel_sdvo_mode_fixup(struct drm_encoder *encoder,
 		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, mode))
 			return false;
 
-		(void) intel_sdvo_set_input_timings_for_mode(intel_sdvo,
-							     mode,
-							     adjusted_mode);
+		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
+							   mode,
+							   adjusted_mode);
 	} else if (intel_sdvo->is_lvds) {
 		if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo,
 							     intel_sdvo->sdvo_lvds_fixed_mode))
 			return false;
 
-		(void) intel_sdvo_set_input_timings_for_mode(intel_sdvo,
-							     mode,
-							     adjusted_mode);
+		(void) intel_sdvo_get_preferred_input_mode(intel_sdvo,
+							   mode,
+							   adjusted_mode);
 	}
 
 	/* Make the CRTC code factor in the SDVO pixel multiplier.  The
-- 
1.7.9.1

Re: [PATCH 2/4] drm/i915: clarify preferred sdvo input mode code

[Chris Wilson]

On Tue, 10 Apr 2012 13:55:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> - kill intel_sdvo->input_dtd, it's only used as a temporary variable,
>   we store the preferred input mode in the adjusted mode at mode_fixup
>   time.
> - rename the function to make it clear what we want it to do (get the
>   preferred mode) and say in a comment what it unfortunately does as a
>   side-effect (set the new output timings).
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Patches 2-4: Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: clarify preferred sdvo input mode code

[Daniel Vetter]

On Tue, Apr 10, 2012 at 01:18:13PM +0100, Chris Wilson wrote:
> On Tue, 10 Apr 2012 13:55:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > - kill intel_sdvo->input_dtd, it's only used as a temporary variable,
> >   we store the preferred input mode in the adjusted mode at mode_fixup
> >   time.
> > - rename the function to make it clear what we want it to do (get the
> >   preferred mode) and say in a comment what it unfortunately does as a
> >   side-effect (set the new output timings).
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Patches 2-4: Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
I've queued patches 2&3 for -next, thanks for the review. I'll propably
redo patch 4 with the new constants once the interlaced fixup is merged
back into -next.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 3/4] drm/i915: don't silently ignore sdvo mode_set failures

[Daniel Vetter]

Unfortunately we can't abort a mode_set, but at least tell the user
that something might have gone wrong when setting the sdvo input or
output timing fails.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index cb2e358..a6de3a6 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1035,7 +1035,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 					     intel_sdvo->sdvo_lvds_fixed_mode);
 	else
 		intel_sdvo_get_dtd_from_mode(&output_dtd, mode);
-	(void) intel_sdvo_set_output_timing(intel_sdvo, &output_dtd);
+	if (!intel_sdvo_set_output_timing(intel_sdvo, &output_dtd))
+		DRM_INFO("Setting output timings on %s failed\n",
+			 SDVO_NAME(intel_sdvo));
 
 	/* Set the input timing to the screen. Assume always input 0. */
 	if (!intel_sdvo_set_target_input(intel_sdvo))
@@ -1057,7 +1059,9 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	 * adjusted_mode.
 	 */
 	intel_sdvo_get_dtd_from_mode(&input_dtd, adjusted_mode);
-	(void) intel_sdvo_set_input_timing(intel_sdvo, &input_dtd);
+	if (!intel_sdvo_set_input_timing(intel_sdvo, &input_dtd))
+		DRM_INFO("Setting input timings on %s failed\n",
+			 SDVO_NAME(intel_sdvo));
 
 	switch (pixel_multiplier) {
 	default:
-- 
1.7.9.1

[PATCH 4/4] drm/i915: debug messge for lossy sdvo dtd -> drm mode conversion

[Daniel Vetter]

We round-trip quite often from sdvo dtd timings through drm mode back
to sdvo dtd timings, e.g. due to mode_fixup. Add an informational
message that tells us when we lose things on the way.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index a6de3a6..2c14e37 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -804,6 +804,11 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 
 	mode->clock = dtd->part1.clock * 10;
 
+	if (dtd->part2.dtd_flags & ~(0x2 | 0x4) || dtd->part2.sdvo_flags)
+		DRM_INFO("Potentially losing SDVO DTD parameters, "
+			 "dtd_flags: 0x%02x, sdvo_flags: 0x%02x\n",
+			 dtd->part2.dtd_flags, dtd->part2.sdvo_flags);
+
 	mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	if (dtd->part2.dtd_flags & 0x2)
 		mode->flags |= DRM_MODE_FLAG_PHSYNC;
-- 
1.7.9.1