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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
