diff mbox

[v4,06/22] drm: omapdrm: Handle FIFO underflow IRQs internally

Message ID 1481675266-24598-7-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart Dec. 14, 2016, 12:27 a.m. UTC 
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>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
Changes since v3:

- Renamed error_irqs to omap_underflow_irqs

Changes since v1:

- Only register error IRQs that exist on the HW
---
 drivers/gpu/drm/omapdrm/omap_drv.c   |  4 +--
 drivers/gpu/drm/omapdrm/omap_drv.h   |  2 +-
 drivers/gpu/drm/omapdrm/omap_irq.c   | 67 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/omapdrm/omap_plane.c | 24 -------------
 4 files changed, 67 insertions(+), 30 deletions(-)

Comments

Tomi Valkeinen Dec. 14, 2016, 10:22 a.m. UTC | #1 
On 14/12/16 02:27, 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>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---

> +static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
> +				    u32 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;
> +
> +	spin_lock(&list_lock);
> +	irqstatus &= priv->irq_mask & mask;
> +	spin_unlock(&list_lock);
> +
> +	if (!irqstatus)
> +		return;

This is called every time we get any DSS interrupt, so I think it would
be good to have a fast-path here without the lock: irqstatus & mask.

Or maybe store the enabled underflow irq bits separately from irq_mask,
as the underflow bits are never changed after the initial setup, and
then there's no need for locking.

 Tomi
Laurent Pinchart Dec. 14, 2016, 11:48 a.m. UTC | #2 
Hi Tomi,

On Wednesday 14 Dec 2016 12:22:29 Tomi Valkeinen wrote:
> On 14/12/16 02:27, 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>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> > 
> > +static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
> > +				    u32 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;
> > +
> > +	spin_lock(&list_lock);
> > +	irqstatus &= priv->irq_mask & mask;
> > +	spin_unlock(&list_lock);
> > +
> > +	if (!irqstatus)
> > +		return;
> 
> This is called every time we get any DSS interrupt, so I think it would
> be good to have a fast-path here without the lock: irqstatus & mask.
>
> Or maybe store the enabled underflow irq bits separately from irq_mask,
> as the underflow bits are never changed after the initial setup, and
> then there's no need for locking.

I'd prefer going for the former, but I'm a bit concerned that an IRQ bit 
defined as FIFO overflow on one platform could be defined as something else on 
another platform and be mistaken.

Given that we already take the same lock in the IRQ handler to call the wait 
handlers, do you think this is really an issue ?
Tomi Valkeinen Dec. 14, 2016, 1:13 p.m. UTC | #3 
On 14/12/16 13:48, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wednesday 14 Dec 2016 12:22:29 Tomi Valkeinen wrote:
>> On 14/12/16 02:27, 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>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>
>>> +static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>>> +				    u32 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;
>>> +
>>> +	spin_lock(&list_lock);
>>> +	irqstatus &= priv->irq_mask & mask;
>>> +	spin_unlock(&list_lock);
>>> +
>>> +	if (!irqstatus)
>>> +		return;
>>
>> This is called every time we get any DSS interrupt, so I think it would
>> be good to have a fast-path here without the lock: irqstatus & mask.
>>
>> Or maybe store the enabled underflow irq bits separately from irq_mask,
>> as the underflow bits are never changed after the initial setup, and
>> then there's no need for locking.
> 
> I'd prefer going for the former, but I'm a bit concerned that an IRQ bit 
> defined as FIFO overflow on one platform could be defined as something else on 
> another platform and be mistaken.

I meant that you could first check irqstatus & mask, and exit if 0.
After that you could do the above spinlock, and the check against
priv->irq_mask & mask.

So yes, it's not perfect either, and in the worst case could mean still
doing the spinlock at every irq... But, if I'm not mistaken, the current
HW versions do not have overlapping irq bits.

> Given that we already take the same lock in the IRQ handler to call the wait 
> handlers, do you think this is really an issue ?

I don't think it's really an issue. But it's nice to optimize irq
handlers, especially as this is a hard-irq.

 Tomi
Tomi Valkeinen Dec. 15, 2016, 9:02 a.m. UTC | #4 
On 14/12/16 13:48, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wednesday 14 Dec 2016 12:22:29 Tomi Valkeinen wrote:
>> On 14/12/16 02:27, 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>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>
>>> +static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>>> +				    u32 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;
>>> +
>>> +	spin_lock(&list_lock);
>>> +	irqstatus &= priv->irq_mask & mask;
>>> +	spin_unlock(&list_lock);
>>> +
>>> +	if (!irqstatus)
>>> +		return;
>>
>> This is called every time we get any DSS interrupt, so I think it would
>> be good to have a fast-path here without the lock: irqstatus & mask.
>>
>> Or maybe store the enabled underflow irq bits separately from irq_mask,
>> as the underflow bits are never changed after the initial setup, and
>> then there's no need for locking.
> 
> I'd prefer going for the former, but I'm a bit concerned that an IRQ bit 
> defined as FIFO overflow on one platform could be defined as something else on 
> another platform and be mistaken.
> 
> Given that we already take the same lock in the IRQ handler to call the wait 
> handlers, do you think this is really an issue ?

Yep, I think it's fine for the time being.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi
diff mbox

Patch

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index fdc83cbcde61..6faba13c8e41 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -315,8 +315,6 @@  static int omap_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_init(dev);
 
-	omap_drm_irq_install(dev);
-
 	ret = omap_modeset_init_properties(dev);
 	if (ret < 0)
 		return ret;
@@ -489,6 +487,8 @@  static int omap_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_reset(dev);
 
+	omap_drm_irq_install(dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 919c14d3503c..a3594fa10ef3 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -102,7 +102,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..57a2de7e0d7b 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,52 @@  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(struct omap_drm_private *priv,
+				    u32 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;
+
+	spin_lock(&list_lock);
+	irqstatus &= priv->irq_mask & mask;
+	spin_unlock(&list_lock);
+
+	if (!irqstatus)
+		return;
+
+	if (!__ratelimit(&_rs))
+		return;
+
+	DRM_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 +246,8 @@  static irqreturn_t omap_irq_handler(int irq, void *arg)
 			drm_handle_vblank(dev, id);
 	}
 
+	omap_irq_fifo_underflow(priv, irqstatus);
+
 	spin_lock_irqsave(&list_lock, flags);
 	list_for_each_entry_safe(handler, n, &priv->irq_list, node) {
 		if (handler->irqmask & irqstatus) {
@@ -218,6 +261,13 @@  static irqreturn_t omap_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static const u32 omap_underflow_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,
+};
+
 /*
  * We need a special version, instead of just using drm_irq_install(),
  * because we need to register the irq via omapdss.  Once omapdss and
@@ -229,10 +279,21 @@  int omap_drm_irq_install(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_drm_irq *error_handler = &priv->error_handler;
+	unsigned int max_planes;
+	unsigned int i;
 	int ret;
 
 	INIT_LIST_HEAD(&priv->irq_list);
 
+	priv->irq_mask = 0;
+
+	max_planes = min(ARRAY_SIZE(priv->planes),
+			 ARRAY_SIZE(omap_underflow_irqs));
+	for (i = 0; i < max_planes; ++i) {
+		if (priv->planes[i])
+			priv->irq_mask |= omap_underflow_irqs[i];
+	}
+
 	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 82b2c23d6769..386d90af70f7 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 {
@@ -204,8 +202,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);
@@ -332,14 +328,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",
@@ -347,13 +335,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,
@@ -377,10 +358,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, possible_crtcs,
 				       &omap_plane_funcs, omap_plane->formats,
 				       omap_plane->nformats, type, NULL);
@@ -394,7 +371,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;
 }