| Message ID | 1452548477-15905-21-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/01/16 23:41, Daniel Vetter wrote: > Again since the drm core takes care of event unlinking/disarming this > is now just needless code. > > v2: Fixup misplaced hunks. > > Cc: Rob Clark <robdclark@gmail.com> > Acked-by: Daniel Stone <daniels@collabora.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1) > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 20 -------------------- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 8 -------- > drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 - > 3 files changed, 29 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 7d07733bdc86..4802da8e6d6f 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -662,26 +662,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) > return IRQ_HANDLED; > } > > -void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file) > -{ > - struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > - struct drm_pending_vblank_event *event; > - struct drm_device *dev = crtc->dev; > - unsigned long flags; > - > - /* Destroy the pending vertical blanking event associated with the > - * pending page flip, if any, and disable vertical blanking interrupts. > - */ > - spin_lock_irqsave(&dev->event_lock, flags); > - event = tilcdc_crtc->event; > - if (event && event->base.file_priv == file) { > - tilcdc_crtc->event = NULL; > - event->base.destroy(&event->base); > - drm_vblank_put(dev, 0); > - } > - spin_unlock_irqrestore(&dev->event_lock, flags); > -} > - Hmm, looks fine, but when I was comparing the omapdrm change and this one, I see tilcdc doing drm_vblank_put() in the removed code but omapdrm doesn't. The other patches that nuke preclose hooks also contain vblank_put. Will there be a vblank_put call missing here, or will there be an extra vblank_put call happening somewhere on omapdrm? Tomi
On Tue, Jan 12, 2016 at 04:19:39PM +0200, Tomi Valkeinen wrote: > > On 11/01/16 23:41, Daniel Vetter wrote: > > Again since the drm core takes care of event unlinking/disarming this > > is now just needless code. > > > > v2: Fixup misplaced hunks. > > > > Cc: Rob Clark <robdclark@gmail.com> > > Acked-by: Daniel Stone <daniels@collabora.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1) > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 20 -------------------- > > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 8 -------- > > drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 - > > 3 files changed, 29 deletions(-) > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > index 7d07733bdc86..4802da8e6d6f 100644 > > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > @@ -662,26 +662,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) > > return IRQ_HANDLED; > > } > > > > -void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file) > > -{ > > - struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > > - struct drm_pending_vblank_event *event; > > - struct drm_device *dev = crtc->dev; > > - unsigned long flags; > > - > > - /* Destroy the pending vertical blanking event associated with the > > - * pending page flip, if any, and disable vertical blanking interrupts. > > - */ > > - spin_lock_irqsave(&dev->event_lock, flags); > > - event = tilcdc_crtc->event; > > - if (event && event->base.file_priv == file) { > > - tilcdc_crtc->event = NULL; > > - event->base.destroy(&event->base); > > - drm_vblank_put(dev, 0); > > - } > > - spin_unlock_irqrestore(&dev->event_lock, flags); > > -} > > - > > Hmm, looks fine, but when I was comparing the omapdrm change and this > one, I see tilcdc doing drm_vblank_put() in the removed code but omapdrm > doesn't. > > The other patches that nuke preclose hooks also contain vblank_put. Will > there be a vblank_put call missing here, or will there be an extra > vblank_put call happening somewhere on omapdrm? Different approaches to the same problem: - omap just unlinks the event from fpriv and still process it normally. But then before sending it out it checks whether the fpriv is still there or not and either sends it, or deletes the event directly. This way the vblank_put is always called from the worker/irq handler as part of the event processing. This is the same approach I implemented in core with this series. - tilcdc (and most other drivers) entirely destroy the event in the preclose hook, which means they must also release any other resources acquired as part of that event. Therefore they have the vblank_put here. But the vblank_put is obviously also in the normal event processing paths, so with the new approach of only unlinking it we can handle this without any special cases in the driver. I hope this explains what's going on. Since you're about driver maintainer no. 3 with the same question: Can you pls review the kerneldoc and make sure it explains this well? I tried to improve it already a bit after Laurent's/Thomas' questions. -Daniel
On 12/01/16 17:12, Daniel Vetter wrote: > Different approaches to the same problem: > > - omap just unlinks the event from fpriv and still process it normally. > But then before sending it out it checks whether the fpriv is still > there or not and either sends it, or deletes the event directly. This > way the vblank_put is always called from the worker/irq handler as part > of the event processing. > > This is the same approach I implemented in core with this series. > > - tilcdc (and most other drivers) entirely destroy the event in the > preclose hook, which means they must also release any other resources > acquired as part of that event. Therefore they have the vblank_put here. > But the vblank_put is obviously also in the normal event processing > paths, so with the new approach of only unlinking it we can handle this > without any special cases in the driver. > > I hope this explains what's going on. Since you're about driver maintainer > no. 3 with the same question: Can you pls review the kerneldoc and make > sure it explains this well? I tried to improve it already a bit after > Laurent's/Thomas' questions. Ok, makes sense. I think the kernel doc is fine. I wasn't able to test tilcdc, as it seems to crash on drm-next at the moment... Need to debug that first. Tomi
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 7d07733bdc86..4802da8e6d6f 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -662,26 +662,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) return IRQ_HANDLED; } -void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file) -{ - struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); - struct drm_pending_vblank_event *event; - struct drm_device *dev = crtc->dev; - unsigned long flags; - - /* Destroy the pending vertical blanking event associated with the - * pending page flip, if any, and disable vertical blanking interrupts. - */ - spin_lock_irqsave(&dev->event_lock, flags); - event = tilcdc_crtc->event; - if (event && event->base.file_priv == file) { - tilcdc_crtc->event = NULL; - event->base.destroy(&event->base); - drm_vblank_put(dev, 0); - } - spin_unlock_irqrestore(&dev->event_lock, flags); -} - struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev) { struct tilcdc_crtc *tilcdc_crtc; diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 4ddb21e7f52f..b39ba213e303 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -349,13 +349,6 @@ fail_free_priv: return ret; } -static void tilcdc_preclose(struct drm_device *dev, struct drm_file *file) -{ - struct tilcdc_drm_private *priv = dev->dev_private; - - tilcdc_crtc_cancel_page_flip(priv->crtc, file); -} - static void tilcdc_lastclose(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; @@ -556,7 +549,6 @@ static struct drm_driver tilcdc_driver = { .driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET, .load = tilcdc_load, .unload = tilcdc_unload, - .preclose = tilcdc_preclose, .lastclose = tilcdc_lastclose, .set_busid = drm_platform_set_busid, .irq_handler = tilcdc_irq, diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index e863ad0d26fe..66105d8dc620 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -163,7 +163,6 @@ struct tilcdc_panel_info { #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev); -void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file); irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc); void tilcdc_crtc_update_clk(struct drm_crtc *crtc); void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,