[Intel-gfx] [PATCH] drm/i915: Preload LUTs if the hw isn't currently using them
Hans de Goede
hdegoede at redhat.com
Tue Nov 5 20:41:58 UTC 2019
Hi,
On 30-10-2019 20:08, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at redhat.com>
> Fixes: 051a6d8d3ca0 ("drm/i915: Move LUT programming to happen after vblank waits")
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at redhat.com>
Reviewed-by: Hans de Goede <hdegoede at 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,
>
More information about the Intel-gfx
mailing list