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

[Chris Wilson <chris@chris-wilson.co.uk>]

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