From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH 6/6] drm/omap: protect omap_crtc's event with event_lock spinlock Date: Wed, 16 Apr 2014 12:36:46 +0530 Message-ID: <534E2C06.7020303@ti.com> References: <1397201015-2807-1-git-send-email-archit@ti.com> <1397201015-2807-7-git-send-email-archit@ti.com> <534BD63C.5050708@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from devils.ext.ti.com (devils.ext.ti.com [198.47.26.153]) by gabe.freedesktop.org (Postfix) with ESMTP id 12E026E9FA for ; Wed, 16 Apr 2014 00:07:43 -0700 (PDT) In-Reply-To: <534BD63C.5050708@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Tomi Valkeinen Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Hi, On Monday 14 April 2014 06:06 PM, Tomi Valkeinen wrote: > On 11/04/14 10:23, Archit Taneja wrote: >> The vblank_cb callback and the page_flip ioctl can occur together in different >> CPU contexts. vblank_cb uses takes tje drm device's event_lock spinlock when >> sending the vblank event and updating omap_crtc->event and omap_crtc->od_fb. >> >> Use the same spinlock in page_flip, to make sure the above omap_crtc parameters >> are configured sequentially. >> >> Signed-off-by: Archit Taneja >> --- >> drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c >> index d74c72c..9c7af42 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c >> @@ -350,6 +350,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, >> struct drm_device *dev = crtc->dev; >> struct omap_crtc *omap_crtc = to_omap_crtc(crtc); >> struct drm_gem_object *bo; >> + unsigned long flags; >> >> DBG("%d -> %d (event=%p)", crtc->fb ? crtc->fb->base.id : -1, >> fb->base.id, event); >> @@ -359,9 +360,12 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, >> return -EINVAL; >> } >> >> + spin_lock_irqsave(&dev->event_lock, flags); >> + >> omap_crtc->event = event; >> omap_crtc->old_fb = crtc->fb = fb; >> >> + spin_unlock_irqrestore(&dev->event_lock, flags); > > There's > > if (omap_crtc->old_fb) > > just above the code you changed. Shouldn't that also be inside the spinlock? Yes, that should come within the spinlock too. On a related note, the mdp5_crtc->event in mdp5_crtc_page_flip(and for mdp4 crtc) should also be inside the spinlock. > > Also, the description contains a few typos. I'll fix these. Thanks, Archit