Re: [PATCH v2 08/17] drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 13, 2015 at 10:23:38PM +0200, Maarten Lankhorst wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> A follow up patch will make intel_modeset_compute_config() deal with
> multiple crtcs, so move crtc specific stuff into the lower level crtc
> specific function.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 583c9105cf49..ec548cbb06ee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -427,6 +427,12 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
>  }
>  
> +static bool
> +needs_modeset(struct drm_crtc_state *state)
> +{
> +	return state->mode_changed || state->active_changed;
> +}
> +
>  /**
>   * Returns whether any output on the specified pipe is of the specified type
>   */
> @@ -11387,6 +11393,15 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * XXX: Add all connectors to make the crtc state match the encoders.
> +	 */
> +	if (!needs_modeset(&pipe_config->base)) {
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		if (ret)
> +			return ret;
> +	}

Comment aside: Eventually we need to invert this check and use
intel_pipe_config_compare with a special mode to a) not scream into dmesg
b) ignore any changes we can fixup without a full modeset (i.e. pfit). And
then use that to decide whether we need a modeset or not for the crtc.
We still rely upon our internal set_mode functions to at least in some
cases do an unconditional modeset on the passed-in crtc for property
updates. But once converted to atomic we need to be more intelligent,
especially since userspace expects no-op changes to get filtered out. So
unconditionally doing a modeset wont cut it either (with the current code
all the set_prop hooks have such checks hand-rolled).
-Daniel

> +
>  	clear_intel_crtc_state(pipe_config);
>  
>  	pipe_config->cpu_transcoder =
> @@ -11478,6 +11493,18 @@ encoder_retry:
>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> +	/* Check if we need to force a modeset */
> +	if (pipe_config->has_audio !=
> +	    to_intel_crtc_state(crtc->state)->has_audio)
> +		pipe_config->base.mode_changed = true;
> +
> +	/*
> +	 * Note we have an issue here with infoframes: current code
> +	 * only updates them on the full mode set path per hw
> +	 * requirements.  So here we should be checking for any
> +	 * required changes and forcing a mode set.
> +	 */
> +
>  	return 0;
>  fail:
>  	return ret;
> @@ -11495,12 +11522,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  	return false;
>  }
>  
> -static bool
> -needs_modeset(struct drm_crtc_state *state)
> -{
> -	return state->mode_changed || state->active_changed;
> -}
> -
>  static void
>  intel_modeset_update_state(struct drm_atomic_state *state)
>  {
> @@ -12093,10 +12114,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	struct intel_crtc_state *pipe_config;
>  	int ret = 0;
>  
> -	ret = drm_atomic_add_affected_connectors(state, crtc);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>  	ret = drm_atomic_helper_check_modeset(state->dev, state);
>  	if (ret)
>  		return ERR_PTR(ret);
> @@ -12122,19 +12139,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	/* Check things that can only be changed through modeset */
> -	if (pipe_config->has_audio !=
> -	    to_intel_crtc(crtc)->config->has_audio)
> -		pipe_config->base.mode_changed = true;
> -
> -	/*
> -	 * Note we have an issue here with infoframes: current code
> -	 * only updates them on the full mode set path per hw
> -	 * requirements.  So here we should be checking for any
> -	 * required changes and forcing a mode set.
> -	 */
> -
> -	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
> +	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
>  
>  	ret = drm_atomic_helper_check_planes(state->dev, state);
>  	if (ret)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx