On Fri, Oct 25, 2019 at 09:23:47PM +0200, Hans de Goede wrote:
> Hi,
>
> On 21-10-2019 16:39, Ville Syrjälä wrote:
> > On Sun, Oct 20, 2019 at 08:19:33PM +0200, 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.
> >
> > The real solution is vblank workers, which I have somewhat implemented
> > here:
> > git://github.com/vsyrjala/linux.git vblank_worker_8_kthread
> >
> > Though even with the qos tricks there we still probably can't quite make
> > it in time. Essentially we have a bit less than one scanline after start
> > of vblank to do the work before pixels start to flow through the pipe.
> > We might be extend that to almost four scanlines but that partocular
> > thing is documeted as debug feature so not sure we should really use it.
> > Also I don't think four scanlines is always enough either. So it's still
> > very much possible that we get the first 100 or so pixels with the old LUT.
>
> Thank you for the info and for the review.
>
>
> >> 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 | 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;
> >
> > The 'no gamma -> yes gamma' thing I might be willing to accept. The rest
> > not so much. I was already pondering about such optimizations for the
> > plane gamma/csc stuff in my vblank branch.
>
> Ok, so I can submit a v2 based on dinq with only the
> if (!old_crtc_state->base.gamma_lut && new_crtc_state->base.gamma_lut)
> check, or
>
> > But for the fastboot case I think what we could do is just sanitize
> > the LUT(s) after readout if gamma wasn't enabled by the BIOS.
>
> We could do this, but this falls a bit outside of my expertise, I would be
> more then happy to test a patch on one of the machines which needs this
> LUTS sanitizing though. I get a very visible flash of a couple of bright
> blue or yellow (2 different machines) on the upper few lines the first
> time the gamma table gets loaded, so verifying that the sanitation kicks
> in is easy.
Actually, how recent is your kernel?
Some issues with the gamma readout were fixed quite recently:
9b000b47cc18 ("drm/i915/color: fix broken gamma state-checker during boot")
d50341274d01 ("drm/i915/color: move check of gamma_enable to specific func/platform")
If those don't help we could try the quick path and just blast the
gamma from orbit like so:
@@ -16787,18 +16787,23 @@ static int intel_initial_commit(struct drm_device *dev)
goto out;
}
+ /*
+ * Let's just turn off the BIOS leftover LUT(s).
+ *
+ * FIXME maybe we shouldn't do this, and instead
+ * we should let fb_helper/whatever replace the
+ * LUT(s) when they start to actually render stuff.
+ * But for now we may get an ugly flash due to
+ * non-atomic gamma updates.
+ */
+ drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
+ drm_property_replace_blob(&crtc_state->gamma_lut, NULL);
+ crtc_state->color_mgmt_changed = true;
+
if (crtc_state->active) {
ret = drm_atomic_add_affected_planes(state, crtc);
if (ret)
goto out;
-
- /*
- * FIXME hack to force a LUT update to avoid the
- * plane update forcing the pipe gamma on without
- * having a proper LUT loaded. Remove once we
- * have readout for pipe gamma enable.
- */
- crtc_state->color_mgmt_changed = true;
}
}
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
