| Message ID | 1461702945-14185-8-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/04/16 23:35, Laurent Pinchart wrote: > As the FIFO underflow IRQ handler just prints an error message to the > kernel log, simplify the code by not registering one IRQ handler per > plane but print the messages directly from the main IRQ handler. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- > drivers/gpu/drm/omapdrm/omap_irq.c | 49 +++++++++++++++++++++++++++++++++--- > drivers/gpu/drm/omapdrm/omap_plane.c | 24 ------------------ > 3 files changed, 47 insertions(+), 28 deletions(-) > @@ -233,6 +271,11 @@ int omap_drm_irq_install(struct drm_device *dev) > > INIT_LIST_HEAD(&priv->irq_list); > > + priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW > + | DISPC_IRQ_VID1_FIFO_UNDERFLOW > + | DISPC_IRQ_VID2_FIFO_UNDERFLOW > + | DISPC_IRQ_VID3_FIFO_UNDERFLOW; Not all VID overlays exist on all DSS versions, so we shouldn't register irqs that don't exist on the HW. Also, I do like it that we deal with crtc or plane interrupts in omap_crtc or omap_plane. Would similar approach here work as you use in the following patches, i.e. just call underflow-handler func in omap_plane.c directly, instead of using the registration mechanism? Tomi
Hi Tomi, On Monday 09 May 2016 17:42:43 Tomi Valkeinen wrote: > On 26/04/16 23:35, Laurent Pinchart wrote: > > As the FIFO underflow IRQ handler just prints an error message to the > > kernel log, simplify the code by not registering one IRQ handler per > > plane but print the messages directly from the main IRQ handler. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- > > drivers/gpu/drm/omapdrm/omap_irq.c | 49 ++++++++++++++++++++++++++++--- > > drivers/gpu/drm/omapdrm/omap_plane.c | 24 ------------------ > > 3 files changed, 47 insertions(+), 28 deletions(-) > > > > @@ -233,6 +271,11 @@ int omap_drm_irq_install(struct drm_device *dev) > > > > INIT_LIST_HEAD(&priv->irq_list); > > > > + priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW > > + | DISPC_IRQ_VID1_FIFO_UNDERFLOW > > + | DISPC_IRQ_VID2_FIFO_UNDERFLOW > > + | DISPC_IRQ_VID3_FIFO_UNDERFLOW; > > Not all VID overlays exist on all DSS versions, so we shouldn't register > irqs that don't exist on the HW. Good point, I'll fix that. > Also, I do like it that we deal with crtc or plane interrupts in > omap_crtc or omap_plane. Would similar approach here work as you use in > the following patches, i.e. just call underflow-handler func in > omap_plane.c directly, instead of using the registration mechanism? I can do that, but given that all we do is just printing error messages, it sounds a bit overkill. I propose moving FIFO underflow IRQ handling to the CRTC and plane code later when/if we need to perform more work in the handlers.
On 06/06/16 02:21, Laurent Pinchart wrote: >> Also, I do like it that we deal with crtc or plane interrupts in >> omap_crtc or omap_plane. Would similar approach here work as you use in >> the following patches, i.e. just call underflow-handler func in >> omap_plane.c directly, instead of using the registration mechanism? > > I can do that, but given that all we do is just printing error messages, it > sounds a bit overkill. I propose moving FIFO underflow IRQ handling to the > CRTC and plane code later when/if we need to perform more work in the > handlers. What's the overkill? Isn't it just making the function public, and calling that from omap_irq? And it's true your patch just prints an error message, but omap_irq_fifo_underflow() is still 35 lines, so it's not a one-liner. I don't feel strongly about this, though =). Tomi
Hi Tomi, On Monday 06 Jun 2016 13:50:13 Tomi Valkeinen wrote: > On 06/06/16 02:21, Laurent Pinchart wrote: > >> Also, I do like it that we deal with crtc or plane interrupts in > >> omap_crtc or omap_plane. Would similar approach here work as you use in > >> the following patches, i.e. just call underflow-handler func in > >> omap_plane.c directly, instead of using the registration mechanism? > > > > I can do that, but given that all we do is just printing error messages, > > it sounds a bit overkill. I propose moving FIFO underflow IRQ handling to > > the CRTC and plane code later when/if we need to perform more work in the > > handlers. > > What's the overkill? Isn't it just making the function public, and > calling that from omap_irq? To turn the IRQ handler into a plane function, we would have to look up the plane corresponding to the overflow source, call the overflow IRQ handler with that plane as a parameter, and then convert the plane into an IRQ source name to be printed. I don't think that's worth it. > And it's true your patch just prints an error message, but > omap_irq_fifo_underflow() is still 35 lines, so it's not a one-liner. > > I don't feel strongly about this, though =).
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index ed15b2dbe47a..4ac629053ab1 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -103,7 +103,7 @@ struct omap_drm_private { /* irq handling: */ struct list_head irq_list; /* list of omap_drm_irq */ - uint32_t vblank_mask; /* irq bits set for userspace vblank */ + uint32_t irq_mask; /* enabled irqs in addition to irq_list */ struct omap_drm_irq error_handler; /* atomic commit */ diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c index 60e1e8016708..eb7133698d07 100644 --- a/drivers/gpu/drm/omapdrm/omap_irq.c +++ b/drivers/gpu/drm/omapdrm/omap_irq.c @@ -32,7 +32,7 @@ static void omap_irq_update(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private; struct omap_drm_irq *irq; - uint32_t irqmask = priv->vblank_mask; + uint32_t irqmask = priv->irq_mask; assert_spin_locked(&list_lock); @@ -153,7 +153,7 @@ int omap_irq_enable_vblank(struct drm_device *dev, unsigned int pipe) DBG("dev=%p, crtc=%u", dev, pipe); spin_lock_irqsave(&list_lock, flags); - priv->vblank_mask |= pipe2vbl(crtc); + priv->irq_mask |= pipe2vbl(crtc); omap_irq_update(dev); spin_unlock_irqrestore(&list_lock, flags); @@ -178,11 +178,47 @@ void omap_irq_disable_vblank(struct drm_device *dev, unsigned int pipe) DBG("dev=%p, crtc=%u", dev, pipe); spin_lock_irqsave(&list_lock, flags); - priv->vblank_mask &= ~pipe2vbl(crtc); + priv->irq_mask &= ~pipe2vbl(crtc); omap_irq_update(dev); spin_unlock_irqrestore(&list_lock, flags); } +static void omap_irq_fifo_underflow(uint32_t irqstatus) +{ + static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL, + DEFAULT_RATELIMIT_BURST); + static const struct { + const char *name; + u32 mask; + } sources[] = { + { "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW }, + { "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW }, + { "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW }, + { "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW }, + }; + + const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW + | DISPC_IRQ_VID1_FIFO_UNDERFLOW + | DISPC_IRQ_VID2_FIFO_UNDERFLOW + | DISPC_IRQ_VID3_FIFO_UNDERFLOW; + unsigned int i; + + if (!(irqstatus & mask)) + return; + + if (!__ratelimit(&_rs)) + return; + + drm_err("error: FIFO underflow on "); + + for (i = 0; i < ARRAY_SIZE(sources); ++i) { + if (sources[i].mask & irqstatus) + pr_cont("%s ", sources[i].name); + } + + pr_cont("(0x%08x)\n", irqstatus); +} + static irqreturn_t omap_irq_handler(int irq, void *arg) { struct drm_device *dev = (struct drm_device *) arg; @@ -205,6 +241,8 @@ static irqreturn_t omap_irq_handler(int irq, void *arg) drm_handle_vblank(dev, id); } + omap_irq_fifo_underflow(irqstatus); + spin_lock_irqsave(&list_lock, flags); list_for_each_entry_safe(handler, n, &priv->irq_list, node) { if (handler->irqmask & irqstatus) { @@ -233,6 +271,11 @@ int omap_drm_irq_install(struct drm_device *dev) INIT_LIST_HEAD(&priv->irq_list); + priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW + | DISPC_IRQ_VID1_FIFO_UNDERFLOW + | DISPC_IRQ_VID2_FIFO_UNDERFLOW + | DISPC_IRQ_VID3_FIFO_UNDERFLOW; + dispc_runtime_get(); dispc_clear_irqstatus(0xffffffff); dispc_runtime_put(); diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 93ee538a99f5..7b7d74072579 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -43,8 +43,6 @@ struct omap_plane { uint32_t nformats; uint32_t formats[32]; - - struct omap_drm_irq error_irq; }; struct omap_plane_state { @@ -200,8 +198,6 @@ static void omap_plane_destroy(struct drm_plane *plane) DBG("%s", omap_plane->name); - omap_irq_unregister(plane->dev, &omap_plane->error_irq); - drm_plane_cleanup(plane); kfree(omap_plane); @@ -320,14 +316,6 @@ static const struct drm_plane_funcs omap_plane_funcs = { .atomic_get_property = omap_plane_atomic_get_property, }; -static void omap_plane_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus) -{ - struct omap_plane *omap_plane = - container_of(irq, struct omap_plane, error_irq); - DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_plane->name, - irqstatus); -} - static const char *plane_names[] = { [OMAP_DSS_GFX] = "gfx", [OMAP_DSS_VIDEO1] = "vid1", @@ -335,13 +323,6 @@ static const char *plane_names[] = { [OMAP_DSS_VIDEO3] = "vid3", }; -static const uint32_t error_irqs[] = { - [OMAP_DSS_GFX] = DISPC_IRQ_GFX_FIFO_UNDERFLOW, - [OMAP_DSS_VIDEO1] = DISPC_IRQ_VID1_FIFO_UNDERFLOW, - [OMAP_DSS_VIDEO2] = DISPC_IRQ_VID2_FIFO_UNDERFLOW, - [OMAP_DSS_VIDEO3] = DISPC_IRQ_VID3_FIFO_UNDERFLOW, -}; - /* initialize plane */ struct drm_plane *omap_plane_init(struct drm_device *dev, int id, enum drm_plane_type type) @@ -365,10 +346,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, plane = &omap_plane->base; - omap_plane->error_irq.irqmask = error_irqs[id]; - omap_plane->error_irq.irq = omap_plane_error_irq; - omap_irq_register(dev, &omap_plane->error_irq); - ret = drm_universal_plane_init(dev, plane, (1 << priv->num_crtcs) - 1, &omap_plane_funcs, omap_plane->formats, omap_plane->nformats, type, NULL); @@ -382,7 +359,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, return plane; error: - omap_irq_unregister(plane->dev, &omap_plane->error_irq); kfree(omap_plane); return NULL; }
As the FIFO underflow IRQ handler just prints an error message to the kernel log, simplify the code by not registering one IRQ handler per plane but print the messages directly from the main IRQ handler. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- drivers/gpu/drm/omapdrm/omap_irq.c | 49 +++++++++++++++++++++++++++++++++--- drivers/gpu/drm/omapdrm/omap_plane.c | 24 ------------------ 3 files changed, 47 insertions(+), 28 deletions(-)