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
