Re: [PATCH 1/6] drm/i915: factor out compute_config from __intel_set_mode v2

[Jesse Barnes <jbarnes@virtuousgeek.org>]

Thanks for the review, Ander.

Daniel, in case you're tempted to merge this, don't just yet.  Shaung's
test bot found an issue with the refactoring patch I'm still tracking
down.  I'll post a v3 with the fix in reply to this...

Thanks,
Jesse

On Thu, 30 Oct 2014 11:53:59 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> This allows us to calculate the full pipe config before we do any mode
> setting work.
> 
> v2:
>   - clarify comments about global vs. per-crtc mode set (Ander)
>   - clean up unnecessary pipe_config = NULL setting (Ander)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 96
> +++++++++++++++++++++++++----------- 1 file changed, 68
> insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index 6e5bc3c..f9a0963 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10910,45 +10910,61 @@ static void update_scanline_offset(struct
> intel_crtc *crtc) crtc->scanline_offset = 1;
>  }
>  
> +static int intel_modeset_compute_config(struct drm_crtc *crtc,
> +					struct drm_display_mode
> *mode,
> +					struct drm_framebuffer *fb,
> +					unsigned *modeset_pipes,
> +					unsigned *prepare_pipes,
> +					unsigned *disable_pipes)
> +{
> +	struct intel_crtc_config *pipe_config = NULL;
> +	int ret = 0;
> +
> +	intel_modeset_affected_pipes(crtc, modeset_pipes,
> +				     prepare_pipes, disable_pipes);
> +
> +	if (!modeset_pipes)
> +		goto out;
> +
> +	/*
> +	 * Note this needs changes when we start tracking multiple
> modes
> +	 * and crtcs.  At that point we'll need to compute the whole
> config
> +	 * (i.e. one pipe_config for each crtc) rather than just the
> one
> +	 * for this crtc.
> +	 */
> +	pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto out;
> +	}
> +	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +			       "[modeset]");
> +	to_intel_crtc(crtc)->new_config = pipe_config;
> +
> +out:
> +	return ret;
> +}
> +
>  static int __intel_set_mode(struct drm_crtc *crtc,
>  			    struct drm_display_mode *mode,
> -			    int x, int y, struct drm_framebuffer *fb)
> +			    int x, int y, struct drm_framebuffer *fb,
> +			    unsigned disable_pipes,
> +			    unsigned prepare_pipes,
> +			    unsigned modeset_pipes)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *saved_mode;
>  	struct intel_crtc_config *pipe_config = NULL;
>  	struct intel_crtc *intel_crtc;
> -	unsigned disable_pipes, prepare_pipes, modeset_pipes;
>  	int ret = 0;
>  
>  	saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
>  	if (!saved_mode)
>  		return -ENOMEM;
>  
> -	intel_modeset_affected_pipes(crtc, &modeset_pipes,
> -				     &prepare_pipes, &disable_pipes);
> -
>  	*saved_mode = crtc->mode;
>  
> -	/* Hack: Because we don't (yet) support global modeset on
> multiple
> -	 * crtcs, we don't keep track of the new mode for more than
> one crtc.
> -	 * Hence simply check whether any bit is set in
> modeset_pipes in all the
> -	 * pieces of code that are not yet converted to deal with
> mutliple crtcs
> -	 * changing their mode at the same time. */
> -	if (modeset_pipes) {
> -		pipe_config = intel_modeset_pipe_config(crtc, fb,
> mode);
> -		if (IS_ERR(pipe_config)) {
> -			ret = PTR_ERR(pipe_config);
> -			pipe_config = NULL;
> -
> -			goto out;
> -		}
> -		intel_dump_pipe_config(to_intel_crtc(crtc),
> pipe_config,
> -				       "[modeset]");
> -		to_intel_crtc(crtc)->new_config = pipe_config;
> -	}
> -
>  	/*
>  	 * See if the config requires any additional preparation,
> e.g.
>  	 * to adjust global state with pipes off.  We need to do this
> @@ -10973,6 +10989,10 @@ static int __intel_set_mode(struct drm_crtc
> *crtc, 
>  	/* crtc->mode is already used by the ->mode_set callbacks,
> hence we need
>  	 * to set it here already despite that we pass it down the
> callchain.
> +	 *
> +	 * Note we'll need to fix this up when we start tracking
> multiple
> +	 * pipes; here we assume a single modeset_pipe and only
> track the
> +	 * single crtc and mode.
>  	 */
>  	if (modeset_pipes) {
>  		crtc->mode = *mode;
> @@ -11042,19 +11062,22 @@ done:
>  	if (ret && crtc->enabled)
>  		crtc->mode = *saved_mode;
>  
> -out:
>  	kfree(pipe_config);
>  	kfree(saved_mode);
>  	return ret;
>  }
>  
> -static int intel_set_mode(struct drm_crtc *crtc,
> -			  struct drm_display_mode *mode,
> -			  int x, int y, struct drm_framebuffer *fb)
> +static int intel_set_mode_pipes(struct drm_crtc *crtc,
> +				struct drm_display_mode *mode,
> +				int x, int y, struct drm_framebuffer
> *fb,
> +				unsigned modeset_pipes,
> +				unsigned prepare_pipes,
> +				unsigned disable_pipes)
>  {
>  	int ret;
>  
> -	ret = __intel_set_mode(crtc, mode, x, y, fb);
> +	ret = __intel_set_mode(crtc, mode, x, y, fb, modeset_pipes,
> +			       prepare_pipes, disable_pipes);
>  
>  	if (ret == 0)
>  		intel_modeset_check_state(crtc->dev);
> @@ -11062,6 +11085,23 @@ static int intel_set_mode(struct drm_crtc
> *crtc, return ret;
>  }
>  
> +static int intel_set_mode(struct drm_crtc *crtc,
> +			  struct drm_display_mode *mode,
> +			  int x, int y, struct drm_framebuffer *fb)
> +{
> +	unsigned modeset_pipes, prepare_pipes, disable_pipes;
> +	int ret;
> +
> +	ret = intel_modeset_compute_config(crtc, mode, fb,
> +					   &modeset_pipes,
> &prepare_pipes,
> +					   &disable_pipes);
> +	if (ret)
> +		return ret;
> +
> +	return intel_set_mode_pipes(crtc, mode, x, y, fb,
> modeset_pipes,
> +				    prepare_pipes, disable_pipes);
> +}
> +
>  void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  {
>  	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> crtc->primary->fb);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx