Re: [PATCH 04/13] drm/i915: add pipe_config->pixel_multiplier

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 27 Mar 2013 00:44:53 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Used by SDVO (and hopefully, eventually HDMI, if we ever get around
> to fixing up the low dotclock CEA modes ...).
> 
> This required adding a new encoder->mode_set callback to be able to
> pass around the intel_crtc_config.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 80 +++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h     | 19 ++-------
>  drivers/gpu/drm/i915/intel_sdvo.c    | 39 +++++++++---------
>  3 files changed, 66 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e22305..3672b8d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4320,14 +4320,15 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>  }
>  
>  static void vlv_update_pll(struct drm_crtc *crtc,
> -			   struct drm_display_mode *mode,
> -			   struct drm_display_mode *adjusted_mode,
>  			   intel_clock_t *clock, intel_clock_t *reduced_clock,
>  			   int num_connectors)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_display_mode *adjusted_mode =
> +		&intel_crtc->config.adjusted_mode;
> +	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;

These arg compaction changes could probably be squashed into the
initial crtc_config patch to make this one smaller.

> @@ -5907,8 +5909,12 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  			encoder->base.base.id,
>  			drm_get_encoder_name(&encoder->base),
>  			mode->base.id, mode->name);
> -		encoder_funcs = encoder->base.helper_private;
> -		encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
> +		if (encoder->mode_set) {
> +			encoder->mode_set(encoder);
> +		} else {
> +			encoder_funcs = encoder->base.helper_private;
> +			encoder_funcs->mode_set(&encoder->base, mode, adjusted_mode);
> +		}
>  	}

This made me do a double take; maybe it's time to
s/encoder/intel_encoder in this function...

Looks good otherwise.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
-- 
Jesse Barnes, Intel Open Source Technology Center