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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux