On 02/25/2013 08:06 AM, Thierry Reding wrote:
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@xxxxxxxxxxxxxxxxx>
---
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.
ok.
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.
I meant the other way round. dc->event is initially NULL. Then pageflip
ioctl is called, close to vblank, the code e.g., executing on core 1. if
(event) branch in tegra_dc_page_flip() executes, assigns dc->event =
event etc. calls tegra_dc_set_base() before onset of vblank, pageflip
completes in vblank, but the irq handler and thereby
tegra_dc_finish_page_flip() running on a different core doesn't notice
dc->event is non-NULL, because the changes haven't propagated to the
different core without some memory write barrier etc., thereby doesn't
run the flip completion in the vblank of actual flip completion. But i
now think my point is probably pointless, because the successive calls
to drm_vblank_get() and tegra_dc_set_base() contain enough locking and
implicit memory barriers that such a stale value there won't happen.
+ 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.
Yep, i think here my argument from above may hold for crtc->fb, nothing
here to prevent a race afaics.
+ /* 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.
Ok. I'm just paranoid about forgetting such things on later changes.
+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.
Fine with me.
thanks,
-mario
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
