Re: [PATCH v4 3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

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

Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
> Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
>> Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
>>> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>>>> Currently we perform our own wait in post_plane_update,
>>>> but the atomic core performs another one in wait_for_vblanks.
>>>> This means that 2 vblanks are done when a fb is changed,
>>>> which is a bit overkill.
>>>>
>>>> Merge them by creating a helper function that takes a crtc mask
>>>> for the planes to wait on.
>>>>
>>>> The broadwell vblank workaround may look gone entirely but this
>>>> is
>>>> not the case. pipe_config->wm_changed is set to true
>>>> when any plane is turned on, which forces a vblank wait.
>>>>
>>>> Changes since v1:
>>>> - Removing the double vblank wait on broadwell moved to its own
>>>> commit.
>>>> Changes since v2:
>>>> - Move out POWER_DOMAIN_MODESET handling to its own commit.
>>>> Changes since v3:
>>>> - Do not wait for vblank on legacy cursor updates. (Ville)
>>>> - Move broadwell vblank workaround comment to page_flip_finished.
>>>> (Ville)
>>>> Changes since v4:
>>>> - Compile fix, legacy_cursor_flip -> *_update.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
>>>> om>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>>>>  drivers/gpu/drm/i915/intel_display.c | 86
>>>> +++++++++++++++++++++++++++---------
>>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>>>  3 files changed, 67 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 4625f8a9ba12..8e579a8505ac 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc
>>>> *crtc)
>>>>  	crtc_state->disable_lp_wm = false;
>>>>  	crtc_state->disable_cxsr = false;
>>>>  	crtc_state->wm_changed = false;
>>>> +	crtc_state->fb_changed = false;
>>>>  
>>>>  	return &crtc_state->base;
>>>>  }
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index 804f2c6f260d..4d4dddc1f970 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
>>>> intel_crtc *crtc)
>>>>  		to_intel_crtc_state(crtc->base.state);
>>>>  	struct drm_device *dev = crtc->base.dev;
>>>>  
>>>> -	if (atomic->wait_vblank)
>>>> -		intel_wait_for_vblank(dev, crtc->pipe);
>>>> -
>>>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>>>  
>>>>  	crtc->wm.cxsr_allowed = true;
>>>> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
>>>> intel_crtc *crtc)
>>>>  		return true;
>>>>  
>>>>  	/*
>>>> +	 * BDW signals flip done immediately if the plane
>>>> +	 * is disabled, even if the plane enable is already
>>>> +	 * armed to occur at the next vblank :(
>>>> +	 */
>>> Having this comment here is just... weird. I think it removes a lot
>>> of
>>> the context that was present before.
>>>
>>>> +
>>>> +	/*
>>>>  	 * A DSPSURFLIVE check isn't enough in case the mmio and
>>>> CS
>>>> flips
>>>>  	 * used the same base address. In that case the mmio
>>>> flip
>>>> might
>>>>  	 * have completed, but the CS hasn't even executed the
>>>> flip
>>>> yet.
>>>> @@ -11778,6 +11781,9 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  	if (!was_visible && !visible)
>>>>  		return 0;
>>>>  
>>>> +	if (fb != old_plane_state->base.fb)
>>>> +		pipe_config->fb_changed = true;
>>>> +
>>>>  	turn_off = was_visible && (!visible || mode_changed);
>>>>  	turn_on = visible && (!was_visible || mode_changed);
>>>>  
>>>> @@ -11793,8 +11799,6 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  
>>>>  		/* must disable cxsr around plane enable/disable
>>>> */
>>>>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>>>> -			if (is_crtc_enabled)
>>>> -				intel_crtc->atomic.wait_vblank =
>>>> true;
>>>>  			pipe_config->disable_cxsr = true;
>>>>  		}
>>> We could have killed the brackets here :)
>> Indeed, will do so in next version.
>>>>  	} else if (intel_wm_need_update(plane, plane_state)) {
>>>> @@ -11810,14 +11814,6 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  		intel_crtc->atomic.post_enable_primary =
>>>> turn_on;
>>>>  		intel_crtc->atomic.update_fbc = true;
>>>>  
>>>> -		/*
>>>> -		 * BDW signals flip done immediately if the
>>>> plane
>>>> -		 * is disabled, even if the plane enable is
>>>> already
>>>> -		 * armed to occur at the next vblank :(
>>>> -		 */
>>>> -		if (turn_on && IS_BROADWELL(dev))
>>>> -			intel_crtc->atomic.wait_vblank = true;
>>>> -
>>>>  		break;
>>>>  	case DRM_PLANE_TYPE_CURSOR:
>>>>  		break;
>>>> @@ -11831,12 +11827,10 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  		if (IS_IVYBRIDGE(dev) &&
>>>>  		    needs_scaling(to_intel_plane_state(plane_sta
>>>> te))
>>>> &&
>>>>  		    !needs_scaling(old_plane_state)) {
>>>> -			to_intel_crtc_state(crtc_state)-
>>>>> disable_lp_wm = true;
>>>> -		} else if (turn_off && !mode_changed) {
>>>> -			intel_crtc->atomic.wait_vblank = true;
>>>> +			pipe_config->disable_lp_wm = true;
>>>> +		} else if (turn_off && !mode_changed)
>>>>  			intel_crtc-
>>>>> atomic.update_sprite_watermarks
>>>>> =
>>>>  				1 << i;
>>>> -		}
>>> Weird brackets here. Either kill on both sides of the if statement
>>> (the
>>> preferred way), or none.
>>>
>>> Also, shouldn't we introduce pipe_config->sprite_changed? What's
>>> guaranteeing that we're going to definitely wait for a vblank
>>> during
>>> sprite updates? I like explicit dumb-proof code instead of
>>> implications
>>> such as "we're doing waits during sprite updates because whenever
>>> we
>>> update sprites, a specific unrelated variable that's used for a
>>> different purpose gets set to true, and we check for this
>>> variable".
>> sprite_changed would be same as fb_changed + wm_changed, and
>> update_sprite_watermarks gets removed in this series
>> because nothing uses it.
> Hmmm, right. For some reason, I was interpreting fb_changed as being
> only valid for the primary fb, not for spr/cur. My bad. So fb_changed
> means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
> could be a better name, or fbs_changed (plural), since we're talking
> about more then one possible plane here. Not a requirement, just
> throwing the idea.
planes_changed is already used, it means that any plane_state is being updated for this crtc.
fbs_changed could work though, most other *_changed are plural.
>>>>  
>>>>  		break;
>>>>  	}
>>>> @@ -13370,6 +13364,48 @@ static int
>>>> intel_atomic_prepare_commit(struct drm_device *dev,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static void intel_atomic_wait_for_vblanks(struct drm_device
>>>> *dev,
>>>> +					  struct
>>>> drm_i915_private
>>>> *dev_priv,
>>>> +					  unsigned crtc_mask)
>>>> +{
>>>> +	unsigned last_vblank_count[I915_MAX_PIPES];
>>>> +	enum pipe pipe;
>>>> +	int ret;
>>>> +
>>>> +	if (!crtc_mask)
>>>> +		return;
>>>> +
>>>> +	for_each_pipe(dev_priv, pipe) {
>>>> +		struct drm_crtc *crtc = dev_priv-
>>>>> pipe_to_crtc_mapping[pipe];
>>>> +
>>>> +		if (!((1 << pipe) & crtc_mask))
>>>> +			continue;
>>>> +
>>>> +		ret = drm_crtc_vblank_get(crtc);
>>>> +		if (ret != 0) {
>>>> +			crtc_mask &= ~(1 << pipe);
>>>> +			continue;
>>> Shouldn't we DRM_ERROR here?
>> WARN_ON is enough, this shouldn't ever happen.
> Even better :)
>
>>>> +		}
>>>> +
>>>> +		last_vblank_count[pipe] =
>>>> drm_crtc_vblank_count(crtc);
>>>> +	}
>>>> +
>>>> +	for_each_pipe(dev_priv, pipe) {
>>>> +		struct drm_crtc *crtc = dev_priv-
>>>>> pipe_to_crtc_mapping[pipe];
>>>> +
>>>> +		if (!((1 << pipe) & crtc_mask))
>>>> +			continue;
>>>> +
>>>> +		wait_event_timeout(dev->vblank[pipe].queue,
>>>> +				last_vblank_count[pipe] !=
>>>> +					drm_crtc_vblank_count(cr
>>>> tc),
>>>> +				msecs_to_jiffies(50));
>>> Also maybe DRM_ERROR in case we hit the timeout?
>>>
>>> I know the original code doesn't have this, but now that we're
>>> doing or
>>> own thing, we may scream in unexpected cases.
>> I'll make it a WARN_ON, shouldn't happen.
>>>> +
>>>> +		drm_crtc_vblank_put(crtc);
>>>> +	}
>>>> +}
>>>> +
>>>> +
>>> Two newlines :)
>> Indeed!
>>>>  /**
>>>>   * intel_atomic_commit - commit validated state object
>>>>   * @dev: DRM device
>>>> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  	int ret = 0, i;
>>>>  	bool hw_check = intel_state->modeset;
>>>>  	unsigned long put_domains[I915_MAX_PIPES] = {};
>>>> +	unsigned crtc_vblank_mask = 0;
>>>>  
>>>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>>>>  	if (ret) {
>>>> @@ -13470,8 +13507,9 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>>  		struct intel_crtc *intel_crtc =
>>>> to_intel_crtc(crtc);
>>>>  		bool modeset = needs_modeset(crtc->state);
>>>> -		bool update_pipe = !modeset &&
>>>> -			to_intel_crtc_state(crtc->state)-
>>>>> update_pipe;
>>>> +		struct intel_crtc_state *pipe_config =
>>>> +			to_intel_crtc_state(crtc->state);
>>>> +		bool update_pipe = !modeset && pipe_config-
>>>>> update_pipe;
>>>>  
>>>>  		if (modeset && crtc->state->active) {
>>>>  			update_scanline_offset(to_intel_crtc(crt
>>>> c));
>>>> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  		    (crtc->state->planes_changed ||
>>>> update_pipe))
>>>>  			drm_atomic_helper_commit_planes_on_crtc(
>>>> crtc
>>>> _state);
>>>>  
>>>> -		intel_post_plane_update(intel_crtc);
>>>> +		if (pipe_config->base.active &&
>>>> +		    (pipe_config->wm_changed || pipe_config-
>>>>> disable_cxsr ||
>>>> +		     pipe_config->fb_changed))
>>> So the wm_changed is just for the BDW workaround + sprites? What
>>> about
>>> this disable_cxsr, why is it here? Also see my comment above about
>>> sprite_changed. Maybe we need some comments here to explain the
>>> complex
>>> checks.
>> No it's waiting for a vblank for post_plane_update so all optimized
>> wm values
>> can get written, it's not just for the BDW workaround.
>> It just happens to be that the BDW w/a gets applied too as a side
>> effect.
> So maybe some comment regarding the BDW WA could be here.
>
> What about disable_cxsr? Why is this here?
That's what matches the current code, see the calc_changes hunk which had a vblank_wait = true.
>>>> +			crtc_vblank_mask |= 1 << i;
>>>>  	}
>>>>  
>>>>  	/* FIXME: add subpixel order */
>>>>  
>>>> -	drm_atomic_helper_wait_for_vblanks(dev, state);
>>>> +	if (!state->legacy_cursor_update)
>>>> +		intel_atomic_wait_for_vblanks(dev, dev_priv,
>>>> crtc_vblank_mask);
>>>>  
>>>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>>> +		intel_post_plane_update(to_intel_crtc(crtc));
>>>> +
>>>>  		if (put_domains[i])
>>>>  			modeset_put_power_domains(dev_priv,
>>>> put_domains[i]);
>>>>  	}
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 335e6b24b0bc..e911c86f873b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>>>>  	bool update_pipe; /* can a fast modeset be performed? */
>>>>  	bool disable_cxsr;
>>>>  	bool wm_changed; /* watermarks are updated */
>>>> +	bool fb_changed; /* fb on any of the planes is changed
>>>> */
>>>>  
>>>>  	/* Pipe source size (ie. panel fitter input size)
>>>>  	 * All planes will be positioned inside this space,
>>>> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>>>>  
>>>>  	/* Sleepable operations to perform after commit */
>>>>  	unsigned fb_bits;
>>>> -	bool wait_vblank;
>>> One of the things that I like about the code without this patch is
>>> that
>>> it's very explicit on when we need to wait for vblanks (except for
>>> the
>>> problem where we wait twice). The new code is not as easy to
>>> read/understand as the old one. This comment is similar to the one
>>> I
>>> made in patch 6: I'm not sure if sacrificing readability is worth
>>> it.
>>>
>>> I wonder that maybe the cleanest fix to the "we're waiting 2
>>> vblanks"
>>> problem is to just remove the call to
>>> drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
>>> Then we'd have a second patch introducing
>>> intel_atomic_wait_for_vblanks() for the "wait for all vblanks at
>>> the
>>> same time" optimization, and then a last commit possibly replacing
>>> commit->wait_vblank for state->fb_changed. Just an idea, not a
>>> request.
>> There were cases in which the atomic helper would wait for vblanks
>> which
>> wouldn't trigger any of the other changes, so it can't be blindly
>> removed. Mostly when
>> updating a plane with a same sized fb.
> Those could be specifically addressed on their own patches, then :)
It would break things though.

I think what might make the most sense is adding a inline function for needs_vblank,
with comments why certain flags require a vblank wait.
>> Wait for vblank is required for unpinning, it would be bad if the
>> current fb is unpinned.
>>
>>> I'll wait for your answers before reaching any conclusions of what
>>> I
>>> think should be done, since I may be misunderstanding something.
>> I want to call all post flip work scheduled later on so it happens
>> after the next vblank.
>> That will remove all confusion on when a vblank is required, because
>> all post_plane_update
>> and unpin will always wait until next vblank.
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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