Re: [PATCH 02/13] drm/i915: Eliminate usage of pipe_wm_parameters from ILK-style WM

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Aug 26, 2015 at 08:37:41AM -0700, Matt Roper wrote:
> On Wed, Aug 26, 2015 at 04:39:12PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 20, 2015 at 06:11:53PM -0700, Matt Roper wrote:
> > > Just pull the info out of the CRTC state structure rather than staging
> > > it in an additional structure.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 84 ++++++++++++++---------------------------
> > >  1 file changed, 28 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index c9cf7cf..d82ea82 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -1787,12 +1787,6 @@ struct skl_pipe_wm_parameters {
> > >  	struct intel_plane_wm_parameters cursor;
> > >  };
> > >  
> > > -struct ilk_pipe_wm_parameters {
> > > -	bool active;
> > > -	uint32_t pipe_htotal;
> > > -	uint32_t pixel_rate;
> > > -};
> > > -
> > >  struct ilk_wm_maximums {
> > >  	uint16_t pri;
> > >  	uint16_t spr;
> > > @@ -1811,7 +1805,7 @@ struct intel_wm_config {
> > >   * For both WM_PIPE and WM_LP.
> > >   * mem_value must be in 0.1us units.
> > >   */
> > > -static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
> > > +static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
> > >  				   const struct intel_plane_state *pstate,
> > >  				   uint32_t mem_value,
> > >  				   bool is_lp)
> > > @@ -1819,16 +1813,16 @@ static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
> > >  	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > >  	uint32_t method1, method2;
> > >  
> > > -	if (!params->active || !pstate->visible)
> > > +	if (!cstate->base.active || !pstate->visible)
> > >  		return 0;
> > 
> > Previously we lookd at crtc->active, now we look at state->base.active.
> > Since the atomic modeset code is a bit insane and doesn't have an
> > intermediate atomic state for the disable->re-enable case, I'm not sure
> > this will fly currently.
> > 
> > I'd be much happier if someone added the intermediate atomic state to
> > handle pipe disabling in a nicer way. Afterwards we should be able to
> > elimiate all special cases dealing with watermarks and just do the wm
> > updates from the commit_planes (or whatever it was called).
> 
> I think I'm missing some of the context for your comment.  At the moment
> we update watermarks after vblank if the CRTC is being re-enabled and
> before the vblank in other cases, so it seems like if we use
> crtc->active here, then when we get to the post-vblank watermark update
> we won't have any watermark values since we'll be acting on the old data
> that says the CRTC is still off.
> 
> When you talk about intermediate atomic state are you saying that you
> want to see CRTC enables take a two step process where the CRTC gets
> enabled with no planes first, then all the planes get turned on during
> the next vblank?

What I think should happen is something like this:

// whatever the user wanted
compute_final_atomic_state()

// include all crtcs in the intermediate state which are
// getting disabled (even temporarily to perform a modeset)
compute_intermediate_atomic_state()

ret = check_state_change(old, intermediate)
ret = check_state_change(intermediate, new)

// commit all planes in on go to make the pop out as atomically as
// possible
for_each_crtc_in(intermediate) {
        commit_planes()
}

for_each_crtc_in(intermediate) {
        disable_crtc()
}

for_each_crtc_in(new) {
        if (!currently_active)
                crtc_enable()
}

// commit all planes in on go to make the pop in as atomically as
// possible
for_each_crtc_in(new) {
        commit_planes()
}

> 
> 
> Matt
> 
> > 
> > >  
> > > -	method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
> > > +	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), bpp, mem_value);
> > >  
> > >  	if (!is_lp)
> > >  		return method1;
> > >  
> > > -	method2 = ilk_wm_method2(params->pixel_rate,
> > > -				 params->pipe_htotal,
> > > +	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> > > +				 cstate->base.adjusted_mode.crtc_htotal,
> > >  				 drm_rect_width(&pstate->dst),
> > >  				 bpp,
> > >  				 mem_value);
> > > @@ -1840,19 +1834,19 @@ static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters *params,
> > >   * For both WM_PIPE and WM_LP.
> > >   * mem_value must be in 0.1us units.
> > >   */
> > > -static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters *params,
> > > +static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
> > >  				   const struct intel_plane_state *pstate,
> > >  				   uint32_t mem_value)
> > >  {
> > >  	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > >  	uint32_t method1, method2;
> > >  
> > > -	if (!params->active || !pstate->visible)
> > > +	if (!cstate->base.active || !pstate->visible)
> > >  		return 0;
> > >  
> > > -	method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
> > > -	method2 = ilk_wm_method2(params->pixel_rate,
> > > -				 params->pipe_htotal,
> > > +	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), bpp, mem_value);
> > > +	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> > > +				 cstate->base.adjusted_mode.crtc_htotal,
> > >  				 drm_rect_width(&pstate->dst),
> > >  				 bpp,
> > >  				 mem_value);
> > > @@ -1863,30 +1857,30 @@ static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters *params,
> > >   * For both WM_PIPE and WM_LP.
> > >   * mem_value must be in 0.1us units.
> > >   */
> > > -static uint32_t ilk_compute_cur_wm(const struct ilk_pipe_wm_parameters *params,
> > > +static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
> > >  				   const struct intel_plane_state *pstate,
> > >  				   uint32_t mem_value)
> > >  {
> > >  	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > >  
> > > -	if (!params->active || !pstate->visible)
> > > +	if (!cstate->base.active || !pstate->visible)
> > >  		return 0;
> > >  
> > > -	return ilk_wm_method2(params->pixel_rate,
> > > -			      params->pipe_htotal,
> > > +	return ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> > > +			      cstate->base.adjusted_mode.crtc_htotal,
> > >  			      drm_rect_width(&pstate->dst),
> > >  			      bpp,
> > >  			      mem_value);
> > >  }
> > >  
> > >  /* Only for WM_LP. */
> > > -static uint32_t ilk_compute_fbc_wm(const struct ilk_pipe_wm_parameters *params,
> > > +static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
> > >  				   const struct intel_plane_state *pstate,
> > >  				   uint32_t pri_val)
> > >  {
> > >  	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > >  
> > > -	if (!params->active || !pstate->visible)
> > > +	if (!cstate->base.active || !pstate->visible)
> > >  		return 0;
> > >  
> > >  	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->dst), bpp);
> > > @@ -2056,7 +2050,7 @@ static bool ilk_validate_wm_level(int level,
> > >  static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
> > >  				 const struct intel_crtc *intel_crtc,
> > >  				 int level,
> > > -				 const struct ilk_pipe_wm_parameters *p,
> > > +				 struct intel_crtc_state *cstate,
> > >  				 struct intel_wm_level *result)
> > >  {
> > >  	struct intel_plane *intel_plane;
> > > @@ -2077,18 +2071,18 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
> > >  
> > >  		switch (intel_plane->base.type) {
> > >  		case DRM_PLANE_TYPE_PRIMARY:
> > > -			result->pri_val = ilk_compute_pri_wm(p, pstate,
> > > +			result->pri_val = ilk_compute_pri_wm(cstate, pstate,
> > >  							     pri_latency,
> > >  							     level);
> > > -			result->fbc_val = ilk_compute_fbc_wm(p, pstate,
> > > +			result->fbc_val = ilk_compute_fbc_wm(cstate, pstate,
> > >  							     result->pri_val);
> > >  			break;
> > >  		case DRM_PLANE_TYPE_OVERLAY:
> > > -			result->spr_val = ilk_compute_spr_wm(p, pstate,
> > > +			result->spr_val = ilk_compute_spr_wm(cstate, pstate,
> > >  							     spr_latency);
> > >  			break;
> > >  		case DRM_PLANE_TYPE_CURSOR:
> > > -			result->cur_val = ilk_compute_cur_wm(p, pstate,
> > > +			result->cur_val = ilk_compute_cur_wm(cstate, pstate,
> > >  							     cur_latency);
> > >  			break;
> > >  		}
> > > @@ -2352,19 +2346,6 @@ static void skl_setup_wm_latency(struct drm_device *dev)
> > >  	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
> > >  }
> > >  
> > > -static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> > > -				      struct ilk_pipe_wm_parameters *p)
> > > -{
> > > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > -
> > > -	if (!intel_crtc->active)
> > > -		return;
> > > -
> > > -	p->active = true;
> > > -	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> > > -	p->pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
> > > -}
> > > -
> > >  static void ilk_compute_wm_config(struct drm_device *dev,
> > >  				  struct intel_wm_config *config)
> > >  {
> > > @@ -2384,10 +2365,10 @@ static void ilk_compute_wm_config(struct drm_device *dev,
> > >  }
> > >  
> > >  /* Compute new watermarks for the pipe */
> > > -static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > > -				  const struct ilk_pipe_wm_parameters *params,
> > > +static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> > >  				  struct intel_pipe_wm *pipe_wm)
> > >  {
> > > +	struct drm_crtc *crtc = cstate->base.crtc;
> > >  	struct drm_device *dev = crtc->dev;
> > >  	const struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > @@ -2412,8 +2393,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > >  		drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> > >  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
> > >  
> > > -
> > > -	pipe_wm->pipe_enabled = params->active;
> > > +	pipe_wm->pipe_enabled = cstate->base.active;
> > >  	pipe_wm->sprites_enabled = sprstate->visible;
> > >  	pipe_wm->sprites_scaled = config.sprites_scaled;
> > >  
> > > @@ -2425,7 +2405,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > >  	if (config.sprites_scaled)
> > >  		max_level = 0;
> > >  
> > > -	ilk_compute_wm_level(dev_priv, intel_crtc, 0, params, &pipe_wm->wm[0]);
> > > +	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, &pipe_wm->wm[0]);
> > >  
> > >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > >  		pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> > > @@ -2442,7 +2422,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > >  	for (level = 1; level <= max_level; level++) {
> > >  		struct intel_wm_level wm = {};
> > >  
> > > -		ilk_compute_wm_level(dev_priv, intel_crtc, level, params, &wm);
> > > +		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, &wm);
> > >  
> > >  		/*
> > >  		 * Disable any watermark level that exceeds the
> > > @@ -3748,19 +3728,17 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  static void ilk_update_wm(struct drm_crtc *crtc)
> > >  {
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct ilk_wm_maximums max;
> > > -	struct ilk_pipe_wm_parameters params = {};
> > >  	struct ilk_wm_values results = {};
> > >  	enum intel_ddb_partitioning partitioning;
> > >  	struct intel_pipe_wm pipe_wm = {};
> > >  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
> > >  	struct intel_wm_config config = {};
> > >  
> > > -	ilk_compute_wm_parameters(crtc, &params);
> > > -
> > > -	intel_compute_pipe_wm(crtc, &params, &pipe_wm);
> > > +	intel_compute_pipe_wm(cstate, &pipe_wm);
> > >  
> > >  	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
> > >  		return;
> > > @@ -3800,12 +3778,6 @@ ilk_update_sprite_wm(struct drm_plane *plane,
> > >  	struct drm_device *dev = plane->dev;
> > >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > >  
> > > -	intel_plane->wm.enabled = enabled;
> > > -	intel_plane->wm.scaled = scaled;
> > > -	intel_plane->wm.horiz_pixels = sprite_width;
> > > -	intel_plane->wm.vert_pixels = sprite_width;
> > > -	intel_plane->wm.bytes_per_pixel = pixel_size;
> > > -
> > >  	/*
> > >  	 * IVB workaround: must disable low power watermarks for at least
> > >  	 * one frame before enabling scaling.  LP watermarks can be re-enabled
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx