Re: [PATCH 07/14] drm/i915: Compute vlv/chv wms the atomic way

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

On Thu, Dec 15, 2016 at 06:12:12PM +0100, Maarten Lankhorst wrote:
> Op 15-12-16 om 17:09 schreef Ville Syrjälä:
> > On Thu, Dec 15, 2016 at 04:45:49PM +0100, Maarten Lankhorst wrote:
> >> Op 15-12-16 om 16:38 schreef Ville Syrjälä:
> >>> On Thu, Dec 15, 2016 at 04:30:54PM +0100, Maarten Lankhorst wrote:
> >>>> Op 12-12-16 om 21:35 schreef ville.syrjala@linux.intel.com:
> >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>
> >>>>> Start computing the vlv/chv watermarks the atomic way, from the
> >>>>> .compute_pipe_wm() hook. We'll recompute the actual watermarks
> >>>>> for only planes that are part of the state, the other planes will
> >>>>> keep their watermark from the last time it was computed.
> >>>>>
> >>>>> And the actual watermark programming will happen from the
> >>>>> .initial_watermarks() hook. For now we'll just compute the
> >>>>> optimal watermarks, and we'll hook up the intermediate
> >>>>> watermarks properly later.
> >>>>>
> >>>>> The DSPARB registers responsible for the FIFO paritioning are
> >>>>> double buffered, so they will be programming from
> >>>>> intel_begin_crtc_commit().
> >>>>>
> >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/i915_drv.h      |   8 +
> >>>>>  drivers/gpu/drm/i915/intel_display.c |  21 ++-
> >>>>>  drivers/gpu/drm/i915/intel_drv.h     |   2 -
> >>>>>  drivers/gpu/drm/i915/intel_pm.c      | 327 +++++++++++++++++++++++------------
> >>>>>  4 files changed, 238 insertions(+), 120 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>> index 20bc04d5e617..f23698f99685 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>> @@ -493,6 +493,14 @@ struct i915_hotplug {
> >>>>>  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
> >>>>>  		for_each_if ((1 << (domain)) & (mask))
> >>>>>  
> >>>>> +#define for_each_intel_plane_in_state(__state, plane, plane_state, __i) \
> >>>>> +	for ((__i) = 0; \
> >>>>> +	     (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> >>>>> +		     ((plane) = to_intel_plane((__state)->base.planes[__i].ptr), \
> >>>>> +		      (plane_state) = to_intel_plane_state((__state)->base.planes[__i].state), 1); \
> >>>>> +	     (__i)++) \
> >>>>> +		for_each_if (plane_state)
> >>>>> +
> >>>>>  struct drm_i915_private;
> >>>>>  struct i915_mm_struct;
> >>>>>  struct i915_mmu_object;
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 3f027341b0f3..8d80873b6643 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -6736,6 +6736,8 @@ static void valleyview_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> >>>>>  static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
> >>>>>  				   struct drm_atomic_state *old_state)
> >>>>>  {
> >>>>> +	struct intel_atomic_state *old_intel_state =
> >>>>> +		to_intel_atomic_state(old_state);
> >>>>>  	struct drm_crtc *crtc = pipe_config->base.crtc;
> >>>>>  	struct drm_device *dev = crtc->dev;
> >>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>>> @@ -6780,7 +6782,8 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
> >>>>>  
> >>>>>  	intel_color_load_luts(&pipe_config->base);
> >>>>>  
> >>>>> -	intel_update_watermarks(intel_crtc);
> >>>>> +	dev_priv->display.initial_watermarks(old_intel_state,
> >>>>> +					     pipe_config);
> >>>>>  	intel_enable_pipe(intel_crtc);
> >>>>>  
> >>>>>  	assert_vblank_disabled(crtc);
> >>>>> @@ -6897,6 +6900,9 @@ static void i9xx_crtc_disable(struct intel_crtc_state *old_crtc_state,
> >>>>>  
> >>>>>  	if (!IS_GEN2(dev_priv))
> >>>>>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> >>>>> +
> >>>>> +	if (!dev_priv->display.initial_watermarks)
> >>>>> +		intel_update_watermarks(intel_crtc);
> >>>>>  }
> >>>>>  
> >>>>>  static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> >>>>> @@ -12980,10 +12986,13 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
> >>>>>  static void
> >>>>>  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >>>>>  {
> >>>>> +	struct drm_i915_private *dev_priv =
> >>>>> +		to_i915(crtc_state->base.crtc->dev);
> >>>>>  	struct drm_crtc_state tmp_state;
> >>>>>  	struct intel_crtc_scaler_state scaler_state;
> >>>>>  	struct intel_dpll_hw_state dpll_hw_state;
> >>>>>  	struct intel_shared_dpll *shared_dpll;
> >>>>> +	struct intel_crtc_wm_state wm_state;
> >>>>>  	bool force_thru;
> >>>>>  
> >>>>>  	/* FIXME: before the switch to atomic started, a new pipe_config was
> >>>>> @@ -12996,6 +13005,8 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >>>>>  	shared_dpll = crtc_state->shared_dpll;
> >>>>>  	dpll_hw_state = crtc_state->dpll_hw_state;
> >>>>>  	force_thru = crtc_state->pch_pfit.force_thru;
> >>>>> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >>>>> +		wm_state = crtc_state->wm;
> >>>>>  
> >>>>>  	memset(crtc_state, 0, sizeof *crtc_state);
> >>>>>  
> >>>>> @@ -13004,6 +13015,8 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >>>>>  	crtc_state->shared_dpll = shared_dpll;
> >>>>>  	crtc_state->dpll_hw_state = dpll_hw_state;
> >>>>>  	crtc_state->pch_pfit.force_thru = force_thru;
> >>>>> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >>>>> +		crtc_state->wm = wm_state;
> >>>> Is this required? In skl we throw away everything and rebuild using drm_atomic_crtc_state_for_each_plane_state.
> >>> I don't want to add all the planes into the state when not necessary.
> >> It doesn't. It uses the fact that updating planes requires crtc mutex, and only iterates over all planes in crtc_state->planes_mask.
> > Which is setting a somewhat dangerous precedent. So no, I don't think I
> > want to do that. It's not immediately obvious from the code that it's
> > 100% safe either since the crtc lock is taken after the plane->state
> > is dereferenced. Which means someon can in parallel be looking at the
> > old plane state while the new state already exists. I can't think of
> > a way it could actually go wrong, but seems easier to stick to the
> > rules and avoid that particular headache entirely.
> You can't do anything with your duplicated plane state without locking crtc_mutex.
> This is what the whole -EDEADLK design will prevent. :)
> >> Any planes outside it are disabled so wm values can be considered 0, and roughly does the following:
> >>
> >> for_each_plane_in_mask(crtc_state->planes_mask)
> >> if ((!plane_state = drm_atomic_get_existing_state(state, plane_state)) plane_state = plane->state;
> >>
> >> plane_state must be const, but that's no problem for wm calculations.
> > And recomputing stuff that didn't even change just seems wasteful.
> It might change if your fifo allocations are different. Could skip the whole recalculation thing when crtc_state->planes_changed = false.

Nah. I want better granularity than that. Also FIFO allocation changes
don't require recomputing watermarks.

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