Re: [PATCH v3 14/22] drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, May 20, 2015 at 06:04:26PM +0200, maarten.lankhorst@linux.intel.com wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> It makes more sense there, since these are computation steps that can
> fail.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I've noticed that a few of the patches in this series were originally
written by Ander, but seem to be missing his s-o-b line.  I think you
generally want to just append your line after his in that case.

One other cosmetic note farther down.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 81d5358efdde..e7aa8610cbdc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  		crtc->scanline_offset = 1;
>  }
>  
> -static int
> -intel_modeset_compute_config(struct drm_atomic_state *state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	int ret, i;
> -
> -	ret = drm_atomic_helper_check_modeset(state->dev, state);
> -	if (ret)
> -		return ret;
> -
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!crtc_state->enable &&
> -		    WARN_ON(crtc_state->active))
> -			crtc_state->active = false;
> -
> -		if (!crtc_state->enable)
> -			continue;
> -
> -		ret = intel_modeset_pipe_config(crtc, state);
> -		if (ret)
> -			return ret;
> -
> -		intel_dump_pipe_config(to_intel_crtc(crtc),
> -				       to_intel_crtc_state(crtc_state),
> -				       "[modeset]");
> -	}
> -
> -	return drm_atomic_helper_check_planes(state->dev, state);
> -}
> -
>  static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static int
> +intel_modeset_compute_config(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int ret, i;
> +
> +	ret = drm_atomic_helper_check_modeset(state->dev, state);
> +	if (ret)
> +		return ret;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (!crtc_state->enable &&
> +		    WARN_ON(crtc_state->active))
> +			crtc_state->active = false;
> +
> +		if (!crtc_state->enable)
> +			continue;
> +
> +		ret = intel_modeset_pipe_config(crtc, state);
> +		if (ret)
> +			return ret;
> +
> +		intel_dump_pipe_config(to_intel_crtc(crtc),
> +				       to_intel_crtc_state(crtc_state),
> +				       "[modeset]");
> +	}
> +
> +	ret = drm_atomic_helper_check_planes(state->dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return __intel_set_mode_checks(state);

Just a cosmetic note, but maybe we should rename this function now?
It's not called from __intel_set_mode anymore and it isn't really
'checks' (but rather setup that we intend to be done during the check
phase), so the whole name seems a bit misleading now.


Matt

> +}
> +
>  static int __intel_set_mode(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -12200,10 +12204,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  	int ret = 0;
>  	int i;
>  
> -	ret = __intel_set_mode_checks(state);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
>  	if (ret)
>  		return ret;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx