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

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

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.

thanks,
-mario

>> +
>> +	tegra_dc_set_base(dc, 0, 0, newfb);
>> +
>> +	if (event) {
>> +		event->pipe = dc->pipe;
>> +
>> +		spin_lock_irqsave(&drm->event_lock, flags);
>> +		dc->event = event;
>> +		spin_unlock_irqrestore(&drm->event_lock, flags);
>> +
>> +		drm_vblank_get(drm, dc->pipe);
>> +	}
>> +
>> +	return 0;
>> +}
>