| Message ID | 20191028185851.592922-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Program LUT before intel_color_commit() if LUT was not previously set v2 | expand |
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
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; + 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); 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;
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(-)