Re: [PATCH 2/4] drm/i915: IBX+ doesn't have separate vsync/hsync controls on the VGA DAC

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 11, 2012 at 09:23:34AM -0700, Jesse Barnes wrote:
> When the PCH split occurred, we dropped support for separate hsync and
> vsync disable in the VGA DAC.  So add a PCH specific DPMS function that
> just uses the port enable bit for controlling DPMS states.
> 
> Before this fix, when anything other than a full DPMS off occurred,
> the VGA port would be left enabled and scanning out while all the other
> heads would turn off as expected.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_crt.c |   42 ++++++++++++++++++++++++++++---------
>  1 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 70b0f1a..bdaefff 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -55,18 +55,36 @@ static struct intel_crt *intel_attached_crt(struct drm_connector *connector)
>  			    struct intel_crt, base);
>  }
>  
> -static void intel_crt_dpms(struct drm_encoder *encoder, int mode)
> +static void pch_crt_dpms(struct drm_encoder *encoder, int mode)
>  {
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 temp, reg;
> +	u32 temp;
>  
> -	if (HAS_PCH_SPLIT(dev))
> -		reg = PCH_ADPA;
> -	else
> -		reg = ADPA;
> +	temp = I915_READ(PCH_ADPA);
> +	temp &= ~ADPA_DAC_ENABLE;
> +
> +	switch (mode) {
> +	case DRM_MODE_DPMS_ON:
> +		temp |= ADPA_DAC_ENABLE;
> +		break;
> +	case DRM_MODE_DPMS_STANDBY:
> +	case DRM_MODE_DPMS_SUSPEND:
> +	case DRM_MODE_DPMS_OFF:
> +		/* Just leave port enable cleared */
> +		break;
> +	}
> +
> +	I915_WRITE(PCH_ADPA, temp);
> +}
> +
> +static void intel_crt_dpms(struct drm_encoder *encoder, int mode)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 temp;
>  
> -	temp = I915_READ(reg);
> +	temp = I915_READ(ADPA);
>  	temp &= ~(ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE);
>  	temp &= ~ADPA_DAC_ENABLE;
>  
> @@ -85,7 +103,7 @@ static void intel_crt_dpms(struct drm_encoder *encoder, int mode)
>  		break;
>  	}
>  
> -	I915_WRITE(reg, temp);
> +	I915_WRITE(ADPA, temp);
>  }
>  
>  static int intel_crt_mode_valid(struct drm_connector *connector,
> @@ -516,8 +534,7 @@ static void intel_crt_reset(struct drm_connector *connector)
>   * Routines for controlling stuff on the analog port
>   */
>  
> -static const struct drm_encoder_helper_funcs intel_crt_helper_funcs = {
> -	.dpms = intel_crt_dpms,
> +static struct drm_encoder_helper_funcs intel_crt_helper_funcs = {
>  	.mode_fixup = intel_crt_mode_fixup,
>  	.prepare = intel_encoder_prepare,
>  	.commit = intel_encoder_commit,
> @@ -602,6 +619,11 @@ void intel_crt_init(struct drm_device *dev)
>  		connector->interlace_allowed = 1;
>  	connector->doublescan_allowed = 0;
>  
> +	if (HAS_PCH_SPLIT(dev))
> +		intel_crt_helper_funcs.dpms = pch_crt_dpms;
> +	else
> +		intel_crt_helper_funcs.dpms = intel_crt_dpms;

I like the clean split of this, but the static struct frobbing really
itches me. Can you either split up intel_crt_helper_funcs into a gmch and
pch_split version or add a dpms function pointer to struct intel_crt?

Patch 1&3 of this series are queued for -next, thanks.
-Daniel

> +
>  	drm_encoder_helper_add(&crt->base.base, &intel_crt_helper_funcs);
>  	drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
>  
> -- 
> 1.7.4.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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