[PATCH v2 2/2] drm/omap: partial workaround for DRA7xx DMM errata i878

[Peter Ujfalusi]

Hi Laurent,

On 2018-04-04 00:11, Laurent Pinchart wrote:
>> +static int dmm_dma_copy(struct dmm *dmm, dma_addr_t src, dma_addr_t dst)
>> +{
>> +	struct dma_device *dma_dev = dmm->wa_dma_chan->device;
>> +	struct dma_async_tx_descriptor *tx;
>> +	enum dma_status status;
>> +	dma_cookie_t cookie;
>> +
>> +	tx = dma_dev->device_prep_dma_memcpy(dmm->wa_dma_chan, dst, src, 4, 0);
>> +	if (!tx) {
>> +		dev_err(dmm->dev, "Failed to prepare DMA memcpy\n");
>> +		return -EIO;
>> +	}
>> +
>> +	cookie = tx->tx_submit(tx);
>> +	if (dma_submit_error(cookie)) {
>> +		dev_err(dmm->dev, "Failed to do DMA tx_submit\n");
>> +		return -EIO;
>> +	}
>> +
>> +	dma_async_issue_pending(dmm->wa_dma_chan);
>> +	status = dma_sync_wait(dmm->wa_dma_chan, cookie);
> 
> dma_sync_wait() has a 5s timeout. You're calling this function with a spinlock 
> held. The end result might be slightly better than a complete system lock as 
> caused by the bug described in i878, but only slightly.
> 
> Unless I'm mistaken the reason you can't sleep here is because of the need to 
> access registers in the interrupt handler. Could we use threaded IRQs to solve 
> this ?

You are right that dma_sync_wait() have 5s timeout. But it is theoretical in this case as the DMA would finish the transfer way faster. As and experiment I have tried this change:

@@ -86,6 +87,7 @@ static int dmm_dma_copy(struct dmm *dmm, dma_addr_t src, dma_addr_t dst)
 	struct dma_async_tx_descriptor *tx;
 	enum dma_status status;
 	dma_cookie_t cookie;
+	unsigned int tout = 0;
 
 	tx = dma_dev->device_prep_dma_memcpy(dmm->wa_dma_chan, dst, src, 4, 0);
 	if (!tx) {
@@ -99,10 +101,19 @@ static int dmm_dma_copy(struct dmm *dmm, dma_addr_t src, dma_addr_t dst)
 		return -EIO;
 	}
 
+	dev_err(dmm->dev, "i878 dma_async_issue_pending()\n");
 	dma_async_issue_pending(dmm->wa_dma_chan);
-	status = dma_sync_wait(dmm->wa_dma_chan, cookie);
+	do {
+		status = dmaengine_tx_status(dmm->wa_dma_chan, cookie, NULL);
+		if (status == DMA_COMPLETE)
+			break;
+		cpu_relax();
+	} while (tout++ < 50);
+
 	if (status != DMA_COMPLETE)
 		dev_err(dmm->dev, "i878 wa DMA copy failure\n");
+	else
+		dev_err(dmm->dev, "i878 wa DMA copy OK (tout: %u)\n", tout);
 
 	dmaengine_terminate_all(dmm->wa_dma_chan);
 	return 0;
@@ -286,6 +298,7 @@ static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
 	u32 status = dmm_read(dmm, DMM_PAT_IRQSTATUS);
 	int i;
 
+	dev_err(dmm->dev, "%s - ENTER\n", __func__);
 	/* ack IRQ */
 	dmm_write(dmm, status, DMM_PAT_IRQSTATUS);
 
@@ -305,6 +318,7 @@ static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
 		status >>= 8;
 	}
 
+	dev_err(dmm->dev, "%s - LEAVE\n", __func__);
 	return IRQ_HANDLED;
 }

and get:
[   58.690093 <    0.005598>] dmm 4e000000.dmm: omap_dmm_irq_handler - ENTER
[   58.695601 <    0.005508>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.701284 <    0.005683>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 1)
[   58.706881 <    0.005597>] dmm 4e000000.dmm: omap_dmm_irq_handler - LEAVE
[   58.712465 <    0.005584>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.718150 <    0.005685>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 0)
[   58.724575 <    0.006425>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.730260 <    0.005685>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 1)
[   58.735872 <    0.005612>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.741554 <    0.005682>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 2)
[   58.747174 <    0.005620>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.752857 <    0.005683>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 2)
[   58.758452 <    0.005595>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.764137 <    0.005685>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 1)
[   58.769731 <    0.005594>] dmm 4e000000.dmm: omap_dmm_irq_handler - ENTER
[   58.775237 <    0.005506>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.780919 <    0.005682>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 2)
[   58.786515 <    0.005596>] dmm 4e000000.dmm: omap_dmm_irq_handler - LEAVE
[   58.786545 <    0.000030>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.797698 <    0.011153>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 1)
[   58.811454 <    0.013756>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.817149 <    0.005695>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 0)
[   58.823160 <    0.006011>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.828846 <    0.005686>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 0)
[   58.834761 <    0.005915>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.840446 <    0.005685>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 0)
[   58.846073 <    0.005627>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.851758 <    0.005685>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 0)
[   58.857355 <    0.005597>] dmm 4e000000.dmm: omap_dmm_irq_handler - ENTER
[   58.862865 <    0.005510>] dmm 4e000000.dmm: i878 dma_async_issue_pending()
[   58.868549 <    0.005684>] dmm 4e000000.dmm: i878 wa DMA copy OK (tout: 1)
[   58.874146 <    0.005597>] dmm 4e000000.dmm: omap_dmm_irq_handler - LEAVE

In the long run I have seen one case where tout was 3.
To be honest I was a bit surprised that we have tout > 0.
Despite the tout number, the diff between the issue pending and completion is around 0.005683.
This includes the printout, but I'm lazy to do proper instrumentation.

But, even this in mind it is a bad idea to poll in hard interrupt handler, and we do that for reading the DMM_PAT_IRQSTATUS register.

I'll try to move the dmm driver to threaded irq and mutex.

> 
>> +	if (status != DMA_COMPLETE)
>> +		dev_err(dmm->dev, "i878 wa DMA copy failure\n");
>> +
>> +	dmaengine_terminate_all(dmm->wa_dma_chan);
>> +	return 0;
>> +}

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki