diff mbox series

[5/5] dma: tegra: add tracepoints to driver

Message ID 20181121161323.20226-6-ben.dooks@codethink.co.uk
State Deferred
Headers show
Series [1/5] dma: tegra: avoid overflow of byte tracking | expand

Commit Message

Ben Dooks Nov. 21, 2018, 4:13 p.m. UTC 
Add some trace-points to the driver to allow for debuging via the
trace pipe.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Fixes since v1:
- take copy of dmachan name instead of pointer to device

Fixes since v2:
- finish off Steven's review comments
- fix saving the dma channel name
- use txstate as argument to trace_tegra_dma_tx_status()
- fix type of dma_cookie to dma_cookie_t

Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
---
 drivers/dma/tegra20-apb-dma.c        |  7 ++++
 include/trace/events/tegra_apb_dma.h | 61 ++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 include/trace/events/tegra_apb_dma.h

Comments

Steven Rostedt Nov. 21, 2018, 5:21 p.m. UTC | #1 
On Wed, 21 Nov 2018 16:13:23 +0000
Ben Dooks <ben.dooks@codethink.co.uk> wrote:

> Add some trace-points to the driver to allow for debuging via the
> trace pipe.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

From a tracing perspective I see nothing wrong with this patch.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> ---
> Fixes since v1:
> - take copy of dmachan name instead of pointer to device
> 
> Fixes since v2:
> - finish off Steven's review comments
> - fix saving the dma channel name
> - use txstate as argument to trace_tegra_dma_tx_status()
> - fix type of dma_cookie to dma_cookie_t
> 
> Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
> Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
Ben Dooks Nov. 22, 2018, 8:29 a.m. UTC | #2 
On 21/11/2018 17:21, Steven Rostedt wrote:
> On Wed, 21 Nov 2018 16:13:23 +0000
> Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> 
>> Add some trace-points to the driver to allow for debuging via the
>> trace pipe.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> 
>  From a tracing perspective I see nothing wrong with this patch.
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Thank you for the feedback.
Jon Hunter Nov. 22, 2018, 8:55 a.m. UTC | #3 
On 21/11/2018 16:13, Ben Dooks wrote:
> Add some trace-points to the driver to allow for debuging via the
> trace pipe.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Fixes since v1:
> - take copy of dmachan name instead of pointer to device
> 
> Fixes since v2:
> - finish off Steven's review comments
> - fix saving the dma channel name
> - use txstate as argument to trace_tegra_dma_tx_status()
> - fix type of dma_cookie to dma_cookie_t
> 
> Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
> Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
> ---
>  drivers/dma/tegra20-apb-dma.c        |  7 ++++
>  include/trace/events/tegra_apb_dma.h | 61 ++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
>  create mode 100644 include/trace/events/tegra_apb_dma.h
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index c60c3f7cad5f..659b49bf6fdb 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -38,6 +38,9 @@
>  
>  #include "dmaengine.h"
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/tegra_apb_dma.h>
> +
>  #define TEGRA_APBDMA_GENERAL			0x0
>  #define TEGRA_APBDMA_GENERAL_ENABLE		BIT(31)
>  
> @@ -672,6 +675,8 @@ static void tegra_dma_tasklet(unsigned long data)
>  		dmaengine_desc_get_callback(&dma_desc->txd, &cb);
>  		cb_count = dma_desc->cb_count;
>  		dma_desc->cb_count = 0;
> +		trace_tegra_dma_complete_cb(&tdc->dma_chan, cb_count,
> +					    cb.callback);
>  		spin_unlock_irqrestore(&tdc->lock, flags);
>  		while (cb_count--)
>  			dmaengine_desc_callback_invoke(&cb, NULL);
> @@ -688,6 +693,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
>  
>  	spin_lock_irqsave(&tdc->lock, flags);
>  
> +	trace_tegra_dma_isr(&tdc->dma_chan, irq);
>  	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>  	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
>  		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
> @@ -846,6 +852,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>  		dma_set_residue(txstate, residual);
>  	}
>  
> +	trace_tegra_dma_tx_status(&tdc->dma_chan, cookie, txstate);
>  	spin_unlock_irqrestore(&tdc->lock, flags);
>  	return ret;
>  }

My only comment here is that apart from the ISR trace, the others could
be added to the dmaengine driver core and then these could be used for
other DMAs as well.

