Re: [PATCH v2 5/5] drm/tegra: Implement page-flipping support

[Mario Kleiner <mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 02/11/2013 10:00 AM, Thierry Reding wrote:
> On Tue, Jan 22, 2013 at 06:27:24PM +0100, Mario Kleiner wrote:
>> On 22.01.13 09:31, Terje Bergström wrote:
>>> On 14.01.2013 18:06, Thierry Reding wrote:
>>>> +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>>>> +			      struct drm_pending_vblank_event *event)
>>>> +{
>>>> +	struct tegra_framebuffer *newfb = to_tegra_fb(fb);
>>>> +	struct tegra_dc *dc = to_tegra_dc(crtc);
>>>> +	struct drm_device *drm = crtc->dev;
>>>> +	unsigned long flags;
>>>> +
>>>> +	if (dc->event)
>>>> +		return -EBUSY;
>> Hi
>>
>> I haven't read the Tegra programming manual or played with this, so
>> maybe i'm wrong for some reason, but i think there is a race here
>> between actual pageflip completion - latching newfb into the scanout
>> base register and the completion routine that gets called from the
>> vblank irq handler.
>>
>> If this code gets called close to vblank or inside vblank, couldn't
>> it happen that tegra_dc_set_base() programs a pageflip that gets
>> executed immediately - the new fb gets latched into the crtc, but
>> the corresponding vblank irq handler for the vblank of flip
>> completion runs before the "if (event)" block can set dc->event =
>> event;? Or vblank irq's are off because drm_vblank_get() is only
>> called at the end of the routine? In both cases the completion
>> routine would miss the correct vblank and only timestamp and emit
>> the completion event 1 vblank after actual flip completion.
>>
>> In that case it would be better to place tegra_dc_set_base() after
>> the "if (event)" block and have some check inside the flip
>> completion routine to make sure the pageflip completion event is
>> only emitted if the scanout is really already latched with the
>> newfb.
> An easier solution might be to expand the scope of the lock a bit to
> encompass the call to tegra_dc_set_base() and extend until the end of
> tegra_dc_page_flip(). That should properly keep the page-flip completion
> from interfering, right?
>
> 	spin_lock_irqsave(&drm->event_lock, flags);
>
> 	tegra_dc_set_base(dc, 0, 0, newfb);
>
> 	if (event) {
> 		event->pipe = dc->pipe;
> 		dc->event = event;
> 		drm_vblank_get(drm, dc->pipe);
> 	}
>
> 	spin_unlock_irqrestore(&drm->event_lock, flags);
>
> Thierry
Yes, that would avoid races between page flip ioctl and the irq handler. 
But i think the tegra_dc_set_base() should go below the if (event) {} 
block, before the spin_unlock_irqrestore() so you can be sure that 
drm_vblank_get() has been called before tegra_dc_set_base() is executed. 
Otherwise vblank irq's may be off during the vblank in which the 
tegra_dc_set_base() takes effect and a flip complete would only get 
reported one scanout cycle later at the next vblank -> You'd signal a 
pageflip completion 1 frame too late.

You also still need to check in tegra_dc_finish_page_flip() if the new 
scanout buffer has really been latched before emitting the flip complete 
event. Otherwise it could happen that your execution of 
tegra_dc_page_flip() intersects with the vblank interval and manages to 
queue the event in time, so the finish_page_flip picks it up, but the 
register programming in tegra_dc_set_base() happened a bit too late, so 
it doesn't get latched in the same vblank but one vblank later --> You'd 
signal pageflip completion 1 frame too early.

I've just downloaded the Tegra-3 TRM and had a quick look. Section 
20.5.1 "Display shadow registers". As far as i understand, if dc->event 
is pending in tegra_dc_finish_page_flip(), you could read back the 
ACTIVE copy of DC_WINBUF_START_ADDR by setting READ_MUX properly in 
DC_CMD_STATE_ACCESS_0, and compare its value against the ARM copy in 
DC_WINBUF_A_START_ADDR_NS_0. If both values match, the pageflip has 
happened and the dc->event can be dequeued and emitted.

That assumes that the ARM copy is latched into the ACTIVE copy only at 
leading edge of vblank. Section 20.5.1 says "...latching happens on the 
next frame boundary after (GENERAL/WIN_A/WIN_B/WIN_C)_ACT_REQ is 
programmed..." - not totally clear to me if this is the same as start of 
vblank?

-mario