From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Kleiner Subject: Re: [PATCH v4 6/9] drm/tegra: Implement page-flipping support Date: Thu, 28 Feb 2013 02:09:27 +0100 Message-ID: <512EAE47.1010909@tuebingen.mpg.de> References: <1361457361-13838-1-git-send-email-thierry.reding@avionic-design.de> <1361457361-13838-7-git-send-email-thierry.reding@avionic-design.de> <5127993B.3020100@gmail.com> <20130225070657.GB18053@avionic-0098.mockup.avionic-design.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mxdfn.gwdg.de (mailer-b2.gwdg.de [134.76.10.16]) by gabe.freedesktop.org (Postfix) with ESMTP id 62A59E5C62 for ; Wed, 27 Feb 2013 17:09:32 -0800 (PST) In-Reply-To: <20130225070657.GB18053@avionic-0098.mockup.avionic-design.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Thierry Reding Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 >>> --- >>> 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