Re: [PATCH v4 6/9] drm/tegra: Implement page-flipping support

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4605 bytes --]

On Fri, Feb 22, 2013 at 05:13:47PM +0100, Mario Kleiner wrote:
> On 02/21/2013 03:35 PM, Thierry Reding wrote:
> >All the necessary support bits like .mode_set_base() and VBLANK are now
> >available, so page-flipping case easily be implemented on top.
> >
> >Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >---
> >Changes in v3:
> >- use drm_send_vblank_event()
> >- set crtc->fb field
> >
> >Changes in v4:
> >- fix a potential race by checking that a framebuffer base has been
> >   latched when completing a page-flip
> >
> >  drivers/gpu/drm/tegra/dc.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/tegra/dc.h  |  2 ++
> >  drivers/gpu/drm/tegra/drm.c |  9 +++++++
> >  drivers/gpu/drm/tegra/drm.h |  5 ++++
> >  4 files changed, 82 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >index 5f55a25..c5d825f 100644
> >--- a/drivers/gpu/drm/tegra/dc.c
> >+++ b/drivers/gpu/drm/tegra/dc.c
> >@@ -183,7 +183,72 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc)
> >  	spin_unlock_irqrestore(&dc->lock, flags);
> >  }
> >+static void tegra_dc_finish_page_flip(struct tegra_dc *dc)
> >+{
> >+	struct drm_device *drm = dc->base.dev;
> >+	struct drm_crtc *crtc = &dc->base;
> >+	struct drm_gem_cma_object *gem;
> >+	unsigned long flags, base;
> >+
> 
> The checks for properly latched scanout look good to me and should
> fix the race between software and hardware. But shouldn't you
> already spin_lock_irqsave() here, before checking dc->event and
> performing write access on the DC_CMD_STATE_ACCESS registers? And
> lock around most of the tegra_dc_page_flip() code below, like in
> your previous iteration of the patch, to prevent a race between
> pageflip ioctl and the vblank irq handler?

Currently there's nothing else that touches the DC_CMD_STATE_ACCESS
register, so that part should be safe.

As to the locking that I removed since the previous patch, I looked at
it more closely and didn't think it was necessary. Also nothing in my
tests seemed to point to any race here, though I probably didn't cover
everything.

> >+	if (!dc->event)
> 
> -> !dc->event may exit prematurely because the irq handler on one
> core doesn't notice the new setup of dc->event by
> tegra_dc_page_flip() on a different core? Don't know if that can
> happen, don't know the memory ordering of arm, but could imagine
> that this or the compiler reordering instructions could cause
> trouble without lock protection.

I'm not sure I follow here. If the handler sees a NULL here, why would
it want to continue? It can safely assume that the page flip wasn't
setup for this interval, can't it?

If indeed the VBLANK interrupt happens exactly around the assignment to
dc->event, then we'll just schedule the page-flip for the next VBLANK.

> >+		return;
> >+
> >+	gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
> -> crtc->fb gets updated in tegra_dc_page_flip() after
> tegra_dc_set_base() without lock -> possibly stale fb here?

Good point. That may indeed require a lock. I'll need to look more
closely.

> >+	/* check if new start address has been latched */
> >+	tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
> >+	base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
> >+	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> >+
> 
> -> Can other code in the driver write to DC_CMD_STATE_ACCESS,
> simultaneously to tegra_dc_page_flip writing->reading->writing and
> cause trouble?

This is the only location where the DC_CMD_STATE_ACCESS register is
actually used in the driver so we should be safe for now.

> >+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >+			      struct drm_pending_vblank_event *event)
> >+{
> >+	struct tegra_dc *dc = to_tegra_dc(crtc);
> >+	struct drm_device *drm = crtc->dev;
> >+
> >+	if (dc->event)
> >+		return -EBUSY;
> >+
> 
> -> Lock already here?
> 
> >+	if (event) {
> >+		event->pipe = dc->pipe;
> >+		dc->event = event;
> >+		drm_vblank_get(drm, dc->pipe);
> >+	}
> >+
> >+	tegra_dc_set_base(dc, 0, 0, fb);
> >+	crtc->fb = fb;
> >+
> 
> -> Unlock here?

I tried to expand the lock after we discussed this previously but it
caused the code to deadlock. While in the process of tracking it down I
decided that the lock wasn't actually required at all.

But I hadn't thought about the stale FB issue, so I'll check again. I
notice that Dave has already merged this series, so if nobody objects
I'll fix it up in a follow-up patch.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel