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

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

On Tue, Feb 09, 2016 at 12:07:01PM +0100, Maarten Lankhorst wrote:
> 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.

We don't really need it. Using the vblank interrupt should work just fine.

> >>>> 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@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!

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