Re: [PATCH 4/4] drm/i915: don't read LVDS regs at compute_config time

[Damien Lespiau <damien.lespiau@intel.com>]

On Fri, Jul 04, 2014 at 01:38:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We may reach this point while the machine is still runtime suspended,
> so we'll hit a WARN. The other encoders also don't touch registers at
> this point, so instead of waking the machine up, write some code to
> keep the register always at the same state, including after we runtime
> suspend/resume.
> 
> Testcase: igt/pm_rpm
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I would store lvds_bpp directly in the encoder structure, have the logic
around A3 power in the _init() function and don't bother re-writing the
A3,B3 control. But anyway, this looks correct to me.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 8aa7973..c511287 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -51,6 +51,7 @@ struct intel_lvds_encoder {
>  
>  	bool is_dual_link;
>  	u32 reg;
> +	u32 a3_power;
>  
>  	struct intel_lvds_connector *attached_connector;
>  };
> @@ -170,8 +171,11 @@ static void intel_pre_enable_lvds(struct intel_encoder *encoder)
>  
>  	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
>  	 * appropriately here, but we need to look more thoroughly into how
> -	 * panels behave in the two modes.
> +	 * panels behave in the two modes. For now, let's just maintain the
> +	 * value we got from the BIOS.
>  	 */
> +	 temp &= ~LVDS_A3_POWER_MASK;
> +	 temp |= lvds_encoder->a3_power;


I guess we actually don't need this as we'll be just preserving what we
read earlier.

>  
>  	/* Set the dithering flag on LVDS as needed, note that there is no
>  	 * special lvds dither control bit on pch-split platforms, dithering is
> @@ -269,7 +273,6 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  				      struct intel_crtc_config *pipe_config)
>  {
>  	struct drm_device *dev = intel_encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_lvds_encoder *lvds_encoder =
>  		to_lvds_encoder(&intel_encoder->base);
>  	struct intel_connector *intel_connector =
> @@ -284,8 +287,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		return false;
>  	}
>  
> -	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
> -	    LVDS_A3_POWER_UP)
> +	if (lvds_encoder->a3_power == LVDS_A3_POWER_UP)
>  		lvds_bpp = 8*3;
>  	else
>  		lvds_bpp = 6*3;
> @@ -1086,6 +1088,9 @@ out:
>  	DRM_DEBUG_KMS("detected %s-link lvds configuration\n",
>  		      lvds_encoder->is_dual_link ? "dual" : "single");
>  
> +	lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
> +				 LVDS_A3_POWER_MASK;
> +
>  	/*
>  	 * Unlock registers and just
>  	 * leave them unlocked
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx