On Wed, Aug 28, 2019 at 08:30:23AM +0200, Maarten Lankhorst wrote: > Op 28-08-2019 om 00:17 schreef Manasi Navare: > > Create a new hook commit_modeset_disables() consistent with the naming > > in drm atomic helpers and similar to the commit_modeset_enables() hook. > > This helps better organize the disable sequence in atomic_commit_tail() > > and move that to this disable hook. > > > > No functional change > > > > v3: > > * Rebase (Manasi) > > v2: > > * Create a helper for old_crtc_state disables (Lucas) > > > > Suggested-by: Daniel Vetter <[email protected]> > > Cc: Ville Syrjälä <[email protected]> > > Cc: Maarten Lankhorst <[email protected]> > > Cc: Matt Roper <[email protected]> > > Cc: Jani Nikula <[email protected]> > > Signed-off-by: Manasi Navare <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 94 +++++++++++++------- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 2 files changed, 61 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 23c8c7f5c47a..7f0ab0101569 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13758,6 +13758,61 @@ static void intel_update_crtc(struct intel_crtc > > *crtc, > > intel_finish_crtc_commit(state, crtc); > > } > > > > +static void intel_old_crtc_state_disables(struct intel_atomic_state *state, > > + struct intel_crtc_state > > *old_crtc_state, > > + struct intel_crtc_state > > *new_crtc_state, > > + struct intel_crtc *crtc) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + > > + intel_crtc_disable_planes(state, crtc); > > + > > + /* > > + * We need to disable pipe CRC before disabling the pipe, > > + * or we race against vblank off. > > + */ > > + intel_crtc_disable_pipe_crc(crtc); > > + > > + dev_priv->display.crtc_disable(old_crtc_state, state); > > + crtc->active = false; > > + intel_fbc_disable(crtc); > > + intel_disable_shared_dpll(old_crtc_state); > > + > > + /* > > + * Underruns don't always raise interrupts, > > + * so check manually. > > + */ > > + intel_check_cpu_fifo_underruns(dev_priv); > > + intel_check_pch_fifo_underruns(dev_priv); > > + > > + /* FIXME unify this for all platforms */ > > + if (!new_crtc_state->base.active && > > + !HAS_GMCH(dev_priv) && > > + dev_priv->display.initial_watermarks) > > + dev_priv->display.initial_watermarks(state, > > + new_crtc_state); > > +} > > + > > +static void intel_commit_modeset_disables(struct intel_atomic_state *state) > > +{ > > + struct intel_crtc_state *new_crtc_state, *old_crtc_state; > > + struct intel_crtc *crtc; > > + int i; > > + > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > new_crtc_state, i) { > > + if (!needs_modeset(new_crtc_state)) > > + continue; > > + > > + intel_pre_plane_update(old_crtc_state, new_crtc_state); > > + > > + if (old_crtc_state->base.active) > > + intel_old_crtc_state_disables(state, > > + old_crtc_state, > > + new_crtc_state, > > + crtc); > > + } > > +} > > + > > static void intel_commit_modeset_enables(struct intel_atomic_state *state) > > { > > struct intel_crtc *crtc; > > @@ -13938,42 +13993,10 @@ static void intel_atomic_commit_tail(struct > > intel_atomic_state *state) > > put_domains[crtc->pipe] = > > modeset_get_crtc_power_domains(new_crtc_state); > > } > > - > > - if (!needs_modeset(new_crtc_state)) > > - continue; > > - > > - intel_pre_plane_update(old_crtc_state, new_crtc_state); > > - > > - if (old_crtc_state->base.active) { > > - intel_crtc_disable_planes(state, crtc); > > - > > - /* > > - * We need to disable pipe CRC before disabling the > > pipe, > > - * or we race against vblank off. > > - */ > > - intel_crtc_disable_pipe_crc(crtc); > > - > > - dev_priv->display.crtc_disable(old_crtc_state, state); > > - crtc->active = false; > > - intel_fbc_disable(crtc); > > - intel_disable_shared_dpll(old_crtc_state); > > - > > - /* > > - * Underruns don't always raise > > - * interrupts, so check manually. > > - */ > > - intel_check_cpu_fifo_underruns(dev_priv); > > - intel_check_pch_fifo_underruns(dev_priv); > > - > > - /* FIXME unify this for all platforms */ > > - if (!new_crtc_state->base.active && > > - !HAS_GMCH(dev_priv) && > > - dev_priv->display.initial_watermarks) > > - dev_priv->display.initial_watermarks(state, > > - > > new_crtc_state); > > - } > > } > > > > + dev_priv->display.commit_modeset_disables(state); > > + > > /* FIXME: Eventually get rid of our crtc->config pointer */ > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) > > crtc->config = new_crtc_state; > > @@ -15929,6 +15952,9 @@ void intel_init_display_hooks(struct > > drm_i915_private *dev_priv) > > dev_priv->display.commit_modeset_enables = > > skl_commit_modeset_enables; > > else > > dev_priv->display.commit_modeset_enables = > > intel_commit_modeset_enables; > > + > > + dev_priv->display.commit_modeset_disables = > > intel_commit_modeset_disables; > > + > > } > > > > static i915_reg_t i915_vgacntrl_reg(struct drm_i915_private *dev_priv) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 75a42e8df67e..db7480831e52 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -289,6 +289,7 @@ struct drm_i915_display_funcs { > > void (*crtc_disable)(struct intel_crtc_state *old_crtc_state, > > struct intel_atomic_state *old_state); > > void (*commit_modeset_enables)(struct intel_atomic_state *state); > > + void (*commit_modeset_disables)(struct intel_atomic_state *state); > > void (*audio_codec_enable)(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state, > > const struct drm_connector_state > > *conn_state); > > Hey, > > Nack, there is no need to start introducing a special case here. The same > enable and disable sequence for gen11 will still work all the way back to > gen9 if transcoder sync is not used. :) For disable all the way back to gen2. >
So just use the same function for all gens and do special handiling only if trans_port_sync_mode() in that same function? Danvet had suggested adding a separate hook for ICL since this feature is supported for ICL+ and having it in separate hook avoids possibility of regressions. But if you strongly feel against it I will not create another hook. > It could still be beneficial to make those separate functions, just no > function pointer. So you mean not create a new commit_modeset_disables hook? Just extract the intel_old_crtc_state_disables() from atomic_commit_tail()? Manasi > > intel_old_crtc_state_disables( > _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
