[PATCH 1/3] drm/msm: dpu: Clear frame_busy_mask bit after trace

[PATCH 1/3] drm/msm: dpu: Clear frame_busy_mask bit after trace

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

We're printing the frame_busy_mask in a trace, but after it's been
cleared. This, as it turns out, is pretty pointless.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index c2e8985b4c54..e56f29190121 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1361,9 +1361,9 @@ static void dpu_encoder_frame_done_callback(
 		/* One of the physical encoders has become idle */
 		for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 			if (dpu_enc->phys_encs[i] == ready_phys) {
-				clear_bit(i, dpu_enc->frame_busy_mask);
 				trace_dpu_enc_frame_done_cb(DRMID(drm_enc), i,
 						dpu_enc->frame_busy_mask[0]);
+				clear_bit(i, dpu_enc->frame_busy_mask);
 			}
 		}
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 2/3] drm/msm: dpu: Add extra_flush_bits to trigger_flush trace

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

It's useful to know which bits of the flush come from "extra_flush_bits"

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  3 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   | 14 +++++++++-----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e56f29190121..8f6880db5c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1444,7 +1444,8 @@ static inline void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
 		ret = ctl->ops.get_pending_flush(ctl);
 
 	trace_dpu_enc_trigger_flush(DRMID(drm_enc), phys->intf_idx,
-				    pending_kickoff_cnt, ctl->idx, ret);
+				    pending_kickoff_cnt, ctl->idx,
+				    extra_flush_bits, ret);
 }
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 0be51db02f2e..ae6b0c51ba52 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -468,14 +468,16 @@ TRACE_EVENT(dpu_enc_frame_done_cb,
 
 TRACE_EVENT(dpu_enc_trigger_flush,
 	TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
-		 int pending_kickoff_cnt, int ctl_idx, u32 pending_flush_ret),
+		 int pending_kickoff_cnt, int ctl_idx, u32 extra_flush_bits,
+		 u32 pending_flush_ret),
 	TP_ARGS(drm_id, intf_idx, pending_kickoff_cnt, ctl_idx,
-		pending_flush_ret),
+		extra_flush_bits, pending_flush_ret),
 	TP_STRUCT__entry(
 		__field(	uint32_t,	drm_id			)
 		__field(	enum dpu_intf,	intf_idx		)
 		__field(	int,		pending_kickoff_cnt	)
 		__field(	int,		ctl_idx			)
+		__field(	u32,		extra_flush_bits	)
 		__field(	u32,		pending_flush_ret	)
 	),
 	TP_fast_assign(
@@ -483,12 +485,14 @@ TRACE_EVENT(dpu_enc_trigger_flush,
 		__entry->intf_idx = intf_idx;
 		__entry->pending_kickoff_cnt = pending_kickoff_cnt;
 		__entry->ctl_idx = ctl_idx;
+		__entry->extra_flush_bits = extra_flush_bits;
 		__entry->pending_flush_ret = pending_flush_ret;
 	),
 	TP_printk("id=%u, intf_idx=%d, pending_kickoff_cnt=%d ctl_idx=%d "
-		  "pending_flush_ret=%u", __entry->drm_id,
-		  __entry->intf_idx, __entry->pending_kickoff_cnt,
-		  __entry->ctl_idx, __entry->pending_flush_ret)
+		  "extra_flush_bits=0x%x pending_flush_ret=0x%x",
+		  __entry->drm_id, __entry->intf_idx,
+		  __entry->pending_kickoff_cnt, __entry->ctl_idx,
+		  __entry->extra_flush_bits, __entry->pending_flush_ret)
 );
 
 DECLARE_EVENT_CLASS(dpu_enc_ktime_template,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

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 <seanpaul@chromium.org>
---
 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,			enable	)
-		__field(	struct dpu_crtc *,	crtc	)
+		__field(	bool,			enabled )
+		__field(	bool,			suspend )
+		__field(	bool,			vblank_requested )
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
 		__entry->enc_id = enc_id;
 		__entry->enable = enable;
-		__entry->crtc = crtc;
+		__entry->enabled = crtc->enabled;
+		__entry->suspend = crtc->suspend;
+		__entry->vblank_requested = crtc->vblank_requested;
 	),
 	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s suspend:%s "
 		  "vblank_req:%s}",
 		  __entry->drm_id, __entry->enc_id,
 		  __entry->enable ? "true" : "false",
-		  __entry->crtc->enabled ? "true" : "false",
-		  __entry->crtc->suspend ? "true" : "false",
-		  __entry->crtc->vblank_requested ? "true" : "false")
+		  __entry->enabled ? "true" : "false",
+		  __entry->suspend ? "true" : "false",
+		  __entry->vblank_requested ? "true" : "false")
 );
 
 DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
@@ -767,18 +775,22 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
 	TP_STRUCT__entry(
 		__field(	uint32_t,		drm_id	)
 		__field(	bool,			enable	)
-		__field(	struct dpu_crtc *,	crtc	)
+		__field(	bool,			enabled )
+		__field(	bool,			suspend )
+		__field(	bool,			vblank_requested )
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
 		__entry->enable = enable;
-		__entry->crtc = crtc;
+		__entry->enabled = crtc->enabled;
+		__entry->suspend = crtc->suspend;
+		__entry->vblank_requested = crtc->vblank_requested;
 	),
 	TP_printk("id:%u enable:%s state{enabled:%s suspend:%s vblank_req:%s}",
 		  __entry->drm_id, __entry->enable ? "true" : "false",
-		  __entry->crtc->enabled ? "true" : "false",
-		  __entry->crtc->suspend ? "true" : "false",
-		  __entry->crtc->vblank_requested ? "true" : "false")
+		  __entry->enabled ? "true" : "false",
+		  __entry->suspend ? "true" : "false",
+		  __entry->vblank_requested ? "true" : "false")
 );
 DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_set_suspend,
 	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
@@ -818,24 +830,24 @@ TRACE_EVENT(dpu_plane_set_scanout,
 	TP_ARGS(index, layout, multirect_index),
 	TP_STRUCT__entry(
 		__field(	enum dpu_sspp,			index	)
-		__field(	struct dpu_hw_fmt_layout*,	layout	)
+		__field_struct(	struct dpu_hw_fmt_layout,	layout	)
 		__field(	enum dpu_sspp_multirect_index,	multirect_index)
 	),
 	TP_fast_assign(
 		__entry->index = index;
-		__entry->layout = layout;
+		__entry->layout = *layout;
 		__entry->multirect_index = multirect_index;
 	),
 	TP_printk("index:%d layout:{%ux%u @ [%u/%u, %u/%u, %u/%u, %u/%u]} "
-		  "multirect_index:%d", __entry->index, __entry->layout->width,
-		  __entry->layout->height, __entry->layout->plane_addr[0],
-		  __entry->layout->plane_size[0],
-		  __entry->layout->plane_addr[1],
-		  __entry->layout->plane_size[1],
-		  __entry->layout->plane_addr[2],
-		  __entry->layout->plane_size[2],
-		  __entry->layout->plane_addr[3],
-		  __entry->layout->plane_size[3], __entry->multirect_index)
+		  "multirect_index:%d", __entry->index, __entry->layout.width,
+		  __entry->layout.height, __entry->layout.plane_addr[0],
+		  __entry->layout.plane_size[0],
+		  __entry->layout.plane_addr[1],
+		  __entry->layout.plane_size[1],
+		  __entry->layout.plane_addr[2],
+		  __entry->layout.plane_size[2],
+		  __entry->layout.plane_addr[3],
+		  __entry->layout.plane_size[3], __entry->multirect_index)
 );
 
 TRACE_EVENT(dpu_plane_disable,
@@ -979,16 +991,16 @@ TRACE_EVENT(dpu_core_perf_update_clk,
 	TP_PROTO(struct drm_device *dev, bool stop_req, u64 clk_rate),
 	TP_ARGS(dev, stop_req, clk_rate),
 	TP_STRUCT__entry(
-		__field(	struct drm_device *,	dev		)
+		__string(	dev_name,		dev->unique	)
 		__field(	bool,			stop_req	)
 		__field(	u64,			clk_rate	)
 	),
 	TP_fast_assign(
-		__entry->dev = dev;
+		__assign_str(dev_name, dev->unique);
 		__entry->stop_req = stop_req;
 		__entry->clk_rate = clk_rate;
 	),
-	TP_printk("dev:%s stop_req:%s clk_rate:%llu", __entry->dev->unique,
+	TP_printk("dev:%s stop_req:%s clk_rate:%llu", __get_str(dev_name),
 		  __entry->stop_req ? "true" : "false", __entry->clk_rate)
 );
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 1/3] drm/msm: dpu: Clear frame_busy_mask bit after trace

[Abhinav Kumar]

On 2018-09-19 11:33, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> We're printing the frame_busy_mask in a trace, but after it's been
> cleared. This, as it turns out, is pretty pointless.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index c2e8985b4c54..e56f29190121 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1361,9 +1361,9 @@ static void dpu_encoder_frame_done_callback(
>  		/* One of the physical encoders has become idle */
>  		for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>  			if (dpu_enc->phys_encs[i] == ready_phys) {
> -				clear_bit(i, dpu_enc->frame_busy_mask);
>  				trace_dpu_enc_frame_done_cb(DRMID(drm_enc), i,
>  						dpu_enc->frame_busy_mask[0]);
> +				clear_bit(i, dpu_enc->frame_busy_mask);
>  			}
>  		}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/msm: dpu: Add extra_flush_bits to trigger_flush trace

[Abhinav Kumar]

On 2018-09-19 11:33, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> It's useful to know which bits of the flush come from 
> "extra_flush_bits"
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  3 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   | 14 +++++++++-----
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index e56f29190121..8f6880db5c99 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1444,7 +1444,8 @@ static inline void
> _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
>  		ret = ctl->ops.get_pending_flush(ctl);
> 
>  	trace_dpu_enc_trigger_flush(DRMID(drm_enc), phys->intf_idx,
> -				    pending_kickoff_cnt, ctl->idx, ret);
> +				    pending_kickoff_cnt, ctl->idx,
> +				    extra_flush_bits, ret);
>  }
> 
>  /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 0be51db02f2e..ae6b0c51ba52 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -468,14 +468,16 @@ TRACE_EVENT(dpu_enc_frame_done_cb,
> 
>  TRACE_EVENT(dpu_enc_trigger_flush,
>  	TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
> -		 int pending_kickoff_cnt, int ctl_idx, u32 pending_flush_ret),
> +		 int pending_kickoff_cnt, int ctl_idx, u32 extra_flush_bits,
> +		 u32 pending_flush_ret),
>  	TP_ARGS(drm_id, intf_idx, pending_kickoff_cnt, ctl_idx,
> -		pending_flush_ret),
> +		extra_flush_bits, pending_flush_ret),
>  	TP_STRUCT__entry(
>  		__field(	uint32_t,	drm_id			)
>  		__field(	enum dpu_intf,	intf_idx		)
>  		__field(	int,		pending_kickoff_cnt	)
>  		__field(	int,		ctl_idx			)
> +		__field(	u32,		extra_flush_bits	)
>  		__field(	u32,		pending_flush_ret	)
>  	),
>  	TP_fast_assign(
> @@ -483,12 +485,14 @@ TRACE_EVENT(dpu_enc_trigger_flush,
>  		__entry->intf_idx = intf_idx;
>  		__entry->pending_kickoff_cnt = pending_kickoff_cnt;
>  		__entry->ctl_idx = ctl_idx;
> +		__entry->extra_flush_bits = extra_flush_bits;
>  		__entry->pending_flush_ret = pending_flush_ret;
>  	),
>  	TP_printk("id=%u, intf_idx=%d, pending_kickoff_cnt=%d ctl_idx=%d "
> -		  "pending_flush_ret=%u", __entry->drm_id,
> -		  __entry->intf_idx, __entry->pending_kickoff_cnt,
> -		  __entry->ctl_idx, __entry->pending_flush_ret)
> +		  "extra_flush_bits=0x%x pending_flush_ret=0x%x",
> +		  __entry->drm_id, __entry->intf_idx,
> +		  __entry->pending_kickoff_cnt, __entry->ctl_idx,
> +		  __entry->extra_flush_bits, __entry->pending_flush_ret)
>  );
> 
>  DECLARE_EVENT_CLASS(dpu_enc_ktime_template,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Abhinav Kumar]

On 2018-09-19 11:33, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> 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 <seanpaul@chromium.org>
After fixing the minor nit please add,
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  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,			enable	)
> -		__field(	struct dpu_crtc *,	crtc	)
> +		__field(	bool,			enabled )
> +		__field(	bool,			suspend )
> +		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
>  		__entry->drm_id = drm_id;
>  		__entry->enc_id = enc_id;
>  		__entry->enable = enable;
> -		__entry->crtc = crtc;
> +		__entry->enabled = crtc->enabled;
> +		__entry->suspend = crtc->suspend;
> +		__entry->vblank_requested = crtc->vblank_requested;
>  	),
>  	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s suspend:%s "
>  		  "vblank_req:%s}",
>  		  __entry->drm_id, __entry->enc_id,
>  		  __entry->enable ? "true" : "false",
> -		  __entry->crtc->enabled ? "true" : "false",
> -		  __entry->crtc->suspend ? "true" : "false",
> -		  __entry->crtc->vblank_requested ? "true" : "false")
> +		  __entry->enabled ? "true" : "false",
> +		  __entry->suspend ? "true" : "false",
> +		  __entry->vblank_requested ? "true" : "false")
>  );
> 
>  DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
> @@ -767,18 +775,22 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
>  	TP_STRUCT__entry(
>  		__field(	uint32_t,		drm_id	)
>  		__field(	bool,			enable	)
> -		__field(	struct dpu_crtc *,	crtc	)
> +		__field(	bool,			enabled )
> +		__field(	bool,			suspend )
> +		__field(	bool,			vblank_requested )
>  	),
>  	TP_fast_assign(
>  		__entry->drm_id = drm_id;
>  		__entry->enable = enable;
> -		__entry->crtc = crtc;
> +		__entry->enabled = crtc->enabled;
> +		__entry->suspend = crtc->suspend;
> +		__entry->vblank_requested = crtc->vblank_requested;
>  	),
>  	TP_printk("id:%u enable:%s state{enabled:%s suspend:%s 
> vblank_req:%s}",
>  		  __entry->drm_id, __entry->enable ? "true" : "false",
> -		  __entry->crtc->enabled ? "true" : "false",
> -		  __entry->crtc->suspend ? "true" : "false",
> -		  __entry->crtc->vblank_requested ? "true" : "false")
> +		  __entry->enabled ? "true" : "false",
> +		  __entry->suspend ? "true" : "false",
> +		  __entry->vblank_requested ? "true" : "false")
>  );
>  DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_set_suspend,
>  	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
> @@ -818,24 +830,24 @@ TRACE_EVENT(dpu_plane_set_scanout,
>  	TP_ARGS(index, layout, multirect_index),
>  	TP_STRUCT__entry(
>  		__field(	enum dpu_sspp,			index	)
> -		__field(	struct dpu_hw_fmt_layout*,	layout	)
> +		__field_struct(	struct dpu_hw_fmt_layout,	layout	)
>  		__field(	enum dpu_sspp_multirect_index,	multirect_index)
>  	),
>  	TP_fast_assign(
>  		__entry->index = index;
> -		__entry->layout = layout;
> +		__entry->layout = *layout;
>  		__entry->multirect_index = multirect_index;
>  	),
>  	TP_printk("index:%d layout:{%ux%u @ [%u/%u, %u/%u, %u/%u, %u/%u]} "
> -		  "multirect_index:%d", __entry->index, __entry->layout->width,
> -		  __entry->layout->height, __entry->layout->plane_addr[0],
> -		  __entry->layout->plane_size[0],
> -		  __entry->layout->plane_addr[1],
> -		  __entry->layout->plane_size[1],
> -		  __entry->layout->plane_addr[2],
> -		  __entry->layout->plane_size[2],
> -		  __entry->layout->plane_addr[3],
> -		  __entry->layout->plane_size[3], __entry->multirect_index)
> +		  "multirect_index:%d", __entry->index, __entry->layout.width,
> +		  __entry->layout.height, __entry->layout.plane_addr[0],
> +		  __entry->layout.plane_size[0],
> +		  __entry->layout.plane_addr[1],
> +		  __entry->layout.plane_size[1],
> +		  __entry->layout.plane_addr[2],
> +		  __entry->layout.plane_size[2],
> +		  __entry->layout.plane_addr[3],
> +		  __entry->layout.plane_size[3], __entry->multirect_index)
>  );
> 
>  TRACE_EVENT(dpu_plane_disable,
> @@ -979,16 +991,16 @@ TRACE_EVENT(dpu_core_perf_update_clk,
>  	TP_PROTO(struct drm_device *dev, bool stop_req, u64 clk_rate),
>  	TP_ARGS(dev, stop_req, clk_rate),
>  	TP_STRUCT__entry(
> -		__field(	struct drm_device *,	dev		)
> +		__string(	dev_name,		dev->unique	)
>  		__field(	bool,			stop_req	)
>  		__field(	u64,			clk_rate	)
>  	),
>  	TP_fast_assign(
> -		__entry->dev = dev;
> +		__assign_str(dev_name, dev->unique);
>  		__entry->stop_req = stop_req;
>  		__entry->clk_rate = clk_rate;
>  	),
> -	TP_printk("dev:%s stop_req:%s clk_rate:%llu", __entry->dev->unique,
> +	TP_printk("dev:%s stop_req:%s clk_rate:%llu", __get_str(dev_name),
>  		  __entry->stop_req ? "true" : "false", __entry->clk_rate)
>  );
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[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 <seanpaul@chromium.org>
> > 
> > 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 <seanpaul@chromium.org>
> After fixing the minor nit please add,
> Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>

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, 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,			enable	)
> > -		__field(	struct dpu_crtc *,	crtc	)
> > +		__field(	bool,			enabled )
> > +		__field(	bool,			suspend )
> > +		__field(	bool,			vblank_requested )
> >  	),
> >  	TP_fast_assign(
> >  		__entry->drm_id = drm_id;
> >  		__entry->enc_id = enc_id;
> >  		__entry->enable = enable;
> > -		__entry->crtc = crtc;
> > +		__entry->enabled = crtc->enabled;
> > +		__entry->suspend = crtc->suspend;
> > +		__entry->vblank_requested = crtc->vblank_requested;
> >  	),
> >  	TP_printk("id:%u encoder:%u enable:%s state{enabled:%s suspend:%s "
> >  		  "vblank_req:%s}",
> >  		  __entry->drm_id, __entry->enc_id,
> >  		  __entry->enable ? "true" : "false",
> > -		  __entry->crtc->enabled ? "true" : "false",
> > -		  __entry->crtc->suspend ? "true" : "false",
> > -		  __entry->crtc->vblank_requested ? "true" : "false")
> > +		  __entry->enabled ? "true" : "false",
> > +		  __entry->suspend ? "true" : "false",
> > +		  __entry->vblank_requested ? "true" : "false")
> >  );
> > 
> >  DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
> > @@ -767,18 +775,22 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template,
> >  	TP_STRUCT__entry(
> >  		__field(	uint32_t,		drm_id	)
> >  		__field(	bool,			enable	)
> > -		__field(	struct dpu_crtc *,	crtc	)
> > +		__field(	bool,			enabled )
> > +		__field(	bool,			suspend )
> > +		__field(	bool,			vblank_requested )
> >  	),
> >  	TP_fast_assign(
> >  		__entry->drm_id = drm_id;
> >  		__entry->enable = enable;
> > -		__entry->crtc = crtc;
> > +		__entry->enabled = crtc->enabled;
> > +		__entry->suspend = crtc->suspend;
> > +		__entry->vblank_requested = crtc->vblank_requested;
> >  	),
> >  	TP_printk("id:%u enable:%s state{enabled:%s suspend:%s
> > vblank_req:%s}",
> >  		  __entry->drm_id, __entry->enable ? "true" : "false",
> > -		  __entry->crtc->enabled ? "true" : "false",
> > -		  __entry->crtc->suspend ? "true" : "false",
> > -		  __entry->crtc->vblank_requested ? "true" : "false")
> > +		  __entry->enabled ? "true" : "false",
> > +		  __entry->suspend ? "true" : "false",
> > +		  __entry->vblank_requested ? "true" : "false")
> >  );
> >  DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_set_suspend,
> >  	TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),
> > @@ -818,24 +830,24 @@ TRACE_EVENT(dpu_plane_set_scanout,
> >  	TP_ARGS(index, layout, multirect_index),
> >  	TP_STRUCT__entry(
> >  		__field(	enum dpu_sspp,			index	)
> > -		__field(	struct dpu_hw_fmt_layout*,	layout	)
> > +		__field_struct(	struct dpu_hw_fmt_layout,	layout	)
> >  		__field(	enum dpu_sspp_multirect_index,	multirect_index)
> >  	),
> >  	TP_fast_assign(
> >  		__entry->index = index;
> > -		__entry->layout = layout;
> > +		__entry->layout = *layout;
> >  		__entry->multirect_index = multirect_index;
> >  	),
> >  	TP_printk("index:%d layout:{%ux%u @ [%u/%u, %u/%u, %u/%u, %u/%u]} "
> > -		  "multirect_index:%d", __entry->index, __entry->layout->width,
> > -		  __entry->layout->height, __entry->layout->plane_addr[0],
> > -		  __entry->layout->plane_size[0],
> > -		  __entry->layout->plane_addr[1],
> > -		  __entry->layout->plane_size[1],
> > -		  __entry->layout->plane_addr[2],
> > -		  __entry->layout->plane_size[2],
> > -		  __entry->layout->plane_addr[3],
> > -		  __entry->layout->plane_size[3], __entry->multirect_index)
> > +		  "multirect_index:%d", __entry->index, __entry->layout.width,
> > +		  __entry->layout.height, __entry->layout.plane_addr[0],
> > +		  __entry->layout.plane_size[0],
> > +		  __entry->layout.plane_addr[1],
> > +		  __entry->layout.plane_size[1],
> > +		  __entry->layout.plane_addr[2],
> > +		  __entry->layout.plane_size[2],
> > +		  __entry->layout.plane_addr[3],
> > +		  __entry->layout.plane_size[3], __entry->multirect_index)
> >  );
> > 
> >  TRACE_EVENT(dpu_plane_disable,
> > @@ -979,16 +991,16 @@ TRACE_EVENT(dpu_core_perf_update_clk,
> >  	TP_PROTO(struct drm_device *dev, bool stop_req, u64 clk_rate),
> >  	TP_ARGS(dev, stop_req, clk_rate),
> >  	TP_STRUCT__entry(
> > -		__field(	struct drm_device *,	dev		)
> > +		__string(	dev_name,		dev->unique	)
> >  		__field(	bool,			stop_req	)
> >  		__field(	u64,			clk_rate	)
> >  	),
> >  	TP_fast_assign(
> > -		__entry->dev = dev;
> > +		__assign_str(dev_name, dev->unique);
> >  		__entry->stop_req = stop_req;
> >  		__entry->clk_rate = clk_rate;
> >  	),
> > -	TP_printk("dev:%s stop_req:%s clk_rate:%llu", __entry->dev->unique,
> > +	TP_printk("dev:%s stop_req:%s clk_rate:%llu", __get_str(dev_name),
> >  		  __entry->stop_req ? "true" : "false", __entry->clk_rate)
> >  );

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno