Message ID | 20191030190815.7359-1-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915: Preload LUTs if the hw isn't currently using them | expand |
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, >
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,