Re: [Freedreno] [PATCH 3/3] drm/msm: dpu: Don't store/deref pointers in trace ringbuffer

2018-09-19 Thread Sean Paul
On Wed, Sep 19, 2018 at 01:04:42PM -0700, Abhinav Kumar wrote:
> On 2018-09-19 11:33, Sean Paul wrote:
> > From: Sean Paul 
> > 
> > TP_printk is not synchronous, so storing pointers and then later
> > derferencing them is a Bad Idea. This patch stores everything locally to
> minor typo "dereferencing",
> > avoid display stomped memory.
> > 
> > Signed-off-by: Sean Paul 
> After fixing the minor nit please add,
> Reviewed-by: Abhinav Kumar 

Thanks for the reviews, Abhinav, I've stuffed the set in dpu-staging/for-next.

Sean

> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 98 +--
> >  1 file changed, 55 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > index ae6b0c51ba52..e12c4cefb742 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> > @@ -686,37 +686,41 @@ TRACE_EVENT(dpu_crtc_setup_mixer,
> > TP_STRUCT__entry(
> > __field(uint32_t,   crtc_id )
> > __field(uint32_t,   plane_id)
> > -   __field(struct drm_plane_state*,state   )
> > -   __field(struct dpu_plane_state*,pstate  )
> > +   __field(uint32_t,   fb_id   )
> > +   __field_struct( struct drm_rect,src_rect)
> > +   __field_struct( struct drm_rect,dst_rect)
> > __field(uint32_t,   stage_idx   )
> > +   __field(enum dpu_stage, stage   )
> > __field(enum dpu_sspp,  sspp)
> > +   __field(uint32_t,   multirect_idx   )
> > +   __field(uint32_t,   multirect_mode  )
> > __field(uint32_t,   pixel_format)
> > __field(uint64_t,   modifier)
> > ),
> > TP_fast_assign(
> > __entry->crtc_id = crtc_id;
> > __entry->plane_id = plane_id;
> > -   __entry->state = state;
> > -   __entry->pstate = pstate;
> > +   __entry->fb_id = state ? state->fb->base.id : 0;
> > +   __entry->src_rect = drm_plane_state_src(state);
> > +   __entry->dst_rect = drm_plane_state_dest(state);
> > __entry->stage_idx = stage_idx;
> > +   __entry->stage = pstate->stage;
> > __entry->sspp = sspp;
> > +   __entry->multirect_idx = pstate->multirect_index;
> > +   __entry->multirect_mode = pstate->multirect_mode;
> > __entry->pixel_format = pixel_format;
> > __entry->modifier = modifier;
> > ),
> > -   TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:{%ux%u+%ux%u} "
> > - "dst:{%ux%u+%ux%u} stage_idx:%u stage:%d, sspp:%d "
> > +   TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:" DRM_RECT_FP_FMT
> > + " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, sspp:%d "
> >   "multirect_index:%d multirect_mode:%u pix_format:%u "
> >   "modifier:%llu",
> > - __entry->crtc_id, __entry->plane_id,
> > - __entry->state->fb ? __entry->state->fb->base.id : -1,
> > - __entry->state->src_w >> 16,  __entry->state->src_h >> 16,
> > - __entry->state->src_x >> 16,  __entry->state->src_y >> 16,
> > - __entry->state->crtc_w,  __entry->state->crtc_h,
> > - __entry->state->crtc_x,  __entry->state->crtc_y,
> > - __entry->stage_idx, __entry->pstate->stage, __entry->sspp,
> > - __entry->pstate->multirect_index,
> > - __entry->pstate->multirect_mode, __entry->pixel_format,
> > - __entry->modifier)
> > + __entry->crtc_id, __entry->plane_id, __entry->fb_id,
> > + DRM_RECT_FP_ARG(&__entry->src_rect),
> > + DRM_RECT_ARG(&__entry->dst_rect),
> > + __entry->stage_idx, __entry->stage, __entry->sspp,
> > + __entry->multirect_idx, __entry->multirect_mode,
> > + __entry->pixel_format, __entry->modifier)
> >  );
> > 
> >  TRACE_EVENT(dpu_crtc_setup_lm_bounds,
> > @@ -725,15 +729,15 @@ TRACE_EVENT(dpu_crtc_setup_lm_bounds,
> > TP_STRUCT__entry(
> > __field(uint32_t,   drm_id  )
> > __field(int,mixer   )
> > -   __field(struct drm_rect *,  bounds  )
> > +   __field_struct( struct drm_rect,bounds  )
> > ),
> > TP_fast_assign(
> > __entry->drm_id = drm_id;
> > __entry->mixer = mixer;
> > -   __entry->bounds = bounds;
> > +   __entry->bounds = *bounds;
> > ),
> > TP_printk("id:%u mixer:%d bounds:" DRM_RECT_FMT, __entry->drm_id,
> > - __entry->mixer, 

[Freedreno] [PATCH 3/3] drm/msm: dpu: Don't store/deref pointers in trace ringbuffer

2018-09-19 Thread Sean Paul
From: Sean Paul 

TP_printk is not synchronous, so storing pointers and then later
derferencing them is a Bad Idea. This patch stores everything locally to
avoid display stomped memory.

Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 98 +--
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index ae6b0c51ba52..e12c4cefb742 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -686,37 +686,41 @@ TRACE_EVENT(dpu_crtc_setup_mixer,
TP_STRUCT__entry(
__field(uint32_t,   crtc_id )
__field(uint32_t,   plane_id)
-   __field(struct drm_plane_state*,state   )
-   __field(struct dpu_plane_state*,pstate  )
+   __field(uint32_t,   fb_id   )
+   __field_struct( struct drm_rect,src_rect)
+   __field_struct( struct drm_rect,dst_rect)
__field(uint32_t,   stage_idx   )
+   __field(enum dpu_stage, stage   )
__field(enum dpu_sspp,  sspp)
+   __field(uint32_t,   multirect_idx   )
+   __field(uint32_t,   multirect_mode  )
__field(uint32_t,   pixel_format)
__field(uint64_t,   modifier)
),
TP_fast_assign(
__entry->crtc_id = crtc_id;
__entry->plane_id = plane_id;
-   __entry->state = state;
-   __entry->pstate = pstate;
+   __entry->fb_id = state ? state->fb->base.id : 0;
+   __entry->src_rect = drm_plane_state_src(state);
+   __entry->dst_rect = drm_plane_state_dest(state);
__entry->stage_idx = stage_idx;
+   __entry->stage = pstate->stage;
__entry->sspp = sspp;
+   __entry->multirect_idx = pstate->multirect_index;
+   __entry->multirect_mode = pstate->multirect_mode;
__entry->pixel_format = pixel_format;
__entry->modifier = modifier;
),
-   TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:{%ux%u+%ux%u} "
- "dst:{%ux%u+%ux%u} stage_idx:%u stage:%d, sspp:%d "
+   TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:" DRM_RECT_FP_FMT
+ " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, sspp:%d "
  "multirect_index:%d multirect_mode:%u pix_format:%u "
  "modifier:%llu",
- __entry->crtc_id, __entry->plane_id,
- __entry->state->fb ? __entry->state->fb->base.id : -1,
- __entry->state->src_w >> 16,  __entry->state->src_h >> 16,
- __entry->state->src_x >> 16,  __entry->state->src_y >> 16,
- __entry->state->crtc_w,  __entry->state->crtc_h,
- __entry->state->crtc_x,  __entry->state->crtc_y,
- __entry->stage_idx, __entry->pstate->stage, __entry->sspp,
- __entry->pstate->multirect_index,
- __entry->pstate->multirect_mode, __entry->pixel_format,
- __entry->modifier)
+ __entry->crtc_id, __entry->plane_id, __entry->fb_id,
+ DRM_RECT_FP_ARG(&__entry->src_rect),
+ DRM_RECT_ARG(&__entry->dst_rect),
+ __entry->stage_idx, __entry->stage, __entry->sspp,
+ __entry->multirect_idx, __entry->multirect_mode,
+ __entry->pixel_format, __entry->modifier)
 );
 
 TRACE_EVENT(dpu_crtc_setup_lm_bounds,
@@ -725,15 +729,15 @@ TRACE_EVENT(dpu_crtc_setup_lm_bounds,
TP_STRUCT__entry(
__field(uint32_t,   drm_id  )
__field(int,mixer   )
-   __field(struct drm_rect *,  bounds  )
+   __field_struct( struct drm_rect,bounds  )
),
TP_fast_assign(
__entry->drm_id = drm_id;
__entry->mixer = mixer;
-   __entry->bounds = bounds;
+   __entry->bounds = *bounds;
),
TP_printk("id:%u mixer:%d bounds:" DRM_RECT_FMT, __entry->drm_id,
- __entry->mixer, DRM_RECT_ARG(__entry->bounds))
+ __entry->mixer, DRM_RECT_ARG(&__entry->bounds))
 );
 
 TRACE_EVENT(dpu_crtc_vblank_enable,
@@ -744,21 +748,25 @@ TRACE_EVENT(dpu_crtc_vblank_enable,
__field(uint32_t,   drm_id  )
__field(uint32_t,   enc_id  )
__field(bool,