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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130211/92bb6f44/attachment.pgp>