Re: [PATCH] drm/i915: Program LUT before intel_color_commit() if LUT was not previously set v2

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 28, 2019 at 07:58:51PM +0100, Hans de Goede wrote:
> Since commit 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after
> vblank waits"), I am seeing an ugly colored flash of the first few display
> lines on 2 Cherry Trail devices when the gamma table gets set for the first
> time. A blue flash on a GPD win and a yellow flash on an Asus T100HA.
> 
> The problem is that since this change, the LUT is programmed after the
> write *and latching* of the double-buffered register which causes the LUT
> to be used starting at the next frame. This means that the old LUT is still
> used for the first couple of lines of the display. If no LUT was in use
> before then the LUT registers may contain bogus values. This leads to
> messed up colors until the new LUT values are written. At least on CHT DSI
> panels this causes messed up colors on the first few lines.
> 
> This commit fixes this by adding a load_lut_before_commit boolean,
> modifying commit_pipe_config() to load the luts earlier if this is set.
> and setting this from intel_color_check when enabling gamma (rather then
> updating an existing gamma table).
> 
> Changes in v2:
> -Simply check for setting load_lut_before_commit to:
>  if (!old_crtc_state->gamma_enable && new_crtc_state->gamma_enable)
> 
> Fixes: 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c         | 14 ++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c       |  6 +++++-
>  drivers/gpu/drm/i915/display/intel_display_types.h |  3 +++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index fa44eb73d088..954a232c15d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1063,6 +1063,8 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state)
>  		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct intel_plane *plane;
>  
> +	new_crtc_state->load_lut_before_commit = false;
> +
>  	if (!new_crtc_state->base.active ||
>  	    drm_atomic_crtc_needs_modeset(&new_crtc_state->base))
>  		return 0;
> @@ -1071,6 +1073,18 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state)
>  	    new_crtc_state->csc_enable == old_crtc_state->csc_enable)
>  		return 0;
>  
> +	/*
> +	 * Normally we load the LUTs after vblank / after the double-buffer
> +	 * registers written by commit have been latched, this avoids a
> +	 * gamma change mid-way the screen. This does mean that the first
> +	 * few lines of the display will (sometimes) still use the old
> +	 * table. This is fine when changing an existing LUT, but if this
> +	 * is the first time the LUT gets loaded, then the hw may contain
> +	 * random values, causing the first lines to have funky colors.
> +	 */
> +	if (!old_crtc_state->gamma_enable && new_crtc_state->gamma_enable)
> +		new_crtc_state->load_lut_before_commit = true;

Unfortunately gamma_enable is not abstract enough to cover all
platforms.

> +
>  	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>  		struct intel_plane_state *plane_state;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index cbf9cf30050c..6b1dc5a5aeb1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14168,8 +14168,11 @@ static void commit_pipe_config(struct intel_atomic_state *state,
>  	 */
>  	if (!modeset) {
>  		if (new_crtc_state->base.color_mgmt_changed ||
> -		    new_crtc_state->update_pipe)
> +		    new_crtc_state->update_pipe) {
> +			if (new_crtc_state->load_lut_before_commit)
> +				intel_color_load_luts(new_crtc_state);

We don't want to do this from within the vblank evade critical
section, so needs to be moved earlier.

Lemme try to cook up something...

>  			intel_color_commit(new_crtc_state);
> +		}
>  
>  		if (INTEL_GEN(dev_priv) >= 9)
>  			skl_detach_scalers(new_crtc_state);
> @@ -14717,6 +14720,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		if (new_crtc_state->base.active &&
>  		    !needs_modeset(new_crtc_state) &&
> +		    !new_crtc_state->load_lut_before_commit &&
>  		    (new_crtc_state->base.color_mgmt_changed ||
>  		     new_crtc_state->update_pipe))
>  			intel_color_load_luts(new_crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 40184e823c84..6bcc997b7ecb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -990,6 +990,9 @@ struct intel_crtc_state {
>  	/* enable pipe csc? */
>  	bool csc_enable;
>  
> +	/* load luts before color settings commit */
> +	bool load_lut_before_commit;
> +
>  	/* Display Stream compression state */
>  	struct {
>  		bool compression_enable;
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx