| Message ID | 20241003-preemption-a750-t-v8-9-5c6cb9f256e0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Preemption support for A7XX | expand |
Op 03-10-2024 om 18:12 schreef Antonino Maniscalco: > Add trace points corresponding to preemption being triggered and being > completed for latency measurement purposes. > > Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > Tested-by: Rob Clark <robdclark@gmail.com> > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8450-HDK > Signed-off-by: Antonino Maniscalco <antomani103@gmail.com> > --- > drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 6 ++++++ > drivers/gpu/drm/msm/msm_gpu_trace.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > index 21e333cb6342d33425eb96f97bcc853e9b041b36..6803d5af60cc8fb0f2a52ee160ffdbf0e8ef0209 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > @@ -7,6 +7,7 @@ > #include "a6xx_gpu.h" > #include "a6xx_gmu.xml.h" > #include "msm_mmu.h" > +#include "msm_gpu_trace.h" > > /* > * Try to transition the preemption state from old to new. Return > @@ -174,6 +175,8 @@ void a6xx_preempt_irq(struct msm_gpu *gpu) > > set_preempt_state(a6xx_gpu, PREEMPT_NONE); > > + trace_msm_gpu_preemption_irq(a6xx_gpu->cur_ring->id); > + > /* > * Retrigger preemption to avoid a deadlock that might occur when preemption > * is skipped due to it being already in flight when requested. > @@ -294,6 +297,9 @@ void a6xx_preempt_trigger(struct msm_gpu *gpu) > */ > ring->restore_wptr = false; > > + trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, > + ring ? ring->id : -1); > + There is no need for the ternary operator. "ring" should be non-NULL, otherwise the code would have already crashed. So the change can just be trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, ring->id);
On 10/8/24 11:10 PM, Kees Bakker wrote: > Op 03-10-2024 om 18:12 schreef Antonino Maniscalco: >> Add trace points corresponding to preemption being triggered and being >> completed for latency measurement purposes. >> >> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >> Tested-by: Rob Clark <robdclark@gmail.com> >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8450-HDK >> Signed-off-by: Antonino Maniscalco <antomani103@gmail.com> >> --- >> drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 6 ++++++ >> drivers/gpu/drm/msm/msm_gpu_trace.h | 28 +++++++++++++++++++++ >> +++++++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c b/drivers/gpu/ >> drm/msm/adreno/a6xx_preempt.c >> index >> 21e333cb6342d33425eb96f97bcc853e9b041b36..6803d5af60cc8fb0f2a52ee160ffdbf0e8ef0209 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c >> @@ -7,6 +7,7 @@ >> #include "a6xx_gpu.h" >> #include "a6xx_gmu.xml.h" >> #include "msm_mmu.h" >> +#include "msm_gpu_trace.h" >> /* >> * Try to transition the preemption state from old to new. Return >> @@ -174,6 +175,8 @@ void a6xx_preempt_irq(struct msm_gpu *gpu) >> set_preempt_state(a6xx_gpu, PREEMPT_NONE); >> + trace_msm_gpu_preemption_irq(a6xx_gpu->cur_ring->id); >> + >> /* >> * Retrigger preemption to avoid a deadlock that might occur >> when preemption >> * is skipped due to it being already in flight when requested. >> @@ -294,6 +297,9 @@ void a6xx_preempt_trigger(struct msm_gpu *gpu) >> */ >> ring->restore_wptr = false; >> + trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, >> + ring ? ring->id : -1); >> + > There is no need for the ternary operator. "ring" should be non-NULL, > otherwise the code would have already crashed. > So the change can just be > trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, ring->id); You are right, we had a similar cleanup but I missed this particular one, thanks for pointing me at it! I apologize for the late response but I've been at XDC and therefore unable to look at my email. I will point this out to Rob since this series is in msm-next to see if I need to send a separate patch to clean this. Best regards,
On Wed, Oct 16, 2024 at 5:13 AM Antonino Maniscalco <antomani103@gmail.com> wrote: > > On 10/8/24 11:10 PM, Kees Bakker wrote: > > Op 03-10-2024 om 18:12 schreef Antonino Maniscalco: > >> Add trace points corresponding to preemption being triggered and being > >> completed for latency measurement purposes. > >> > >> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > >> Tested-by: Rob Clark <robdclark@gmail.com> > >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD > >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD > >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8450-HDK > >> Signed-off-by: Antonino Maniscalco <antomani103@gmail.com> > >> --- > >> drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 6 ++++++ > >> drivers/gpu/drm/msm/msm_gpu_trace.h | 28 +++++++++++++++++++++ > >> +++++++ > >> 2 files changed, 34 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c b/drivers/gpu/ > >> drm/msm/adreno/a6xx_preempt.c > >> index > >> 21e333cb6342d33425eb96f97bcc853e9b041b36..6803d5af60cc8fb0f2a52ee160ffdbf0e8ef0209 100644 > >> --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > >> @@ -7,6 +7,7 @@ > >> #include "a6xx_gpu.h" > >> #include "a6xx_gmu.xml.h" > >> #include "msm_mmu.h" > >> +#include "msm_gpu_trace.h" > >> /* > >> * Try to transition the preemption state from old to new. Return > >> @@ -174,6 +175,8 @@ void a6xx_preempt_irq(struct msm_gpu *gpu) > >> set_preempt_state(a6xx_gpu, PREEMPT_NONE); > >> + trace_msm_gpu_preemption_irq(a6xx_gpu->cur_ring->id); > >> + > >> /* > >> * Retrigger preemption to avoid a deadlock that might occur > >> when preemption > >> * is skipped due to it being already in flight when requested. > >> @@ -294,6 +297,9 @@ void a6xx_preempt_trigger(struct msm_gpu *gpu) > >> */ > >> ring->restore_wptr = false; > >> + trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, > >> + ring ? ring->id : -1); > >> + > > There is no need for the ternary operator. "ring" should be non-NULL, > > otherwise the code would have already crashed. > > So the change can just be > > trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, ring->id); > > You are right, we had a similar cleanup but I missed this particular > one, thanks for pointing me at it! I apologize for the late response but > I've been at XDC and therefore unable to look at my email. I will point > this out to Rob since this series is in msm-next to see if I need to > send a separate patch to clean this. Yes, please send a new commit, I don't want to re-write history on msm-next BR, -R > Best regards, > -- > Antonino Maniscalco <antomani103@gmail.com>
On 10/16/24 10:33 PM, Rob Clark wrote: > On Wed, Oct 16, 2024 at 5:13 AM Antonino Maniscalco > <antomani103@gmail.com> wrote: >> >> On 10/8/24 11:10 PM, Kees Bakker wrote: >>> Op 03-10-2024 om 18:12 schreef Antonino Maniscalco: >>>> Add trace points corresponding to preemption being triggered and being >>>> completed for latency measurement purposes. >>>> >>>> Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>> Tested-by: Rob Clark <robdclark@gmail.com> >>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD >>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD >>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8450-HDK >>>> Signed-off-by: Antonino Maniscalco <antomani103@gmail.com> >>>> --- >>>> drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 6 ++++++ >>>> drivers/gpu/drm/msm/msm_gpu_trace.h | 28 +++++++++++++++++++++ >>>> +++++++ >>>> 2 files changed, 34 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c b/drivers/gpu/ >>>> drm/msm/adreno/a6xx_preempt.c >>>> index >>>> 21e333cb6342d33425eb96f97bcc853e9b041b36..6803d5af60cc8fb0f2a52ee160ffdbf0e8ef0209 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c >>>> @@ -7,6 +7,7 @@ >>>> #include "a6xx_gpu.h" >>>> #include "a6xx_gmu.xml.h" >>>> #include "msm_mmu.h" >>>> +#include "msm_gpu_trace.h" >>>> /* >>>> * Try to transition the preemption state from old to new. Return >>>> @@ -174,6 +175,8 @@ void a6xx_preempt_irq(struct msm_gpu *gpu) >>>> set_preempt_state(a6xx_gpu, PREEMPT_NONE); >>>> + trace_msm_gpu_preemption_irq(a6xx_gpu->cur_ring->id); >>>> + >>>> /* >>>> * Retrigger preemption to avoid a deadlock that might occur >>>> when preemption >>>> * is skipped due to it being already in flight when requested. >>>> @@ -294,6 +297,9 @@ void a6xx_preempt_trigger(struct msm_gpu *gpu) >>>> */ >>>> ring->restore_wptr = false; >>>> + trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, >>>> + ring ? ring->id : -1); >>>> + >>> There is no need for the ternary operator. "ring" should be non-NULL, >>> otherwise the code would have already crashed. >>> So the change can just be >>> trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, ring->id); >> >> You are right, we had a similar cleanup but I missed this particular >> one, thanks for pointing me at it! I apologize for the late response but >> I've been at XDC and therefore unable to look at my email. I will point >> this out to Rob since this series is in msm-next to see if I need to >> send a separate patch to clean this. > > Yes, please send a new commit, I don't want to re-write history on msm-next Makes sense. I noticed somebody else has already sent the patch for it https://lore.kernel.org/linux-arm-msm/20241011052315.4713-1-everestkc@everestkc.com.np/ > > BR, > -R > >> Best regards, >> -- >> Antonino Maniscalco <antomani103@gmail.com> Best regards,
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c index 21e333cb6342d33425eb96f97bcc853e9b041b36..6803d5af60cc8fb0f2a52ee160ffdbf0e8ef0209 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c @@ -7,6 +7,7 @@ #include "a6xx_gpu.h" #include "a6xx_gmu.xml.h" #include "msm_mmu.h" +#include "msm_gpu_trace.h" /* * Try to transition the preemption state from old to new. Return @@ -174,6 +175,8 @@ void a6xx_preempt_irq(struct msm_gpu *gpu) set_preempt_state(a6xx_gpu, PREEMPT_NONE); + trace_msm_gpu_preemption_irq(a6xx_gpu->cur_ring->id); + /* * Retrigger preemption to avoid a deadlock that might occur when preemption * is skipped due to it being already in flight when requested. @@ -294,6 +297,9 @@ void a6xx_preempt_trigger(struct msm_gpu *gpu) */ ring->restore_wptr = false; + trace_msm_gpu_preemption_trigger(a6xx_gpu->cur_ring->id, + ring ? ring->id : -1); + spin_unlock_irqrestore(&ring->preempt_lock, flags); gpu_write64(gpu, diff --git a/drivers/gpu/drm/msm/msm_gpu_trace.h b/drivers/gpu/drm/msm/msm_gpu_trace.h index ac40d857bc4578377b03b4cedd138c87144997e4..7f863282db0d7812c8fd53b3f1fc0cd5635028ba 100644 --- a/drivers/gpu/drm/msm/msm_gpu_trace.h +++ b/drivers/gpu/drm/msm/msm_gpu_trace.h @@ -177,6 +177,34 @@ TRACE_EVENT(msm_gpu_resume, TP_printk("%u", __entry->dummy) ); +TRACE_EVENT(msm_gpu_preemption_trigger, + TP_PROTO(int ring_id_from, int ring_id_to), + TP_ARGS(ring_id_from, ring_id_to), + TP_STRUCT__entry( + __field(int, ring_id_from) + __field(int, ring_id_to) + ), + TP_fast_assign( + __entry->ring_id_from = ring_id_from; + __entry->ring_id_to = ring_id_to; + ), + TP_printk("preempting %u -> %u", + __entry->ring_id_from, + __entry->ring_id_to) +); + +TRACE_EVENT(msm_gpu_preemption_irq, + TP_PROTO(u32 ring_id), + TP_ARGS(ring_id), + TP_STRUCT__entry( + __field(u32, ring_id) + ), + TP_fast_assign( + __entry->ring_id = ring_id; + ), + TP_printk("preempted to %u", __entry->ring_id) +); + #endif #undef TRACE_INCLUDE_PATH