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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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.
It can't go wrong as long as it is done before swapping state. We don't write
any plane state so worst that can happen is following:

thread1:
- grab crtc lock
- inspect plane state

thread2:
- grab plane lock
- duplicate plane state, only part that can race with thread1. This is why the iterator has to be const.
- grab crtc lock, wait for thread1 to complete or begin its nonblocking update
- commit and swap state

For things like watermark recalculations it's perfectly safe. Worst that can happen is that either thread
has to back off early with -EDEADLK, in which case the calculations are thrown away when starting over.
>> 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's done on skylake because the per crtc allocations have to be changed when a plane is updated.

Adding other planes to the state unnecessarily may introduce extra waits, which is tested for example by

kms_cursor_legacy.flip-vs-cursor-busy-crc-legacy

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx