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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 20-10-2019 20:19, 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 intel_begin_crtc_commit to load the luts earlier if this is set,
> and setting this from intel_color_check when a LUT table was not in use
> before (and thus may contain bogus values), or when the table size
> changes.
> 
> Fixes: 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

So this failed to apply in CI, I based this on 5.4 and I clearly need to rebase
it on top of dinq. Before I do this I would appreciate a review though, as I'm
not entirely sure that this is the right approach to fixing the issue.

Regards,

Hans



> ---
>   drivers/gpu/drm/i915/display/intel_color.c    | 26 +++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_display.c  |  7 +++++
>   .../drm/i915/display/intel_display_types.h    |  3 +++
>   3 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 71a0201437a9..0da6dcc5bebd 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1052,6 +1052,32 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state)
>   		new_crtc_state->update_planes |= BIT(plane->id);
>   	}
>   
> +	/*
> +	 * 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.
> +	 *
> +	 * So if were enabling a LUT for the first time or changing the table
> +	 * size, then we must do this before the commit to avoid corrupting
> +	 * the first lines of the display.
> +	 */
> +	if (!old_crtc_state->base.gamma_lut && new_crtc_state->base.gamma_lut)
> +		new_crtc_state->load_lut_before_commit = true;
> +	else if (!old_crtc_state->base.degamma_lut &&
> +		 new_crtc_state->base.degamma_lut)
> +		new_crtc_state->load_lut_before_commit = true;
> +	else if (old_crtc_state->base.gamma_lut &&
> +		 new_crtc_state->base.gamma_lut &&
> +		 lut_is_legacy(old_crtc_state->base.gamma_lut) !=
> +			lut_is_legacy(new_crtc_state->base.gamma_lut))
> +		new_crtc_state->load_lut_before_commit = true;
> +	else
> +		new_crtc_state->load_lut_before_commit = false;
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index aa54bb22796d..21442b0dd134 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14033,6 +14033,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);
> @@ -14529,6 +14530,12 @@ static void intel_begin_crtc_commit(struct intel_atomic_state *state,
>   		intel_atomic_get_new_crtc_state(state, crtc);
>   	bool modeset = needs_modeset(new_crtc_state);
>   
> +	if (!modeset &&
> +	    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);
> +
>   	/* Perform vblank evasion around commit operation */
>   	intel_pipe_update_start(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 449abaea619f..bbdeb3be64e6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -973,6 +973,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;
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel