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

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

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?

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