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

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

[Hans de Goede]

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>
---
 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;
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BUILD: failure for drm/i915: Program LUT before intel_color_commit() if LUT was not previously set

[Patchwork]

== Series Details ==

Series: drm/i915: Program LUT before intel_color_commit() if LUT was not previously set
URL   : https://patchwork.freedesktop.org/series/68278/
State : failure

== Summary ==

Applying: drm/i915: Program LUT before intel_color_commit() if LUT was not previously set
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/display/intel_color.c
M	drivers/gpu/drm/i915/display/intel_display.c
M	drivers/gpu/drm/i915/display/intel_display_types.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/display/intel_display_types.h
Auto-merging drivers/gpu/drm/i915/display/intel_display.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_display.c
Auto-merging drivers/gpu/drm/i915/display/intel_color.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Program LUT before intel_color_commit() if LUT was not previously set
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Hans de Goede]

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

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

[Ville Syrjälä]

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.

> 
> 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    | 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.

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.

> +
>  	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;
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Hans de Goede]

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@redhat.com>
>> ---
>>   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.

Regards,

Hans






> 
>> +
>>   	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;
>> -- 
>> 2.23.0
> 

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

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

[Hans de Goede]

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@redhat.com>
>> ---
>>   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.

Regards,

Hans






> 
>> +
>>   	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;
>> -- 
>> 2.23.0
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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@redhat.com>
> >> ---
> >>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

[Hans de Goede]

Hi,

On 25-10-2019 21:45, Ville Syrjälä wrote:
> 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@redhat.com>
>>>> ---
>>>>    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")

I was testing with 5.4-rc# which does not have these commits. I've switched
to dinq but that does not help.

> 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;
>   		}
>   	}
> 

Ok, I've given this a try, but this also does not help, which is somewhat
expected. We only load the LUT when crtc_state->gamma_lut != NULL so
I believe that explicitly setting to NULL from intel_initial_commit() does
not make a difference, as the PIPECONF_GAMMA_MODE bits in the PIPECONF register
are already set to disabled.

If anything we should force load a linear gamma table to the pipe's LUT at
probe time when gamma was not enabled by the GOP, rather then setting the
gamma_lut to NULL.

My theory is that the LUT is *not* used by the GOP so crtc_state->gamma_enable
is initially false. The problem happens when userspace then actually sets
the gamma_lut blob. My theory is as follows:

1. GOP initializes DSI panel on CHT, does not use the "legacy gamma LUT"
2. i915 driver loads, all is well
3. plymouth does a drmModeCrtcSetGamma
4. intel_color.c sets crtc_state->gamma_enable to true on the commit
5. Since commit 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
    we program the double-bufferred PIPECONF register which has the legacy gamma enable bit
    before the vblank;
6. While we upload the new legacy gamma LUT values after vblank.
7. This causes the first few lines of the frame after the vblank to use the old LUT
    values and the old values appear to contain noise (this seems to be the same
    noise every boot given a certain model hw). This noise leads to a bright blue or yellow
    (depending on the hw) flash in the first couple of lines.

I will submit a new version of my patch checking only gamma_enable as you requested,
then we'll see from there.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Hans de Goede]

Hi,

On 25-10-2019 21:45, Ville Syrjälä wrote:
> 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@redhat.com>
>>>> ---
>>>>    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")

I was testing with 5.4-rc# which does not have these commits. I've switched
to dinq but that does not help.

> 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;
>   		}
>   	}
> 

Ok, I've given this a try, but this also does not help, which is somewhat
expected. We only load the LUT when crtc_state->gamma_lut != NULL so
I believe that explicitly setting to NULL from intel_initial_commit() does
not make a difference, as the PIPECONF_GAMMA_MODE bits in the PIPECONF register
are already set to disabled.

If anything we should force load a linear gamma table to the pipe's LUT at
probe time when gamma was not enabled by the GOP, rather then setting the
gamma_lut to NULL.

My theory is that the LUT is *not* used by the GOP so crtc_state->gamma_enable
is initially false. The problem happens when userspace then actually sets
the gamma_lut blob. My theory is as follows:

1. GOP initializes DSI panel on CHT, does not use the "legacy gamma LUT"
2. i915 driver loads, all is well
3. plymouth does a drmModeCrtcSetGamma
4. intel_color.c sets crtc_state->gamma_enable to true on the commit
5. Since commit 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
    we program the double-bufferred PIPECONF register which has the legacy gamma enable bit
    before the vblank;
6. While we upload the new legacy gamma LUT values after vblank.
7. This causes the first few lines of the frame after the vblank to use the old LUT
    values and the old values appear to contain noise (this seems to be the same
    noise every boot given a certain model hw). This noise leads to a bright blue or yellow
    (depending on the hw) flash in the first couple of lines.

I will submit a new version of my patch checking only gamma_enable as you requested,
then we'll see from there.

Regards,

Hans

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

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

[Hans de Goede]

Hi,

On 25-10-2019 21:45, Ville Syrjälä wrote:
> 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@redhat.com>
>>>> ---
>>>>    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")

I was testing with 5.4-rc# which does not have these commits. I've switched
to dinq but that does not help.

> 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;
>   		}
>   	}
> 

Ok, I've given this a try, but this also does not help, which is somewhat
expected. We only load the LUT when crtc_state->gamma_lut != NULL so
I believe that explicitly setting to NULL from intel_initial_commit() does
not make a difference, as the PIPECONF_GAMMA_MODE bits in the PIPECONF register
are already set to disabled.

If anything we should force load a linear gamma table to the pipe's LUT at
probe time when gamma was not enabled by the GOP, rather then setting the
gamma_lut to NULL.

My theory is that the LUT is *not* used by the GOP so crtc_state->gamma_enable
is initially false. The problem happens when userspace then actually sets
the gamma_lut blob. My theory is as follows:

1. GOP initializes DSI panel on CHT, does not use the "legacy gamma LUT"
2. i915 driver loads, all is well
3. plymouth does a drmModeCrtcSetGamma
4. intel_color.c sets crtc_state->gamma_enable to true on the commit
5. Since commit 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
    we program the double-bufferred PIPECONF register which has the legacy gamma enable bit
    before the vblank;
6. While we upload the new legacy gamma LUT values after vblank.
7. This causes the first few lines of the frame after the vblank to use the old LUT
    values and the old values appear to contain noise (this seems to be the same
    noise every boot given a certain model hw). This noise leads to a bright blue or yellow
    (depending on the hw) flash in the first couple of lines.

I will submit a new version of my patch checking only gamma_enable as you requested,
then we'll see from there.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx