[PATCH] drm/i915: don't clobber the special upscaling lvds timings

[PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Daniel Vetter]

This regression has been introduced in

commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 14:49:20 2012 +0100

    drm/i915: fixup interlaced vertical timings confusion, part 1

Unfortunately that commit failed to take into account that the lvds
code does some special adjustements to the crtc timings for upscaling
an centering.

Fix this by explicitly computing crtc timings in the lvds mode fixup
function and setting a special flag in mode->private_flags if the crtc
timings have been adjusted.

Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++--
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_lvds.c    |    6 ++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bae38ac..8be3091 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3478,8 +3478,11 @@ static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
 			return false;
 	}
 
-	/* All interlaced capable intel hw wants timings in frames. */
-	drm_mode_set_crtcinfo(adjusted_mode, 0);
+	/* All interlaced capable intel hw wants timings in frames. Note though
+	 * that intel_lvds_mode_fixup does some funny tricks with the crtc
+	 * timings, so we need to be careful not to clobber these.*/
+	if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
+		drm_mode_set_crtcinfo(adjusted_mode, 0);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5a14149..dd1c061 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -105,6 +105,7 @@
 #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
 #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
 #define INTEL_MODE_DP_FORCE_6BPC (0x10)
+#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
 
 static inline void
 intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 95db2e9..30e2c82 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -187,6 +187,8 @@ centre_horizontally(struct drm_display_mode *mode,
 
 	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
 	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
+
+	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static void
@@ -208,6 +210,8 @@ centre_vertically(struct drm_display_mode *mode,
 
 	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
 	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
+
+	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static inline u32 panel_fitter_scaling(u32 source, u32 target)
@@ -283,6 +287,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 	for_each_pipe(pipe)
 		I915_WRITE(BCLRPAT(pipe), 0);
 
+	drm_mode_set_crtcinfo(adjusted_mode, 0);
+
 	switch (intel_lvds->fitting_mode) {
 	case DRM_MODE_SCALE_CENTER:
 		/*
-- 
1.7.10

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Chris Wilson]

On Sat, 14 Apr 2012 18:17:57 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This regression has been introduced in
> 
> commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jan 28 14:49:20 2012 +0100
> 
>     drm/i915: fixup interlaced vertical timings confusion, part 1
> 
> Unfortunately that commit failed to take into account that the lvds
> code does some special adjustements to the crtc timings for upscaling
> an centering.
> 
> Fix this by explicitly computing crtc timings in the lvds mode fixup
> function and setting a special flag in mode->private_flags if the crtc
> timings have been adjusted.

Whilst the patch does what it says on the tin, I think this just
underlines the fact that our handling of crtcinfo is just wrong and
consists of fragile hacks. :(
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Daniel Vetter]

On Sat, Apr 14, 2012 at 05:43:49PM +0100, Chris Wilson wrote:
> On Sat, 14 Apr 2012 18:17:57 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > This regression has been introduced in
> > 
> > commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Sat Jan 28 14:49:20 2012 +0100
> > 
> >     drm/i915: fixup interlaced vertical timings confusion, part 1
> > 
> > Unfortunately that commit failed to take into account that the lvds
> > code does some special adjustements to the crtc timings for upscaling
> > an centering.
> > 
> > Fix this by explicitly computing crtc timings in the lvds mode fixup
> > function and setting a special flag in mode->private_flags if the crtc
> > timings have been adjusted.
> 
> Whilst the patch does what it says on the tin, I think this just
> underlines the fact that our handling of crtcinfo is just wrong and
> consists of fragile hacks. :(

Actually I think it's much better now, we compute the crtc info only at
select places now
- encoder->mode_fixup (lvds only atm), this is required to set the new
  CRTC_TIMING_SET flag. In my grepping for the original cleanup patch I've
  failed to notice this, hence the regression.
- crtc->mode_fixup, but only when timings are not yet set (with this
  patch).
- some fixed modes we use for load detect and the fixed TV modes, I think
  these won't go through the above mode_fixup stuff, so I've left them
  out.

To contrast with the state before these two patches:
- No longer splattered all over (some of it was deep down in the encoder
  code, e.g. sdvo).
- No more stupid confusion about interlaced timings.

So imho things are better, but suggestions for further improvements always
highly welcome.

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

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Chris Wilson]

On Sat, 14 Apr 2012 18:54:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> To contrast with the state before these two patches:
> - No longer splattered all over (some of it was deep down in the encoder
>   code, e.g. sdvo).
> - No more stupid confusion about interlaced timings.

Good things for sure. My biggest concern is that we have conflicting
requirements upon the mode as setup in crtcinfo that are not being
detected. The task of mode-valid is to find a compromise mode that works
for all attached connectors, and more importantly make sure we reject
combinations of connectors that would result in no compatible modes
(above and beyond the combination of encoders that are illegal).

And most of that concern is due to the fact that the modesetting code
seems to have evolved ad-hoc with very few sanity checks and no
overarching design.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Eugeni Dodonov]

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

On Sat, Apr 14, 2012 at 14:05, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> And most of that concern is due to the fact that the modesetting code
> seems to have evolved ad-hoc with very few sanity checks and no
> overarching design.
>

Yep, I have to agree on that :(.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 652 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: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Eugeni Dodonov]

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

On Sat, Apr 14, 2012 at 13:17, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> This regression has been introduced in
>
> commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jan 28 14:49:20 2012 +0100
>
>    drm/i915: fixup interlaced vertical timings confusion, part 1
>
> Unfortunately that commit failed to take into account that the lvds
> code does some special adjustements to the crtc timings for upscaling
> an centering.
>
> Fix this by explicitly computing crtc timings in the lvds mode fixup
> function and setting a special flag in mode->private_flags if the crtc
> timings have been adjusted.
>
> Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

As for the patch itself, as far as I can tell, it does what it meant to, so:

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

I have one small suggestion in one hunk below, but that's it.

-       /* All interlaced capable intel hw wants timings in frames. */
> -       drm_mode_set_crtcinfo(adjusted_mode, 0);
> +       /* All interlaced capable intel hw wants timings in frames. Note
> though
> +        * that intel_lvds_mode_fixup does some funny tricks with the crtc
> +        * timings, so we need to be careful not to clobber these.*/
> +       if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
> +               drm_mode_set_crtcinfo(adjusted_mode, 0);
>

To simplify the life of next volunteers to touch those paths, perhaps we
should improve the comment either here, or at the
INTEL_MODE_CRTC_TIMING_SET definition, to mention that this flag should be
set by any encoder that does its timing config on its own and does not
wants his custom settings to get overwritten by the generic
intel_crtc_mode_fixup()
call?

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 2872 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: don't clobber the special upscaling lvds timings

[Daniel Vetter]

This regression has been introduced in

commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 14:49:20 2012 +0100

    drm/i915: fixup interlaced vertical timings confusion, part 1

Unfortunately that commit failed to take into account that the lvds
code does some special adjustements to the crtc timings for upscaling
an centering.

Fix this by explicitly computing crtc timings in the lvds mode fixup
function and setting a special flag in mode->private_flags if the crtc
timings have been adjusted.

v2: Add a comment to explain the new mode driver private flag,
suggested by Eugeni Dodonov.

Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++--
 drivers/gpu/drm/i915/intel_drv.h     |    4 ++++
 drivers/gpu/drm/i915/intel_lvds.c    |    6 ++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bae38ac..8be3091 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3478,8 +3478,11 @@ static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
 			return false;
 	}
 
-	/* All interlaced capable intel hw wants timings in frames. */
-	drm_mode_set_crtcinfo(adjusted_mode, 0);
+	/* All interlaced capable intel hw wants timings in frames. Note though
+	 * that intel_lvds_mode_fixup does some funny tricks with the crtc
+	 * timings, so we need to be careful not to clobber these.*/
+	if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
+		drm_mode_set_crtcinfo(adjusted_mode, 0);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5a14149..715afa1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -105,6 +105,10 @@
 #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
 #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
 #define INTEL_MODE_DP_FORCE_6BPC (0x10)
+/* This flag must be set by the encoder's mode_fixup if it changes the crtc
+ * timings in the mode to prevent the crtc fixup from overwriting them.
+ * Currently only lvds needs that. */
+#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
 
 static inline void
 intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 95db2e9..30e2c82 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -187,6 +187,8 @@ centre_horizontally(struct drm_display_mode *mode,
 
 	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
 	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
+
+	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static void
@@ -208,6 +210,8 @@ centre_vertically(struct drm_display_mode *mode,
 
 	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
 	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
+
+	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static inline u32 panel_fitter_scaling(u32 source, u32 target)
@@ -283,6 +287,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 	for_each_pipe(pipe)
 		I915_WRITE(BCLRPAT(pipe), 0);
 
+	drm_mode_set_crtcinfo(adjusted_mode, 0);
+
 	switch (intel_lvds->fitting_mode) {
 	case DRM_MODE_SCALE_CENTER:
 		/*
-- 
1.7.10

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Chris Wilson]

On Sun, 15 Apr 2012 19:24:34 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This regression has been introduced in
> 
> commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jan 28 14:49:20 2012 +0100
> 
>     drm/i915: fixup interlaced vertical timings confusion, part 1
> 
> Unfortunately that commit failed to take into account that the lvds
> code does some special adjustements to the crtc timings for upscaling
> an centering.
> 
> Fix this by explicitly computing crtc timings in the lvds mode fixup
> function and setting a special flag in mode->private_flags if the crtc
> timings have been adjusted.
> 
> v2: Add a comment to explain the new mode driver private flag,
> suggested by Eugeni Dodonov.
> 
> Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++++--
>  drivers/gpu/drm/i915/intel_drv.h     |    4 ++++
>  drivers/gpu/drm/i915/intel_lvds.c    |    6 ++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bae38ac..8be3091 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3478,8 +3478,11 @@ static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
>  			return false;
>  	}
>  
> -	/* All interlaced capable intel hw wants timings in frames. */
> -	drm_mode_set_crtcinfo(adjusted_mode, 0);
> +	/* All interlaced capable intel hw wants timings in frames. Note though
> +	 * that intel_lvds_mode_fixup does some funny tricks with the crtc
> +	 * timings, so we need to be careful not to clobber these.*/
> +	if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
> +		drm_mode_set_crtcinfo(adjusted_mode, 0);
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5a14149..715afa1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -105,6 +105,10 @@
>  #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
>  #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
>  #define INTEL_MODE_DP_FORCE_6BPC (0x10)
> +/* This flag must be set by the encoder's mode_fixup if it changes the crtc
> + * timings in the mode to prevent the crtc fixup from overwriting them.
> + * Currently only lvds needs that. */
> +#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
>  
>  static inline void
>  intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 95db2e9..30e2c82 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -187,6 +187,8 @@ centre_horizontally(struct drm_display_mode *mode,
>  
>  	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
>  	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
> +
> +	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
>  }
>  
>  static void
> @@ -208,6 +210,8 @@ centre_vertically(struct drm_display_mode *mode,
>  
>  	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
>  	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
> +
> +	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
>  }
>  
>  static inline u32 panel_fitter_scaling(u32 source, u32 target)
> @@ -283,6 +287,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
>  	for_each_pipe(pipe)
>  		I915_WRITE(BCLRPAT(pipe), 0);
>  
> +	drm_mode_set_crtcinfo(adjusted_mode, 0);

This line at least is confusing, since adjusted_mode is already
overriden in intel_fixed_panel_mode().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Daniel Vetter]

This regression has been introduced in

commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jan 28 14:49:20 2012 +0100

    drm/i915: fixup interlaced vertical timings confusion, part 1

Unfortunately that commit failed to take into account that the lvds
code does some special adjustements to the crtc timings for upscaling
an centering.

Fix this by explicitly computing crtc timings in the lvds mode fixup
function and setting a special flag in mode->private_flags if the crtc
timings have been adjusted.

v2: Add a comment to explain the new mode driver private flag,
suggested by Eugeni Dodonov.

v3: Kill the confusing and now redundant set_crtcinfo call in
intel_fixed_panel_mode, noticed by Chris Wilson.

Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++--
 drivers/gpu/drm/i915/intel_drv.h     |    4 ++++
 drivers/gpu/drm/i915/intel_lvds.c    |    6 ++++++
 drivers/gpu/drm/i915/intel_panel.c   |    2 --
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bae38ac..8be3091 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3478,8 +3478,11 @@ static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
 			return false;
 	}
 
-	/* All interlaced capable intel hw wants timings in frames. */
-	drm_mode_set_crtcinfo(adjusted_mode, 0);
+	/* All interlaced capable intel hw wants timings in frames. Note though
+	 * that intel_lvds_mode_fixup does some funny tricks with the crtc
+	 * timings, so we need to be careful not to clobber these.*/
+	if (!(adjusted_mode->private_flags & INTEL_MODE_CRTC_TIMINGS_SET))
+		drm_mode_set_crtcinfo(adjusted_mode, 0);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5a14149..715afa1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -105,6 +105,10 @@
 #define INTEL_MODE_PIXEL_MULTIPLIER_SHIFT (0x0)
 #define INTEL_MODE_PIXEL_MULTIPLIER_MASK (0xf << INTEL_MODE_PIXEL_MULTIPLIER_SHIFT)
 #define INTEL_MODE_DP_FORCE_6BPC (0x10)
+/* This flag must be set by the encoder's mode_fixup if it changes the crtc
+ * timings in the mode to prevent the crtc fixup from overwriting them.
+ * Currently only lvds needs that. */
+#define INTEL_MODE_CRTC_TIMINGS_SET (0x20)
 
 static inline void
 intel_mode_set_pixel_multiplier(struct drm_display_mode *mode,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 95db2e9..30e2c82 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -187,6 +187,8 @@ centre_horizontally(struct drm_display_mode *mode,
 
 	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
 	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
+
+	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static void
@@ -208,6 +210,8 @@ centre_vertically(struct drm_display_mode *mode,
 
 	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
 	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
+
+	mode->private_flags |= INTEL_MODE_CRTC_TIMINGS_SET;
 }
 
 static inline u32 panel_fitter_scaling(u32 source, u32 target)
@@ -283,6 +287,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 	for_each_pipe(pipe)
 		I915_WRITE(BCLRPAT(pipe), 0);
 
+	drm_mode_set_crtcinfo(adjusted_mode, 0);
+
 	switch (intel_lvds->fitting_mode) {
 	case DRM_MODE_SCALE_CENTER:
 		/*
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 230a141..48177ec 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -47,8 +47,6 @@ intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 	adjusted_mode->vtotal = fixed_mode->vtotal;
 
 	adjusted_mode->clock = fixed_mode->clock;
-
-	drm_mode_set_crtcinfo(adjusted_mode, 0);
 }
 
 /* adjusted_mode has been preset to be the panel's fixed mode */
-- 
1.7.10

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Chris Wilson]

On Sun, 15 Apr 2012 19:53:19 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This regression has been introduced in
> 
> commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jan 28 14:49:20 2012 +0100
> 
>     drm/i915: fixup interlaced vertical timings confusion, part 1
> 
> Unfortunately that commit failed to take into account that the lvds
> code does some special adjustements to the crtc timings for upscaling
> an centering.
> 
> Fix this by explicitly computing crtc timings in the lvds mode fixup
> function and setting a special flag in mode->private_flags if the crtc
> timings have been adjusted.
> 
> v2: Add a comment to explain the new mode driver private flag,
> suggested by Eugeni Dodonov.
> 
> v3: Kill the confusing and now redundant set_crtcinfo call in
> intel_fixed_panel_mode, noticed by Chris Wilson.
> 
> Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The patch fixes a regression and shouldn't have any side-effects as far
as I can see, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Just wish there were more ponies.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: don't clobber the special upscaling lvds timings

[Daniel Vetter]

On Sun, Apr 15, 2012 at 07:03:18PM +0100, Chris Wilson wrote:
> On Sun, 15 Apr 2012 19:53:19 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > This regression has been introduced in
> > 
> > commit ca9bfa7eed20ea34e862804e62aae10eb159edbb
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Sat Jan 28 14:49:20 2012 +0100
> > 
> >     drm/i915: fixup interlaced vertical timings confusion, part 1
> > 
> > Unfortunately that commit failed to take into account that the lvds
> > code does some special adjustements to the crtc timings for upscaling
> > an centering.
> > 
> > Fix this by explicitly computing crtc timings in the lvds mode fixup
> > function and setting a special flag in mode->private_flags if the crtc
> > timings have been adjusted.
> > 
> > v2: Add a comment to explain the new mode driver private flag,
> > suggested by Eugeni Dodonov.
> > 
> > v3: Kill the confusing and now redundant set_crtcinfo call in
> > intel_fixed_panel_mode, noticed by Chris Wilson.
> > 
> > Reported-and-Tested-by: Hans de Bruin <jmdebruin@xmsnet.nl>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43071
> > Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> The patch fixes a regression and shouldn't have any side-effects as far
> as I can see, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Just wish there were more ponies.

Picked up for -fixes, thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48