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

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 10/23/2014 09:50 PM, Jesse Barnes wrote:
> This allows us to calculate the full pipe config before we do any mode
> setting work.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 93 +++++++++++++++++++++++++-----------
>   1 file changed, 65 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e5bc3c..d5f95e4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10910,45 +10910,62 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>   		crtc->scanline_offset = 1;
>   }
>
> +static int intel_modeset_compute_config(struct drm_crtc *crtc,

s/drm_crtc/intel_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;
> +
> +	/* 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. */

This comment seems out of place here since it refers to checking for set 
bits in modeset_pipes. I guess the important part of it is to explain 
the fact that only one new pipe config is obtained here, so maybe it can 
reworded to make that clearer.

And there's another "if (modeset_pipes)" left in __intel_set_mode() to 
which this comment applies.

> +	pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		pipe_config = NULL;
> +
> +		goto out;

Nothing will use pipe_config after this point, so there's no need to set 
it to NULL.

> +	}
> +	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +			       "[modeset]");
> +	to_intel_crtc(crtc)->new_config = pipe_config;
> +
> +out:
> +	return ret;

There's nothing being done here, so we could avoid this goto dance and 
just use returns instead.

Anyway, mostly nit picks, so either way,

Reviewed-by: Ander Conselvan de Oliveira 
<ander.conselvan.de.oliveira@intel.com>

> +}
> +
>   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
> @@ -11042,19 +11059,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 +11082,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