[Intel-gfx] [PATCH] drm/i915: Don't initialize pipe config after choosing DPLLs.

[Ville Syrjälä]

On Fri, Nov 07, 2014 at 04:07:50PM -0800, Bob Paauwe wrote:
> The pipe config needs to be initialized before calling crtc_compute_clock
> since this will update the new_config structure DPLL values. Initializing
> the new_config structure after calling crtc_compute_clock can result in
> incorrect timing values.
> 
> This regression was introduced in
> 
> commit 0dbdf89f27b17ae1eceed6782c2917f74cbb5d59
> Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> Date:   Wed Oct 29 11:32:33 2014 +0200
> 
>     drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs
> 
> 	and
> 
> 	commit 00d958817dd3daaa452c221387ddaf23d1e4c06f
> 	Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> 	Date:   Wed Oct 29 11:32:36 2014 +0200
> 
> 	    drm/i915: Covert remaining platforms to choose DPLLS before disabling CRTCs
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe at intel.com>
> CC: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ff071a7..53f3d3a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10774,7 +10774,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		}
>  		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
>  				       "[modeset]");
> -		to_intel_crtc(crtc)->new_config = pipe_config;

new_config _is_ initialized here.

> +
> +		/* mode_set/enable/disable functions rely on a correct pipe
> +		 * config. */
> +		to_intel_crtc(crtc)->config = *pipe_config;
> +		to_intel_crtc(crtc)->new_config = &to_intel_crtc(crtc)->config;

And this will clobber the old config before we even know if the modeset
will succeed. That's not what we want.

You didn't really describe the problem you're seeing, so coming up with
theories is a bit hard. I guess one problem could be that some piece of
code is still looking at crtc->config when it should be looking at
crtc->new_config. In any case, I suggest you tell us a bit more before
anyone spends too much time guessing.

>  	}
>  
>  	/*
> @@ -10820,10 +10824,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 */
>  	if (modeset_pipes) {
>  		crtc->mode = *mode;
> -		/* mode_set/enable/disable functions rely on a correct pipe
> -		 * config. */
> -		to_intel_crtc(crtc)->config = *pipe_config;
> -		to_intel_crtc(crtc)->new_config = &to_intel_crtc(crtc)->config;
>  
>  		/*
>  		 * Calculate and store various constants which
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC