| Message ID | 20190523200756.25314-4-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | omapdrm: DSI command mode panel support | expand |
Hi Sebastian, On 23/05/2019 23:07, Sebastian Reichel wrote: > @@ -302,6 +328,30 @@ void omap_crtc_vblank_irq(struct drm_crtc *crtc) > DBG("%s: apply done", omap_crtc->name); > } > > +void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus) > +{ > + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > + > + if (!omap_crtc->framedone_handler) { > + dev_warn(omap_crtc->base.dev->dev, "no framedone handler?"); > + return; > + } This triggers on normal displays. FRAMEDONE is an interrupt we get when DISPC's output videoport is being turned off. It's raised after the last frame has been finished (i.e. the DISPC is truly done with that videoport). We get it for both conventional displays (when the display is turned off) and for DSI command mode (when a single frame has been sent), as in both cases the videoport is disabled after the operation. For conventional displays, you can think FRAMEDONE as the last vsync. We also have special handling for FRAMEDONE in omap_crtc_set_enabled(), which is used to get the drm driver to wait for FRAMEDONE when disabling the display. I wonder if this separate framedone handling might somehow conflict with that code. And/or should these be somehow combined. Tomi
Hi Tomi, On Tue, May 28, 2019 at 01:19:01PM +0300, Tomi Valkeinen wrote: > Hi Sebastian, > > On 23/05/2019 23:07, Sebastian Reichel wrote: > > > @@ -302,6 +328,30 @@ void omap_crtc_vblank_irq(struct drm_crtc *crtc) > > DBG("%s: apply done", omap_crtc->name); > > } > > +void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus) > > +{ > > + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > > + > > + if (!omap_crtc->framedone_handler) { > > + dev_warn(omap_crtc->base.dev->dev, "no framedone handler?"); > > + return; > > + } > > This triggers on normal displays. > > FRAMEDONE is an interrupt we get when DISPC's output videoport is being > turned off. It's raised after the last frame has been finished (i.e. the > DISPC is truly done with that videoport). > > We get it for both conventional displays (when the display is turned off) > and for DSI command mode (when a single frame has been sent), as in both > cases the videoport is disabled after the operation. For conventional > displays, you can think FRAMEDONE as the last vsync. Ok, but it should only trigger when framedone irq is enabled. This commit adds the required infrastructure, but does not call omap_irq_enable_framedone() anywhere. The next commit enables it, but only for manually updated displays. > We also have special handling for FRAMEDONE in omap_crtc_set_enabled(), > which is used to get the drm driver to wait for FRAMEDONE when disabling the > display. I wonder if this separate framedone handling might somehow conflict > with that code. And/or should these be somehow combined. Oh sorry, I missed the part that omap_irq_wait_init() actually enables the framedone irq. It should be enough to just drop the warning (and the curly brackets) to keep existing behaviour. The code exits early with the above warning for any existing code (since that does not register a framedone handler). DSI on the other hand does not reach the omap_irq_wait_init() part. Regarding combining the logic: I don't think there is anything to combine right now. It should be possible to simplify the logic after DSI has been converted to drm_panel style, since this will move the update logic for the screen content from the panel driver to DSI core. TLDR: It's enough to remove the warning. Do you need a new submission for this? -- Sebastian
On 30/05/2019 00:55, Sebastian Reichel wrote: > Oh sorry, I missed the part that omap_irq_wait_init() actually > enables the framedone irq. It should be enough to just drop the > warning (and the curly brackets) to keep existing behaviour. The > code exits early with the above warning for any existing code (since > that does not register a framedone handler). DSI on the other hand > does not reach the omap_irq_wait_init() part. Regarding combining > the logic: I don't think there is anything to combine right now. > It should be possible to simplify the logic after DSI has been > converted to drm_panel style, since this will move the update logic > for the screen content from the panel driver to DSI core. > > TLDR: It's enough to remove the warning. Do you need a new > submission for this? Ok. No, I can edit the patch. Nikolaus, are you able to test DSI videomode with these patches? Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 86827a061b0b..68697820d189 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -51,6 +51,9 @@ struct omap_crtc { bool pending; wait_queue_head_t pending_wait; struct drm_pending_vblank_event *event; + + void (*framedone_handler)(void *); + void *framedone_handler_data; }; /* ----------------------------------------------------------------------------- @@ -230,6 +233,18 @@ static int omap_crtc_dss_register_framedone( struct omap_drm_private *priv, enum omap_channel channel, void (*handler)(void *), void *data) { + struct drm_crtc *crtc = priv->channels[channel]->crtc; + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + struct drm_device *dev = omap_crtc->base.dev; + + if (omap_crtc->framedone_handler) + return -EBUSY; + + dev_dbg(dev->dev, "register framedone %s", omap_crtc->name); + + omap_crtc->framedone_handler = handler; + omap_crtc->framedone_handler_data = data; + return 0; } @@ -237,6 +252,17 @@ static void omap_crtc_dss_unregister_framedone( struct omap_drm_private *priv, enum omap_channel channel, void (*handler)(void *), void *data) { + struct drm_crtc *crtc = priv->channels[channel]->crtc; + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + struct drm_device *dev = omap_crtc->base.dev; + + dev_dbg(dev->dev, "unregister framedone %s", omap_crtc->name); + + WARN_ON(omap_crtc->framedone_handler != handler); + WARN_ON(omap_crtc->framedone_handler_data != data); + + omap_crtc->framedone_handler = NULL; + omap_crtc->framedone_handler_data = NULL; } static const struct dss_mgr_ops mgr_ops = { @@ -302,6 +328,30 @@ void omap_crtc_vblank_irq(struct drm_crtc *crtc) DBG("%s: apply done", omap_crtc->name); } +void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus) +{ + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + + if (!omap_crtc->framedone_handler) { + dev_warn(omap_crtc->base.dev->dev, "no framedone handler?"); + return; + } + + omap_crtc->framedone_handler(omap_crtc->framedone_handler_data); + + spin_lock(&crtc->dev->event_lock); + /* Send the vblank event if one has been requested. */ + if (omap_crtc->event) { + drm_crtc_send_vblank_event(crtc, omap_crtc->event); + omap_crtc->event = NULL; + } + omap_crtc->pending = false; + spin_unlock(&crtc->dev->event_lock); + + /* Wake up omap_atomic_complete. */ + wake_up(&omap_crtc->pending_wait); +} + static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc) { struct omap_drm_private *priv = crtc->dev->dev_private; diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.h b/drivers/gpu/drm/omapdrm/omap_crtc.h index d9de437ba9dd..d33bbb7a4f90 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.h +++ b/drivers/gpu/drm/omapdrm/omap_crtc.h @@ -41,5 +41,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, int omap_crtc_wait_pending(struct drm_crtc *crtc); void omap_crtc_error_irq(struct drm_crtc *crtc, u32 irqstatus); void omap_crtc_vblank_irq(struct drm_crtc *crtc); +void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus); #endif /* __OMAPDRM_CRTC_H__ */ diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c index 329ad26d6d50..01dda84ca2ee 100644 --- a/drivers/gpu/drm/omapdrm/omap_irq.c +++ b/drivers/gpu/drm/omapdrm/omap_irq.c @@ -85,6 +85,28 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait, return ret == 0 ? -1 : 0; } +int omap_irq_enable_framedone(struct drm_crtc *crtc, bool enable) +{ + struct drm_device *dev = crtc->dev; + struct omap_drm_private *priv = dev->dev_private; + unsigned long flags; + enum omap_channel channel = omap_crtc_channel(crtc); + int framedone_irq = + priv->dispc_ops->mgr_get_framedone_irq(priv->dispc, channel); + + DBG("dev=%p, crtc=%u, enable=%d", dev, channel, enable); + + spin_lock_irqsave(&priv->wait_lock, flags); + if (enable) + priv->irq_mask |= framedone_irq; + else + priv->irq_mask &= ~framedone_irq; + omap_irq_update(dev); + spin_unlock_irqrestore(&priv->wait_lock, flags); + + return 0; +} + /** * enable_vblank - enable vblank interrupt events * @dev: DRM device @@ -217,6 +239,9 @@ static irqreturn_t omap_irq_handler(int irq, void *arg) if (irqstatus & priv->dispc_ops->mgr_get_sync_lost_irq(priv->dispc, channel)) omap_crtc_error_irq(crtc, irqstatus); + + if (irqstatus & priv->dispc_ops->mgr_get_framedone_irq(priv->dispc, channel)) + omap_crtc_framedone_irq(crtc, irqstatus); } omap_irq_ocp_error_handler(dev, irqstatus); diff --git a/drivers/gpu/drm/omapdrm/omap_irq.h b/drivers/gpu/drm/omapdrm/omap_irq.h index 9d5441468eca..02abb4ed9813 100644 --- a/drivers/gpu/drm/omapdrm/omap_irq.h +++ b/drivers/gpu/drm/omapdrm/omap_irq.h @@ -27,6 +27,7 @@ struct drm_device; struct omap_irq_wait; int omap_irq_enable_vblank(struct drm_crtc *crtc); +int omap_irq_enable_framedone(struct drm_crtc *crtc, bool enable); void omap_irq_disable_vblank(struct drm_crtc *crtc); void omap_drm_irq_uninstall(struct drm_device *dev); int omap_drm_irq_install(struct drm_device *dev);