On Tue, Mar 25, 2014 at 7:34 AM, Thierry Reding thierry.reding@gmail.com wrote:
On Mon, Mar 24, 2014 at 05:42:28PM +0900, Alexandre Courbot wrote: [...]
diff --git a/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
[...]
+#include <subdev/ibus.h>
+struct nvea_ibus_priv {
struct nouveau_ibus base;+};
+static void +nvea_ibus_init_priv_ring(struct nvea_ibus_priv *priv) +{
nv_mask(priv, 0x137250, 0x3f, 0);nv_mask(priv, 0x000200, 0x20, 0);udelay(20);usleep_range()?
Sure.
+static void +nvea_ibus_intr(struct nouveau_subdev *subdev) +{
[...]
/* Acknowledge interrupt */nv_mask(priv, 0x12004c, 0x2, 0x2);while (--retry >= 0) {command = nv_rd32(priv, 0x12004c) & 0x3f;if (command == 0)break;}if (retry < 0)nv_warn(priv, "timeout waiting for ringmaster ack\n");+}
Perhaps I'm being paranoid, but this loop now depends on the frequency of the various clocks involved and therefore might break at some point if the frequencies get sufficiently high.
So a slightly safer implementation would use a proper timeout using a combination of msecs_to_jiffies(), time_before() and usleep_range(), like so:
timeout = jiffies + msecs_to_jiffies(...); while (time_before(jiffies, timeout)) { command = nv_rd32(...) & 0x3f; if (command == 0) break; usleep_range(...); } if (time_after(jiffies, timeout)) nv_warn(...);
Right, now that I look at this code again I don't even understand why I left it this way. Maybe I left some early test code slip into the final patch, sorry about that.
This assumes that there's some known timeout after which the ringmaster is expected to have acked the interrupt. On that note, I wonder if the warning is accurate here: it's my understanding that writing 0x2 to the register does acknowledge the interrupt, so the ringmaster does in fact "clear" it rather than "acknowledge" it, doesn't it?
Although now that I mention it I seem to remember that this write is actually sending a command to the ring master and perhaps waiting for the register to return to 0 is indeed waiting for an ACK of sorts. Maybe adding a comment or so describing what this sequence does would be appropriate here?
Can we from an IP point of view? AFAIK this sequence has never been publicly documented.