Re: [PATCH 2/2] drm/i915/display: Move the commit_tail() disable sequence to commit_modeset_disables() hook

[Manasi Navare <manasi.d.navare@intel.com>]

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 <daniel.vetter@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx