[Nouveau] Delays in DRM nouveau_bios.c

[Pekka Paalanen]

On Thu, 20 Aug 2009 14:40:24 +0300
Pekka Paalanen <pq at iki.fi> wrote:

> Hi,
> 
> questions will follow.

This patch builds, but I have not tried to run it.
I'm hoping Stuart or someone could comment on it.

> ---
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c b/drivers/gpu/drm/nouveau/nouveau_bios.c
> index 99f7bd4..13b3fb1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bios.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
> @@ -40,8 +40,6 @@
>  #define BIOSLOG(sip, fmt, arg...) NV_DEBUG(sip->dev, fmt, ##arg)
>  #define LOG_OLD_VALUE(x) //x
>  
> -#define BIOS_USLEEP(n) mdelay((n)/1000)
> -
>  #define ROM16(x) le16_to_cpu(*(uint16_t *)&(x))
>  #define ROM32(x) le32_to_cpu(*(uint32_t *)&(x))
>  
> @@ -50,6 +48,15 @@ struct init_exec {
>  	bool repeat;
>  };
>  
> +static inline void bios_usleep(unsigned usecs)
> +{
> +	might_sleep();
> +	if (usecs < 1000 * MAX_UDELAY_MS)
> +		udelay(usecs);
> +	else
> +		msleep(usecs / 1000 + 1);
> +}

This function is to replace the BIOS_USLEEP() macro with something,
that works both for short and long delays. BIOS_USLEEP would truncate
all sleeps to milliseconds, meaning delays less than 1000 us would
be zero. Also, mdelay() is a busyloop delay burning CPU cycles for
nothing. msleep() is not, it will reschedule until the delay has
passed. The change raises two questions:

- Is the bios code ever called in a situation, where scheduling
is not allowed (interrupt context, pre-emption disabled, inside
spinlocks, etc.)?

- Is the delay accuracy on the msleep path critical?

>  static bool nv_cksum(const uint8_t *data, unsigned int length)
>  {
>  	/* there's a few checksums in the BIOS, so here's a generic checking function */
> @@ -262,7 +269,7 @@ static int parse_init_table(struct nvbios *, unsigned int, struct init_exec *);
>  static void still_alive(void)
>  {
>  //	sync();
> -//	BIOS_USLEEP(2000);
> +//	bios_usleep(2000);
>  }
>  
>  static uint32_t
> @@ -1608,17 +1615,18 @@ init_condition_time(struct nvbios *bios, uint16_t offset,
>  
>  	for (; retries > 0; retries--)
>  		if (bios_condition_met(bios, offset, cond)) {
> -			BIOSLOG(bios, "0x%04X: Condition met, continuing\n", offset);
> +			BIOSLOG(bios, "0x%04X: Condition met, continuing\n",
> +								offset);
>  			break;
>  		} else {
>  			BIOSLOG(bios, "0x%04X: Condition not met, sleeping for 20ms\n", offset);
> -			BIOS_USLEEP(20000);
> +			bios_usleep(20000);

It so happens, that 20000 was the maximum allowed udelay() constant time
for x86. Did the number come from that?

>  		}
>  
>  	if (!bios_condition_met(bios, offset, cond)) {
>  		NV_WARN(bios->dev,
>  			"0x%04X: Condition still not met after %dms, "
> -			"skiping following opcodes\n", offset, 20 * retries);
> +			"skipping following opcodes\n", offset, 20 * retries);
>  		iexec->execute = false;
>  	}
>  
> @@ -1851,7 +1859,7 @@ init_reset(struct nvbios *bios, uint16_t offset, struct init_exec *iexec)
>  	bios_wr32(bios, NV_PBUS_PCI_NV_19, 0);
>  	bios_wr32(bios, reg, value1);
>  
> -	BIOS_USLEEP(10);
> +	bios_usleep(10);

With the old BIOS_USLEEP, this delay would vanish.

>  
>  	bios_wr32(bios, reg, value2);
>  	bios_wr32(bios, NV_PBUS_PCI_NV_19, pci_nv_19);
> @@ -2233,8 +2241,7 @@ init_time(struct nvbios *bios, uint16_t offset, struct init_exec *iexec)
>  	BIOSLOG(bios, "0x%04X: Sleeping for 0x%04X microseconds\n",
>  		offset, time);
>  
> -	BIOS_USLEEP(time);
> -
> +	bios_usleep(time);

Can here be delay accuracy issues? How much are we allowed to exceed the
specified delay?

>  	return true;
>  }
>  
> @@ -2872,9 +2879,11 @@ static int call_lvds_manufacturer_script(struct drm_device *dev, struct dcb_entr
>  
>  	run_digital_op_script(dev, scriptofs, dcbent, head, bios->fp.dual_link);
>  
> -	if (script == LVDS_PANEL_OFF)
> +	if (script == LVDS_PANEL_OFF) {
>  		/* off-on delay in ms */
> -		BIOS_USLEEP(ROM16(bios->data[bios->fp.xlated_entry + 7]));
> +		bios_usleep(ROM16(bios->data[bios->fp.xlated_entry + 7]));
> +	}

The comment and code disagree here. Is it microseconds or milliseconds?

-- 
Pekka Paalanen
http://www.iki.fi/pq/