Re: [PATCH] drm/i915: Preload LUTs if the hw isn't currently using them

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 30-10-2019 20:08, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The LUTs are single buffered so in order to program them without
> tearing we'd have to do it during vblank (actually to be 100%
> effective it has to happen between start of vblank and frame start).
> We have no proper mechanism for that at the moment so we just
> defer loading them after the vblank waits have happened. That
> is not quite sufficient (especially when committing multiple pipes
> whose vblanks don't line up) so the LUT load will often leak into
> the following frame causing tearing.
> 
> However in case the hardware wasn't previously using the LUT we
> can preload it before setting the enable bit (which is double
> buffered so won't tear). Let's determine if we can do such
> preloading and make it happen. Slight variation between the
> hardware requires some platforms specifics in the checks.
> 
> Hans is seeing ugly colored flash on VLV/CHV macchines (GPD win
> and Asus T100HA) when the gamma LUT gets loaded for the first
> time as the BIOS has left some junk in the LUT memory.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Fixes: 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thank you very much for this fix.

I've given it a test run and it fixes the flash. The code also looks good to me:

Tested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>   drivers/gpu/drm/i915/display/intel_atomic.c   |  1 +
>   drivers/gpu/drm/i915/display/intel_color.c    | 61 +++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_display.c  |  6 ++
>   .../drm/i915/display/intel_display_types.h    |  1 +
>   4 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 9cd6d2348a1e..c2875b10adf9 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -200,6 +200,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>   	crtc_state->update_wm_pre = false;
>   	crtc_state->update_wm_post = false;
>   	crtc_state->fifo_changed = false;
> +	crtc_state->preload_luts = false;
>   	crtc_state->wm.need_postvbl_update = false;
>   	crtc_state->fb_bits = 0;
>   	crtc_state->update_planes = 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index fa44eb73d088..d33676e82140 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1022,6 +1022,55 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
>   	dev_priv->display.color_commit(crtc_state);
>   }
>   
> +static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> +	struct intel_atomic_state *state =
> +		to_intel_atomic_state(new_crtc_state->base.state);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +
> +	return !old_crtc_state->base.gamma_lut &&
> +		!old_crtc_state->base.degamma_lut;
> +}
> +
> +static bool chv_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> +	struct intel_atomic_state *state =
> +		to_intel_atomic_state(new_crtc_state->base.state);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +
> +	/*
> +	 * CGM_PIPE_MODE is itself single buffered. We'd have to
> +	 * somehow split it out from chv_load_luts() if we wanted
> +	 * the ability to preload the GCM LUTs/CSC without tearing.
> +	 */
> +	if (old_crtc_state->cgm_mode || new_crtc_state->cgm_mode)
> +		return false;
> +
> +	return !old_crtc_state->base.gamma_lut;
> +}
> +
> +static bool glk_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> +	struct intel_atomic_state *state =
> +		to_intel_atomic_state(new_crtc_state->base.state);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +
> +	/*
> +	 * The hardware degamma is active whenever the pipe
> +	 * CSC is active. Thus even if the old state has no
> +	 * software degamma we need to avoid clobbering the
> +	 * linear hardware degamma mid scanout.
> +	 */
> +	return !old_crtc_state->csc_enable &&
> +		!old_crtc_state->base.gamma_lut;
> +}
> +
>   int intel_color_check(struct intel_crtc_state *crtc_state)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> @@ -1165,6 +1214,8 @@ static int i9xx_color_check(struct intel_crtc_state *crtc_state)
>   	if (ret)
>   		return ret;
>   
> +	crtc_state->preload_luts = intel_can_preload_luts(crtc_state);
> +
>   	return 0;
>   }
>   
> @@ -1217,6 +1268,8 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
>   	if (ret)
>   		return ret;
>   
> +	crtc_state->preload_luts = chv_can_preload_luts(crtc_state);
> +
>   	return 0;
>   }
>   
> @@ -1271,6 +1324,8 @@ static int ilk_color_check(struct intel_crtc_state *crtc_state)
>   	if (ret)
>   		return ret;
>   
> +	crtc_state->preload_luts = intel_can_preload_luts(crtc_state);
> +
>   	return 0;
>   }
>   
> @@ -1328,6 +1383,8 @@ static int ivb_color_check(struct intel_crtc_state *crtc_state)
>   	if (ret)
>   		return ret;
>   
> +	crtc_state->preload_luts = intel_can_preload_luts(crtc_state);
> +
>   	return 0;
>   }
>   
> @@ -1366,6 +1423,8 @@ static int glk_color_check(struct intel_crtc_state *crtc_state)
>   	if (ret)
>   		return ret;
>   
> +	crtc_state->preload_luts = glk_can_preload_luts(crtc_state);
> +
>   	return 0;
>   }
>   
> @@ -1415,6 +1474,8 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>   
>   	crtc_state->csc_mode = icl_csc_mode(crtc_state);
>   
> +	crtc_state->preload_luts = intel_can_preload_luts(crtc_state);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a23375621185..dd655ba3560c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14201,6 +14201,11 @@ static void intel_update_crtc(struct intel_crtc *crtc,
>   		/* vblanks work again, re-enable pipe CRC. */
>   		intel_crtc_enable_pipe_crc(crtc);
>   	} else {
> +		if (new_crtc_state->preload_luts &&
> +		    (new_crtc_state->base.color_mgmt_changed ||
> +		     new_crtc_state->update_pipe))
> +			intel_color_load_luts(new_crtc_state);
> +
>   		intel_pre_plane_update(old_crtc_state, new_crtc_state);
>   
>   		if (new_crtc_state->update_pipe)
> @@ -14713,6 +14718,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->preload_luts &&
>   		    (new_crtc_state->base.color_mgmt_changed ||
>   		     new_crtc_state->update_pipe))
>   			intel_color_load_luts(new_crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 40184e823c84..ae332115daee 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -775,6 +775,7 @@ struct intel_crtc_state {
>   	bool disable_cxsr;
>   	bool update_wm_pre, update_wm_post; /* watermarks are updated */
>   	bool fifo_changed; /* FIFO split is changed */
> +	bool preload_luts;
>   
>   	/* Pipe source size (ie. panel fitter input size)
>   	 * All planes will be positioned inside this space,
> 

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