From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event() Date: Wed, 6 Aug 2014 15:23:01 +0200 Message-ID: <20140806132301.GK8727@phenom.ffwll.local> References: <1407325803-6944-1-git-send-email-ville.syrjala@linux.intel.com> <1407325803-6944-10-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1407325803-6944-10-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Wed, Aug 06, 2014 at 02:49:52PM +0300, ville.syrjala@linux.intel.com wro= te: > From: Ville Syrj=E4l=E4 > = > Currently it's possible that the following will happen: > 1. drm_wait_vblank() calls drm_vblank_get() > 2. drm_vblank_off() gets called > 3. drm_wait_vblank() calls drm_queue_vblank_event() which > adds the event to the queue event though vblank interrupts > are currently disabled (and may not be re-enabled ever again). > = > To fix the problem, add another vblank->enabled check into > drm_queue_vblank_event(). > = > drm_vblank_off() holds event_lock around the vblank disable, > so no further locking needs to be added to drm_queue_vblank_event(). > vblank disable from another source is not possible since > drm_wait_vblank() already holds a vblank reference. > = > Reviewed-by: Matt Roper > Signed-off-by: Ville Syrj=E4l=E4 I guess the window is too small here to make this reproducible in a test? Especially since each attempt will take a few hundred ms ... -Daniel > --- > drivers/gpu/drm/drm_irq.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > = > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 9353609..b2428cb 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1270,6 +1270,7 @@ static int drm_queue_vblank_event(struct drm_device= *dev, int pipe, > union drm_wait_vblank *vblwait, > struct drm_file *file_priv) > { > + struct drm_vblank_crtc *vblank =3D &dev->vblank[pipe]; > struct drm_pending_vblank_event *e; > struct timeval now; > unsigned long flags; > @@ -1293,6 +1294,18 @@ static int drm_queue_vblank_event(struct drm_devic= e *dev, int pipe, > = > spin_lock_irqsave(&dev->event_lock, flags); > = > + /* > + * drm_vblank_off() might have been called after we called > + * drm_vblank_get(). drm_vblank_off() holds event_lock > + * around the vblank disable, so no need for further locking. > + * The reference from drm_vblank_get() protects against > + * vblank disable from another source. > + */ > + if (!vblank->enabled) { > + ret =3D -EINVAL; > + goto err_unlock; > + } > + > if (file_priv->event_space < sizeof e->event) { > ret =3D -EBUSY; > goto err_unlock; > -- = > 1.8.5.5 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch