Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 26, 2015 at 09:31:48AM +0100, Maarten Lankhorst wrote:
> Op 22-10-15 om 17:15 schreef Ville Syrjälä:
> > On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote:
> >> Op 22-10-15 om 15:30 schreef Ville Syrjälä:
> >>> On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote:
> >>>> By handling this after the atomic helper waits for vblanks there will
> >>>> be one less wait for vblank in the atomic path.
> >>>>
> >>>> Also get rid of the double wait_for_vblank on broadwell, looks like
> >>>> it's a bug doing the vblank wait twice.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_display.c | 28 +++-------------------------
> >>>>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
> >>>>  2 files changed, 3 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index 051a1e2b1c55..b8e1a5471bed 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
> >>>>  	int pipe = intel_crtc->pipe;
> >>>>  
> >>>>  	/*
> >>>> -	 * 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 (IS_BROADWELL(dev))
> >>>> -		intel_wait_for_vblank(dev, pipe);
> >>> The right fix for this crap is to not use the flip done interrupt. But
> >>> apparently no one wants to fix that.
> >> This is fixed later on when converting page_flip to atomic. I've modified
> >> page_flip_finished to ignore flips when visible_changed happens on broadwell.
> > Sounds like another hack.
> It's not a hack when it's a workaround for buggy hw. ;-)
> >
> >>> So now I'm worried that we now depend on the atomic helper doing
> >>> synchornous vblank waits here. Are we expecting those vblank waits to
> >>> remain there? I would have expected to get rid of all synchronois vblank
> >>> waits in plane codepaths (apart from ones for hw workarounds).
> >> In my unify page flip and atomic series I first add all post_plane_update and
> >> unpinning to unpin_work_fn.
> >>
> >> After that I have a patch to convert the last part of atomic after wait_for_vblanks
> >> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async.
> >> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip,
> >> so it will finish before next one starts.
> >>
> >>> Also does the helper actually wait for vblanks when the plane gets
> >>> enabled w/o the framebuffer actually changing?
> >> afaict it does, as long as the crtc is enabled.a
> > Where is that code? All I see is a old_fb != new_fb check.
> Oh indeed, I missed that one. That would make it unreliable to use though. :-/
> 
> I think drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active and maybe take a force flag to force a vblank wait..
> 
> Daniel would you be ok with such a patch?

The helper vblank wait is just to avoid tearing, which can't happen if the
fb doesn't change.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx