| Message ID | 1397201015-2807-7-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <archit@ti.com> > --- > 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? Also, the description contains a few typos. Tomi
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 <archit@ti.com> >> --- >> 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
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); /* * Hold a reference temporarily until the crtc is updated * and takes the reference to the bo. This avoids it
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 <archit@ti.com> --- drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)