Nouveau - Apr 2014 - [PATCH 06/12] drm/nouveau/ibus: add GK20A support

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()?
> +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(...);

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?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140324/16ffb2ac/attachment-0001.sig>

On Tue, Mar 25, 2014 at 7:34 AM, Thierry Reding
<thierry.reding at 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.

On Wed, Apr 2, 2014 at 9:52 AM, Alexandre Courbot <gnurou at gmail.com>
wrote:
> On Tue, Mar 25, 2014 at 7:34 AM, Thierry Reding
> <thierry.reding at 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.
I just remembered about this, but there's also the nv_wait() macro,
which you could use, e.g.

if (!nv_wait(subdev, 0x12004c, 0x3f, 0x00))
  nv_warn(...)

It has built-in timeout logic/etc (although no sleeps in the middle).
It does use the timer subdev, so if that's not operational at this
point, you can't use it.
>
>> 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.
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau