[Intel-gfx] [PATCH v2 2/9] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v3.

[Maarten Lankhorst]

Op 09-02-16 om 11:42 schreef Ville Syrjälä:
> On Tue, Feb 09, 2016 at 11:27:44AM +0100, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 13-01-16 om 13:58 schreef Ville Syrjälä:
>>> On Mon, Jan 11, 2016 at 01:27:42PM +0100, Maarten Lankhorst wrote:
>>>> 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.
>>> Still NAK, because you just removed the comment entirely.
>> I may have removed the comment but there will always be an unconditional wait because of the wm changes.
>> In some future commit I will rework do_intel_finish_page_flip to deal with atomic, and in that function the comment
>> will be useful again.
> The comment is the spec here, so it should be kept. Actually what I
> really want is to stop using the flip done interrupt entirely since
> it's arguably broken, at which point the comment should problably be
> moved to somewhere else (interrupt setup code, flip completion code,
> etc.). But removing the comment would be bad since then people might
> not understand why we do it the way we do.
I think the flip done interrupt will still be needed for cs flips, but Chris was against removing it iirc. I'll move the comment to page_flip_finished for now.
>>>> 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.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>>>>  drivers/gpu/drm/i915/intel_display.c | 80 ++++++++++++++++++++++++++----------
>>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>>>  3 files changed, 61 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 9682d94af710..ba9a57f33c43 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>>>>  	crtc_state->disable_cxsr = false;
>>>>  	crtc_state->wm_changed = false;
>>>>  	crtc_state->wm.need_postvbl_update = 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 2aa1c5367625..eac73f005a70 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4796,9 +4796,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;
>>>> @@ -11872,6 +11869,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;
>>> Isn't that going to slow down cursor updates once again?
>> Very likely.. Shall I add a if (!state->legacy_cursor_update) to intel_atomic_wait_for_vblanks ?
> Not sure. Still wishing we could have just had the proper fps>=vrefresh
> support fron the start.
>
Working on it!