Cheers
Jon
Ben Dooks Nov. 22, 2018, 3:15 p.m. UTC | #4 
On 22/11/2018 08:55, Jon Hunter wrote:
> 
> On 21/11/2018 16:13, Ben Dooks wrote:
>> Add some trace-points to the driver to allow for debuging via the
>> trace pipe.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> Fixes since v1:
>> - take copy of dmachan name instead of pointer to device
>>
>> Fixes since v2:
>> - finish off Steven's review comments
>> - fix saving the dma channel name
>> - use txstate as argument to trace_tegra_dma_tx_status()
>> - fix type of dma_cookie to dma_cookie_t
>>
>> Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
>> Cc: Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
>> ---
>>   drivers/dma/tegra20-apb-dma.c        |  7 ++++
>>   include/trace/events/tegra_apb_dma.h | 61 ++++++++++++++++++++++++++++
>>   2 files changed, 68 insertions(+)
>>   create mode 100644 include/trace/events/tegra_apb_dma.h
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index c60c3f7cad5f..659b49bf6fdb 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -38,6 +38,9 @@
>>   
>>   #include "dmaengine.h"
>>   
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/tegra_apb_dma.h>
>> +
>>   #define TEGRA_APBDMA_GENERAL			0x0
>>   #define TEGRA_APBDMA_GENERAL_ENABLE		BIT(31)
>>   
>> @@ -672,6 +675,8 @@ static void tegra_dma_tasklet(unsigned long data)
>>   		dmaengine_desc_get_callback(&dma_desc->txd, &cb);
>>   		cb_count = dma_desc->cb_count;
>>   		dma_desc->cb_count = 0;
>> +		trace_tegra_dma_complete_cb(&tdc->dma_chan, cb_count,
>> +					    cb.callback);
>>   		spin_unlock_irqrestore(&tdc->lock, flags);
>>   		while (cb_count--)
>>   			dmaengine_desc_callback_invoke(&cb, NULL);
>> @@ -688,6 +693,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
>>   
>>   	spin_lock_irqsave(&tdc->lock, flags);
>>   
>> +	trace_tegra_dma_isr(&tdc->dma_chan, irq);
>>   	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>>   	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
>>   		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
>> @@ -846,6 +852,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>>   		dma_set_residue(txstate, residual);
>>   	}
>>   
>> +	trace_tegra_dma_tx_status(&tdc->dma_chan, cookie, txstate);
>>   	spin_unlock_irqrestore(&tdc->lock, flags);
>>   	return ret;
>>   }
> 
> My only comment here is that apart from the ISR trace, the others could
> be added to the dmaengine driver core and then these could be used for
> other DMAs as well.

Hmm, I will go have a look at how difficult that would be.
Vinod Koul Jan. 4, 2019, 3:06 p.m. UTC | #5 
On 22-11-18, 15:15, Ben Dooks wrote:
> On 22/11/2018 08:55, Jon Hunter wrote:

> > My only comment here is that apart from the ISR trace, the others could
> > be added to the dmaengine driver core and then these could be used for
> > other DMAs as well.
> 
> Hmm, I will go have a look at how difficult that would be.

Shouldn't be :) All the drivers use generic dma_chan struct and we can
use that to trace, but I am going to merges this and you can send a
patch to move it to core :)
Ben Dooks Jan. 4, 2019, 3:10 p.m. UTC | #6 
On 2019-01-04 15:06, Vinod Koul wrote:
> On 22-11-18, 15:15, Ben Dooks wrote:
>> On 22/11/2018 08:55, Jon Hunter wrote:
> 
>> > My only comment here is that apart from the ISR trace, the others could
>> > be added to the dmaengine driver core and then these could be used for
>> > other DMAs as well.
>> 
>> Hmm, I will go have a look at how difficult that would be.
> 
> Shouldn't be :) All the drivers use generic dma_chan struct and we can
> use that to trace, but I am going to merges this and you can send a
> patch to move it to core :)

Ok, so just to check that the whole series is going to be merged?
I was just wondering if anything needs to be re-sent as I had not
heard anything back on this series in a while.
Vinod Koul Jan. 4, 2019, 3:24 p.m. UTC | #7 
On 04-01-19, 15:10, Ben Dooks wrote:
> 
> 
> On 2019-01-04 15:06, Vinod Koul wrote:
> > On 22-11-18, 15:15, Ben Dooks wrote:
> > > On 22/11/2018 08:55, Jon Hunter wrote:
> > 
> > > > My only comment here is that apart from the ISR trace, the others could
> > > > be added to the dmaengine driver core and then these could be used for
> > > > other DMAs as well.
> > > 
> > > Hmm, I will go have a look at how difficult that would be.
> > 
> > Shouldn't be :) All the drivers use generic dma_chan struct and we can
> > use that to trace, but I am going to merges this and you can send a
> > patch to move it to core :)
> 
> Ok, so just to check that the whole series is going to be merged?
> I was just wondering if anything needs to be re-sent as I had not
> heard anything back on this series in a while.

Yeah sorry about unusual delay, travel, vacation, work life caused a bit
of issues for me. Things should be fine now...

I have merged the entire series and it will show up in next after merge
window closes...
diff mbox series

Patch

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c60c3f7cad5f..659b49bf6fdb 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -38,6 +38,9 @@ 
 
 #include "dmaengine.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/tegra_apb_dma.h>
+
 #define TEGRA_APBDMA_GENERAL			0x0
 #define TEGRA_APBDMA_GENERAL_ENABLE		BIT(31)
 
@@ -672,6 +675,8 @@  static void tegra_dma_tasklet(unsigned long data)
 		dmaengine_desc_get_callback(&dma_desc->txd, &cb);
 		cb_count = dma_desc->cb_count;
 		dma_desc->cb_count = 0;
+		trace_tegra_dma_complete_cb(&tdc->dma_chan, cb_count,
+					    cb.callback);
 		spin_unlock_irqrestore(&tdc->lock, flags);
 		while (cb_count--)
 			dmaengine_desc_callback_invoke(&cb, NULL);
@@ -688,6 +693,7 @@  static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 
 	spin_lock_irqsave(&tdc->lock, flags);
 
+	trace_tegra_dma_isr(&tdc->dma_chan, irq);
 	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
 	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
@@ -846,6 +852,7 @@  static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 		dma_set_residue(txstate, residual);
 	}
 
+	trace_tegra_dma_tx_status(&tdc->dma_chan, cookie, txstate);
 	spin_unlock_irqrestore(&tdc->lock, flags);
 	return ret;
 }
diff --git a/include/trace/events/tegra_apb_dma.h b/include/trace/events/tegra_apb_dma.h
new file mode 100644
index 000000000000..0818f6286110
--- /dev/null
+++ b/include/trace/events/tegra_apb_dma.h
@@ -0,0 +1,61 @@ 
+#if !defined(_TRACE_TEGRA_APB_DMA_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TEGRA_APM_DMA_H
+
+#include <linux/tracepoint.h>
+#include <linux/dmaengine.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM tegra_apb_dma
+
+TRACE_EVENT(tegra_dma_tx_status,
+	TP_PROTO(struct dma_chan *dc, dma_cookie_t cookie, struct dma_tx_state *state),
+	TP_ARGS(dc, cookie, state),
+	TP_STRUCT__entry(
+		__string(chan,	dev_name(&dc->dev->device))
+		__field(dma_cookie_t, cookie)
+		__field(__u32,	residue)
+	),
+	TP_fast_assign(
+		__assign_str(chan, dev_name(&dc->dev->device));
+		__entry->cookie = cookie;
+		__entry->residue = state ? state->residue : (u32)-1;
+	),
+	TP_printk("channel %s: dma cookie %d, residue %u",
+		  __get_str(chan), __entry->cookie, __entry->residue)
+);
+
+TRACE_EVENT(tegra_dma_complete_cb,
+	TP_PROTO(struct dma_chan *dc, int count, void *ptr),
+	TP_ARGS(dc, count, ptr),
+	TP_STRUCT__entry(
+		__string(chan,	dev_name(&dc->dev->device))
+		__field(int,	count)
+		__field(void *,	ptr)
+		),
+	TP_fast_assign(
+		__assign_str(chan, dev_name(&dc->dev->device));
+		__entry->count = count;
+		__entry->ptr = ptr;
+		),
+	TP_printk("channel %s: done %d, ptr %p",
+		  __get_str(chan), __entry->count, __entry->ptr)
+);
+
+TRACE_EVENT(tegra_dma_isr,
+	TP_PROTO(struct dma_chan *dc, int irq),
+	TP_ARGS(dc, irq),
+	TP_STRUCT__entry(
+		__string(chan,	dev_name(&dc->dev->device))
+		__field(int,	irq)
+	),
+	TP_fast_assign(
+		__assign_str(chan, dev_name(&dc->dev->device));
+		__entry->irq = irq;
+	),
+	TP_printk("%s: irq %d\n",  __get_str(chan), __entry->irq)
+);
+
+#endif /*  _TRACE_TEGRADMA_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>