[Intel-gfx] [PATCH] drm/i915: add missing condition for committing planes on crtc

[Daniel Vetter]

On Wed, May 04, 2016 at 12:13:39PM +0100, Lionel Landwerlin wrote:
> We are currently missing the color management update condition to
> commit planes on crtc.
> 
> v2: add comment about moving the commit of color management registers
>     to an async worker
> 
> Fixes: 20a34e78f0d7 (drm/i915: Update color management during vblank evasion.)
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 45c218d..4936743 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13544,6 +13544,15 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>  	return false;
>  }
>  
> +static bool needs_commit_planes_on_crtc(struct drm_crtc_state *crtc_state)
> +{
> +	/* TODO: drop the color management condition once committing
> +	 * those registers has been moved to an async worker. */
> +	return (crtc_state->planes_changed ||
> +		crtc_state->color_mgmt_changed ||
> +		to_intel_crtc_state(crtc_state)->update_pipe);
> +}

I think it'd be cleaner (as in related logic pulled together) and more in
spirit of the atomic framework if you set planes_changed when you notice
that color mgm state needs an update. That makes it clear that on intel hw
we do such changes with a plain flip (well later on maybe we need a bool
for the vblank worker). Other hw might need a full modeset.

Adding more conditions like this to the commit code makes it imo really
fragile and hard to understand. At least imo computing how to commit state
is best placed next to when we compute what changed.

All the *_changed variables computed by the helpers/core are meant to be
changeable by the driver (assuming it knows what it's doing), and in
general can be upgraded to true without causing any trouble (except maybe
running some code that doesn't necessarily need to be run).
-Daniel

> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13649,7 +13658,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		bool modeset = needs_modeset(crtc->state);
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc->state);
> -		bool update_pipe = !modeset && pipe_config->update_pipe;
>  
>  		if (modeset && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
> @@ -13663,8 +13671,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		    drm_atomic_get_existing_plane_state(state, crtc->primary))
>  			intel_fbc_enable(intel_crtc);
>  
> -		if (crtc->state->active &&
> -		    (crtc->state->planes_changed || update_pipe))
> +		if (crtc->state->active && !modeset &&
> +		    needs_commit_planes_on_crtc(crtc->state))
>  			drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
>  
>  		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
> @@ -13974,6 +13982,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	if (modeset)
>  		return;
>  
> +	/* TODO: Color management registers commit should be moved to
> +	 * an async worker when we have them. */
>  	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
>  		intel_color_load_luts(crtc->state);
> -- 
> 2.8.0.rc3.226.g39d4020
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch