All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-08 21:12 ` Chia-I Wu
  0 siblings, 0 replies; 12+ messages in thread
From: Chia-I Wu @ 2022-04-08 21:12 UTC (permalink / raw)
  To: freedreno
  Cc: dri-devel, Sean Paul, Abhinav Kumar, David Airlie, Sumit Semwal,
	Christian König, linux-arm-msm, Daniel Vetter, Rob Clark

In practice, trace_dma_fence_init is good enough and almost no driver
calls trace_dma_fence_emit.  But this is still more correct in theory.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
Cc: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_gpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index faf0c242874e..a82193f41ea2 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -15,6 +15,7 @@
 #include <linux/string_helpers.h>
 #include <linux/devcoredump.h>
 #include <linux/sched/task.h>
+#include <trace/events/dma_fence.h>
 
 /*
  * Power Management:
@@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
 	gpu->active_submits++;
 	mutex_unlock(&gpu->active_lock);
 
+	trace_dma_fence_emit(submit->hw_fence);
 	gpu->funcs->submit(gpu, submit);
 	gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
 
-- 
2.35.1.1178.g4f1659d476-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-08 21:12 ` Chia-I Wu
  0 siblings, 0 replies; 12+ messages in thread
From: Chia-I Wu @ 2022-04-08 21:12 UTC (permalink / raw)
  To: freedreno
  Cc: Rob Clark, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
	Sumit Semwal, Sean Paul, Christian König

In practice, trace_dma_fence_init is good enough and almost no driver
calls trace_dma_fence_emit.  But this is still more correct in theory.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
Cc: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_gpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index faf0c242874e..a82193f41ea2 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -15,6 +15,7 @@
 #include <linux/string_helpers.h>
 #include <linux/devcoredump.h>
 #include <linux/sched/task.h>
+#include <trace/events/dma_fence.h>
 
 /*
  * Power Management:
@@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
 	gpu->active_submits++;
 	mutex_unlock(&gpu->active_lock);
 
+	trace_dma_fence_emit(submit->hw_fence);
 	gpu->funcs->submit(gpu, submit);
 	gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
 
-- 
2.35.1.1178.g4f1659d476-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
  2022-04-08 21:12 ` Chia-I Wu
@ 2022-04-09  0:06   ` Dmitry Baryshkov
  -1 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-04-09  0:06 UTC (permalink / raw)
  To: Chia-I Wu
  Cc: freedreno, dri-devel, Sean Paul, Abhinav Kumar, David Airlie,
	Sumit Semwal, Christian König, linux-arm-msm, Daniel Vetter,
	Rob Clark

On Sat, 9 Apr 2022 at 00:12, Chia-I Wu <olvaffe@gmail.com> wrote:
>
> In practice, trace_dma_fence_init is good enough and almost no driver
> calls trace_dma_fence_emit.  But this is still more correct in theory.

Please mention in the commit message that the trace_dma_fence_init()
is called from dma_fence_init().
With that in place:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/msm_gpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index faf0c242874e..a82193f41ea2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -15,6 +15,7 @@
>  #include <linux/string_helpers.h>
>  #include <linux/devcoredump.h>
>  #include <linux/sched/task.h>
> +#include <trace/events/dma_fence.h>
>
>  /*
>   * Power Management:
> @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>         gpu->active_submits++;
>         mutex_unlock(&gpu->active_lock);
>
> +       trace_dma_fence_emit(submit->hw_fence);
>         gpu->funcs->submit(gpu, submit);
>         gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>
> --
> 2.35.1.1178.g4f1659d476-goog
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-09  0:06   ` Dmitry Baryshkov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-04-09  0:06 UTC (permalink / raw)
  To: Chia-I Wu
  Cc: Rob Clark, David Airlie, freedreno, Abhinav Kumar, dri-devel,
	Christian König, linux-arm-msm, Sean Paul, Sumit Semwal

On Sat, 9 Apr 2022 at 00:12, Chia-I Wu <olvaffe@gmail.com> wrote:
>
> In practice, trace_dma_fence_init is good enough and almost no driver
> calls trace_dma_fence_emit.  But this is still more correct in theory.

Please mention in the commit message that the trace_dma_fence_init()
is called from dma_fence_init().
With that in place:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/msm_gpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index faf0c242874e..a82193f41ea2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -15,6 +15,7 @@
>  #include <linux/string_helpers.h>
>  #include <linux/devcoredump.h>
>  #include <linux/sched/task.h>
> +#include <trace/events/dma_fence.h>
>
>  /*
>   * Power Management:
> @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>         gpu->active_submits++;
>         mutex_unlock(&gpu->active_lock);
>
> +       trace_dma_fence_emit(submit->hw_fence);
>         gpu->funcs->submit(gpu, submit);
>         gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>
> --
> 2.35.1.1178.g4f1659d476-goog
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
  2022-04-08 21:12 ` Chia-I Wu
@ 2022-04-09 14:33   ` Christian König
  -1 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2022-04-09 14:33 UTC (permalink / raw)
  To: Chia-I Wu, freedreno
  Cc: Rob Clark, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
	Sean Paul, Sumit Semwal

Am 08.04.22 um 23:12 schrieb Chia-I Wu:
> In practice, trace_dma_fence_init is good enough and almost no driver
> calls trace_dma_fence_emit.  But this is still more correct in theory.

Well, the reason why basically no driver is calling this is because it 
is pretty much deprecated.

We do have a case in the GPU scheduler where it makes sense to distinct 
between init and emit, but it doesn't really matter for drivers.

So I'm not sure if it's a good idea to add that here.

Regards,
Christian.

>
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/msm/msm_gpu.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index faf0c242874e..a82193f41ea2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -15,6 +15,7 @@
>   #include <linux/string_helpers.h>
>   #include <linux/devcoredump.h>
>   #include <linux/sched/task.h>
> +#include <trace/events/dma_fence.h>
>   
>   /*
>    * Power Management:
> @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>   	gpu->active_submits++;
>   	mutex_unlock(&gpu->active_lock);
>   
> +	trace_dma_fence_emit(submit->hw_fence);
>   	gpu->funcs->submit(gpu, submit);
>   	gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>   


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-09 14:33   ` Christian König
  0 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2022-04-09 14:33 UTC (permalink / raw)
  To: Chia-I Wu, freedreno
  Cc: dri-devel, Sean Paul, Abhinav Kumar, David Airlie, Sumit Semwal,
	linux-arm-msm, Daniel Vetter, Rob Clark

Am 08.04.22 um 23:12 schrieb Chia-I Wu:
> In practice, trace_dma_fence_init is good enough and almost no driver
> calls trace_dma_fence_emit.  But this is still more correct in theory.

Well, the reason why basically no driver is calling this is because it 
is pretty much deprecated.

We do have a case in the GPU scheduler where it makes sense to distinct 
between init and emit, but it doesn't really matter for drivers.

So I'm not sure if it's a good idea to add that here.

Regards,
Christian.

>
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> Cc: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/msm/msm_gpu.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index faf0c242874e..a82193f41ea2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -15,6 +15,7 @@
>   #include <linux/string_helpers.h>
>   #include <linux/devcoredump.h>
>   #include <linux/sched/task.h>
> +#include <trace/events/dma_fence.h>
>   
>   /*
>    * Power Management:
> @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>   	gpu->active_submits++;
>   	mutex_unlock(&gpu->active_lock);
>   
> +	trace_dma_fence_emit(submit->hw_fence);
>   	gpu->funcs->submit(gpu, submit);
>   	gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>   


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
  2022-04-09 14:33   ` Christian König
@ 2022-04-09 17:45     ` Chia-I Wu
  -1 siblings, 0 replies; 12+ messages in thread
From: Chia-I Wu @ 2022-04-09 17:45 UTC (permalink / raw)
  To: Christian König
  Cc: freedreno, ML dri-devel, Sean Paul, Abhinav Kumar, David Airlie,
	Sumit Semwal, linux-arm-msm, Daniel Vetter, Rob Clark

On Sat, Apr 9, 2022 at 7:33 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 08.04.22 um 23:12 schrieb Chia-I Wu:
> > In practice, trace_dma_fence_init is good enough and almost no driver
> > calls trace_dma_fence_emit.  But this is still more correct in theory.
>
> Well, the reason why basically no driver is calling this is because it
> is pretty much deprecated.
Why is it considered deprecated?  trace_dma_fence_{emit,signaled} are
useful to visualize fence timelines.  I am actually less sure about
how trace_dma_fence_{init,destroy} are used.

Is it because trace_dma_fence_init is called automatically, and is
good enough most of the time?

>
> We do have a case in the GPU scheduler where it makes sense to distinct
> between init and emit, but it doesn't really matter for drivers.
virtio also has a case where init and emit can be far apart, when the
host cannot process commands fast enough and there is no space in
virtqueue.

>
> So I'm not sure if it's a good idea to add that here.
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > Cc: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/msm_gpu.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index faf0c242874e..a82193f41ea2 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/string_helpers.h>
> >   #include <linux/devcoredump.h>
> >   #include <linux/sched/task.h>
> > +#include <trace/events/dma_fence.h>
> >
> >   /*
> >    * Power Management:
> > @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >       gpu->active_submits++;
> >       mutex_unlock(&gpu->active_lock);
> >
> > +     trace_dma_fence_emit(submit->hw_fence);
> >       gpu->funcs->submit(gpu, submit);
> >       gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
> >
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-09 17:45     ` Chia-I Wu
  0 siblings, 0 replies; 12+ messages in thread
From: Chia-I Wu @ 2022-04-09 17:45 UTC (permalink / raw)
  To: Christian König
  Cc: Rob Clark, David Airlie, freedreno, Abhinav Kumar, ML dri-devel,
	linux-arm-msm, Sean Paul, Sumit Semwal

On Sat, Apr 9, 2022 at 7:33 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 08.04.22 um 23:12 schrieb Chia-I Wu:
> > In practice, trace_dma_fence_init is good enough and almost no driver
> > calls trace_dma_fence_emit.  But this is still more correct in theory.
>
> Well, the reason why basically no driver is calling this is because it
> is pretty much deprecated.
Why is it considered deprecated?  trace_dma_fence_{emit,signaled} are
useful to visualize fence timelines.  I am actually less sure about
how trace_dma_fence_{init,destroy} are used.

Is it because trace_dma_fence_init is called automatically, and is
good enough most of the time?

>
> We do have a case in the GPU scheduler where it makes sense to distinct
> between init and emit, but it doesn't really matter for drivers.
virtio also has a case where init and emit can be far apart, when the
host cannot process commands fast enough and there is no space in
virtqueue.

>
> So I'm not sure if it's a good idea to add that here.
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > Cc: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/msm_gpu.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index faf0c242874e..a82193f41ea2 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/string_helpers.h>
> >   #include <linux/devcoredump.h>
> >   #include <linux/sched/task.h>
> > +#include <trace/events/dma_fence.h>
> >
> >   /*
> >    * Power Management:
> > @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >       gpu->active_submits++;
> >       mutex_unlock(&gpu->active_lock);
> >
> > +     trace_dma_fence_emit(submit->hw_fence);
> >       gpu->funcs->submit(gpu, submit);
> >       gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
> >
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
  2022-04-09 14:33   ` Christian König
@ 2022-04-12 19:41     ` Rob Clark
  -1 siblings, 0 replies; 12+ messages in thread
From: Rob Clark @ 2022-04-12 19:41 UTC (permalink / raw)
  To: Christian König
  Cc: David Airlie, freedreno, Abhinav Kumar, dri-devel, Sumit Semwal,
	linux-arm-msm, Sean Paul

On Sat, Apr 9, 2022 at 7:33 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 08.04.22 um 23:12 schrieb Chia-I Wu:
> > In practice, trace_dma_fence_init is good enough and almost no driver
> > calls trace_dma_fence_emit.  But this is still more correct in theory.
>
> Well, the reason why basically no driver is calling this is because it
> is pretty much deprecated.
>
> We do have a case in the GPU scheduler where it makes sense to distinct
> between init and emit, but it doesn't really matter for drivers.
>
> So I'm not sure if it's a good idea to add that here.

visualization can't easily differentiate between drivers/timelines
where the split matters and ones where it doesn't..  IMO it is better
to just have the extra trace even in the cases where it comes at the
same time as the init trace

BR,
-R

> Regards,
> Christian.
>
> >
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > Cc: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/msm_gpu.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index faf0c242874e..a82193f41ea2 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/string_helpers.h>
> >   #include <linux/devcoredump.h>
> >   #include <linux/sched/task.h>
> > +#include <trace/events/dma_fence.h>
> >
> >   /*
> >    * Power Management:
> > @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >       gpu->active_submits++;
> >       mutex_unlock(&gpu->active_lock);
> >
> > +     trace_dma_fence_emit(submit->hw_fence);
> >       gpu->funcs->submit(gpu, submit);
> >       gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
> >
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-12 19:41     ` Rob Clark
  0 siblings, 0 replies; 12+ messages in thread
From: Rob Clark @ 2022-04-12 19:41 UTC (permalink / raw)
  To: Christian König
  Cc: Chia-I Wu, freedreno, dri-devel, Sean Paul, Abhinav Kumar,
	David Airlie, Sumit Semwal, linux-arm-msm, Daniel Vetter

On Sat, Apr 9, 2022 at 7:33 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 08.04.22 um 23:12 schrieb Chia-I Wu:
> > In practice, trace_dma_fence_init is good enough and almost no driver
> > calls trace_dma_fence_emit.  But this is still more correct in theory.
>
> Well, the reason why basically no driver is calling this is because it
> is pretty much deprecated.
>
> We do have a case in the GPU scheduler where it makes sense to distinct
> between init and emit, but it doesn't really matter for drivers.
>
> So I'm not sure if it's a good idea to add that here.

visualization can't easily differentiate between drivers/timelines
where the split matters and ones where it doesn't..  IMO it is better
to just have the extra trace even in the cases where it comes at the
same time as the init trace

BR,
-R

> Regards,
> Christian.
>
> >
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > Cc: Rob Clark <robdclark@chromium.org>
> > ---
> >   drivers/gpu/drm/msm/msm_gpu.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index faf0c242874e..a82193f41ea2 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/string_helpers.h>
> >   #include <linux/devcoredump.h>
> >   #include <linux/sched/task.h>
> > +#include <trace/events/dma_fence.h>
> >
> >   /*
> >    * Power Management:
> > @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >       gpu->active_submits++;
> >       mutex_unlock(&gpu->active_lock);
> >
> > +     trace_dma_fence_emit(submit->hw_fence);
> >       gpu->funcs->submit(gpu, submit);
> >       gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
> >
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
  2022-04-12 19:41     ` Rob Clark
@ 2022-04-26 16:43       ` Christian König
  -1 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2022-04-26 16:43 UTC (permalink / raw)
  To: Rob Clark
  Cc: Chia-I Wu, freedreno, dri-devel, Sean Paul, Abhinav Kumar,
	David Airlie, Sumit Semwal, linux-arm-msm, Daniel Vetter

Sorry for the delayed reply.

Am 12.04.22 um 21:41 schrieb Rob Clark:
> On Sat, Apr 9, 2022 at 7:33 AM Christian König <christian.koenig@amd.com> wrote:
>> Am 08.04.22 um 23:12 schrieb Chia-I Wu:
>>> In practice, trace_dma_fence_init is good enough and almost no driver
>>> calls trace_dma_fence_emit.  But this is still more correct in theory.
>> Well, the reason why basically no driver is calling this is because it
>> is pretty much deprecated.
>>
>> We do have a case in the GPU scheduler where it makes sense to distinct
>> between init and emit, but it doesn't really matter for drivers.
>>
>> So I'm not sure if it's a good idea to add that here.
> visualization can't easily differentiate between drivers/timelines
> where the split matters and ones where it doesn't..  IMO it is better
> to just have the extra trace even in the cases where it comes at the
> same time as the init trace

That's exactly the reason why I want to remove the extra trace.

To make it clear this is only useful for debugging and *NOT* for 
actually visualizing things.

So by adding that here you add more confusion than solving anything.

Regards,
Christian.

>
> BR,
> -R
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
>>> Cc: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/msm_gpu.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>>> index faf0c242874e..a82193f41ea2 100644
>>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/string_helpers.h>
>>>    #include <linux/devcoredump.h>
>>>    #include <linux/sched/task.h>
>>> +#include <trace/events/dma_fence.h>
>>>
>>>    /*
>>>     * Power Management:
>>> @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>>        gpu->active_submits++;
>>>        mutex_unlock(&gpu->active_lock);
>>>
>>> +     trace_dma_fence_emit(submit->hw_fence);
>>>        gpu->funcs->submit(gpu, submit);
>>>        gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit
@ 2022-04-26 16:43       ` Christian König
  0 siblings, 0 replies; 12+ messages in thread
From: Christian König @ 2022-04-26 16:43 UTC (permalink / raw)
  To: Rob Clark
  Cc: David Airlie, freedreno, Abhinav Kumar, dri-devel, Sumit Semwal,
	linux-arm-msm, Sean Paul

Sorry for the delayed reply.

Am 12.04.22 um 21:41 schrieb Rob Clark:
> On Sat, Apr 9, 2022 at 7:33 AM Christian König <christian.koenig@amd.com> wrote:
>> Am 08.04.22 um 23:12 schrieb Chia-I Wu:
>>> In practice, trace_dma_fence_init is good enough and almost no driver
>>> calls trace_dma_fence_emit.  But this is still more correct in theory.
>> Well, the reason why basically no driver is calling this is because it
>> is pretty much deprecated.
>>
>> We do have a case in the GPU scheduler where it makes sense to distinct
>> between init and emit, but it doesn't really matter for drivers.
>>
>> So I'm not sure if it's a good idea to add that here.
> visualization can't easily differentiate between drivers/timelines
> where the split matters and ones where it doesn't..  IMO it is better
> to just have the extra trace even in the cases where it comes at the
> same time as the init trace

That's exactly the reason why I want to remove the extra trace.

To make it clear this is only useful for debugging and *NOT* for 
actually visualizing things.

So by adding that here you add more confusion than solving anything.

Regards,
Christian.

>
> BR,
> -R
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
>>> Cc: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/msm_gpu.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>>> index faf0c242874e..a82193f41ea2 100644
>>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/string_helpers.h>
>>>    #include <linux/devcoredump.h>
>>>    #include <linux/sched/task.h>
>>> +#include <trace/events/dma_fence.h>
>>>
>>>    /*
>>>     * Power Management:
>>> @@ -769,6 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>>        gpu->active_submits++;
>>>        mutex_unlock(&gpu->active_lock);
>>>
>>> +     trace_dma_fence_emit(submit->hw_fence);
>>>        gpu->funcs->submit(gpu, submit);
>>>        gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-04-26 16:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-08 21:12 [PATCH] drm/msm: add trace_dma_fence_emit to msm_gpu_submit Chia-I Wu
2022-04-08 21:12 ` Chia-I Wu
2022-04-09  0:06 ` Dmitry Baryshkov
2022-04-09  0:06   ` Dmitry Baryshkov
2022-04-09 14:33 ` Christian König
2022-04-09 14:33   ` Christian König
2022-04-09 17:45   ` Chia-I Wu
2022-04-09 17:45     ` Chia-I Wu
2022-04-12 19:41   ` Rob Clark
2022-04-12 19:41     ` Rob Clark
2022-04-26 16:43     ` Christian König
2022-04-26 16:43       ` Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.