Re: [PATCH v2 3/4] drm/amd/display: Add pipe_state tracepoint

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/11, Kazlauskas, Nicholas wrote:
> On 2020-09-11 10:59 a.m., Rodrigo Siqueira wrote:
> > This commit introduces a trace mechanism for struct pipe_ctx by adding a
> > middle layer struct in the amdgpu_dm_trace.h for capturing the most
> > important data from struct pipe_ctx and showing its data via tracepoint.
> > This tracepoint was added to dc.c and dcn10_hw_sequencer, however, it
> > can be added to other DCN architecture.
> > 
> > Co-developed-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
> 
> 
> This patch, while very useful, unfortunately pulls in a lot of DM code into
> DC so I would prefer to hold off on this one for now.

Hi Nicholas, first of all, thanks for your feedback.

By "pulls in a lot of DM code into DC" do you mean all references to
plane_state and plane_res? Or the data inserted in the struct?
 
> It would be better if this had a proper DC interface for tracing/logging
> these states. If the API was more like how we handle tracing register
> reads/writes this would be cleaner.

Could you elaborate a little bit more about  "a proper DC interface"?
What is your view of this sort of interface?

Also, how about Patch 04? Same problems?

Best Regards
Rodrigo Siqueira

> Regards,
> Nicholas Kazlauskas
> 
> > ---
> >   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 172 ++++++++++++++++++
> >   drivers/gpu/drm/amd/display/dc/core/dc.c      |  11 ++
> >   .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  17 +-
> >   3 files changed, 195 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > index 5fb4c4a5c349..53f62506e17c 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > @@ -376,6 +376,178 @@ TRACE_EVENT(amdgpu_dm_atomic_check_finish,
> >   		      __entry->async_update, __entry->allow_modeset)
> >   );
> > +#ifndef _AMDGPU_DM_TRACE_STRUCTS_DEFINED_
> > +#define _AMDGPU_DM_TRACE_STRUCTS_DEFINED_
> > +
> > +struct amdgpu_dm_trace_pipe_state {
> > +	int pipe_idx;
> > +	const void *stream;
> > +	int stream_w;
> > +	int stream_h;
> > +	int dst_x;
> > +	int dst_y;
> > +	int dst_w;
> > +	int dst_h;
> > +	int src_x;
> > +	int src_y;
> > +	int src_w;
> > +	int src_h;
> > +	int clip_x;
> > +	int clip_y;
> > +	int clip_w;
> > +	int clip_h;
> > +	int recout_x;
> > +	int recout_y;
> > +	int recout_w;
> > +	int recout_h;
> > +	int viewport_x;
> > +	int viewport_y;
> > +	int viewport_w;
> > +	int viewport_h;
> > +	int flip_immediate;
> > +	int surface_pitch;
> > +	int format;
> > +	int swizzle;
> > +	unsigned int update_flags;
> > +};
> > +
> > +#define fill_out_trace_pipe_state(trace_pipe_state, pipe_ctx) \
> > +	do { \
> > +		trace_pipe_state.pipe_idx	= (pipe_ctx)->pipe_idx; \
> > +		trace_pipe_state.stream		= (pipe_ctx)->stream; \
> > +		trace_pipe_state.stream_w	= (pipe_ctx)->stream->timing.h_addressable; \
> > +		trace_pipe_state.stream_h	= (pipe_ctx)->stream->timing.v_addressable; \
> > +		trace_pipe_state.dst_x		= (pipe_ctx)->plane_state->dst_rect.x; \
> > +		trace_pipe_state.dst_y		= (pipe_ctx)->plane_state->dst_rect.y; \
> > +		trace_pipe_state.dst_w		= (pipe_ctx)->plane_state->dst_rect.width; \
> > +		trace_pipe_state.dst_h		= (pipe_ctx)->plane_state->dst_rect.height; \
> > +		trace_pipe_state.src_x		= (pipe_ctx)->plane_state->src_rect.x; \
> > +		trace_pipe_state.src_y		= (pipe_ctx)->plane_state->src_rect.y; \
> > +		trace_pipe_state.src_w		= (pipe_ctx)->plane_state->src_rect.width; \
> > +		trace_pipe_state.src_h		= (pipe_ctx)->plane_state->src_rect.height; \
> > +		trace_pipe_state.clip_x		= (pipe_ctx)->plane_state->clip_rect.x; \
> > +		trace_pipe_state.clip_y		= (pipe_ctx)->plane_state->clip_rect.y; \
> > +		trace_pipe_state.clip_w		= (pipe_ctx)->plane_state->clip_rect.width; \
> > +		trace_pipe_state.clip_h		= (pipe_ctx)->plane_state->clip_rect.height; \
> > +		trace_pipe_state.recout_x	= (pipe_ctx)->plane_res.scl_data.recout.x; \
> > +		trace_pipe_state.recout_y	= (pipe_ctx)->plane_res.scl_data.recout.y; \
> > +		trace_pipe_state.recout_w	= (pipe_ctx)->plane_res.scl_data.recout.width; \
> > +		trace_pipe_state.recout_h	= (pipe_ctx)->plane_res.scl_data.recout.height; \
> > +		trace_pipe_state.viewport_x	= (pipe_ctx)->plane_res.scl_data.viewport.x; \
> > +		trace_pipe_state.viewport_y	= (pipe_ctx)->plane_res.scl_data.viewport.y; \
> > +		trace_pipe_state.viewport_w	= (pipe_ctx)->plane_res.scl_data.viewport.width; \
> > +		trace_pipe_state.viewport_h	= (pipe_ctx)->plane_res.scl_data.viewport.height; \
> > +		trace_pipe_state.flip_immediate = (pipe_ctx)->plane_state->flip_immediate; \
> > +		trace_pipe_state.surface_pitch	= (pipe_ctx)->plane_state->plane_size.surface_pitch; \
> > +		trace_pipe_state.format		= (pipe_ctx)->plane_state->format; \
> > +		trace_pipe_state.swizzle	= (pipe_ctx)->plane_state->tiling_info.gfx9.swizzle; \
> > +		trace_pipe_state.update_flags	= (pipe_ctx)->update_flags.raw; \
> > +	} while (0)
> > +
> > +#endif /* _AMDGPU_DM_TRACE_STRUCTS_DEFINED_ */
> > +
> > +TRACE_EVENT(amdgpu_dm_dc_pipe_state,
> > +	    TP_PROTO(const struct amdgpu_dm_trace_pipe_state *pipe_state),
> > +	    TP_ARGS(pipe_state),
> > +	    TP_STRUCT__entry(
> > +			     __field(int, pipe_idx)
> > +			     __field(const void *, stream)
> > +			     __field(int, stream_w)
> > +			     __field(int, stream_h)
> > +			     __field(int, dst_x)
> > +			     __field(int, dst_y)
> > +			     __field(int, dst_w)
> > +			     __field(int, dst_h)
> > +			     __field(int, src_x)
> > +			     __field(int, src_y)
> > +			     __field(int, src_w)
> > +			     __field(int, src_h)
> > +			     __field(int, clip_x)
> > +			     __field(int, clip_y)
> > +			     __field(int, clip_w)
> > +			     __field(int, clip_h)
> > +			     __field(int, recout_x)
> > +			     __field(int, recout_y)
> > +			     __field(int, recout_w)
> > +			     __field(int, recout_h)
> > +			     __field(int, viewport_x)
> > +			     __field(int, viewport_y)
> > +			     __field(int, viewport_w)
> > +			     __field(int, viewport_h)
> > +			     __field(int, flip_immediate)
> > +			     __field(int, surface_pitch)
> > +			     __field(int, format)
> > +			     __field(int, swizzle)
> > +			     __field(unsigned int, update_flags)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		       __entry->pipe_idx = pipe_state->pipe_idx;
> > +		       __entry->stream = pipe_state->stream;
> > +		       __entry->stream_w = pipe_state->stream_w;
> > +		       __entry->stream_h = pipe_state->stream_h;
> > +		       __entry->dst_x = pipe_state->dst_x;
> > +		       __entry->dst_y = pipe_state->dst_y;
> > +		       __entry->dst_w = pipe_state->dst_w;
> > +		       __entry->dst_h = pipe_state->dst_h;
> > +		       __entry->src_x = pipe_state->src_x;
> > +		       __entry->src_y = pipe_state->src_y;
> > +		       __entry->src_w = pipe_state->src_w;
> > +		       __entry->src_h = pipe_state->src_h;
> > +		       __entry->clip_x = pipe_state->clip_x;
> > +		       __entry->clip_y = pipe_state->clip_y;
> > +		       __entry->clip_w = pipe_state->clip_w;
> > +		       __entry->clip_h = pipe_state->clip_h;
> > +		       __entry->recout_x = pipe_state->recout_x;
> > +		       __entry->recout_y = pipe_state->recout_y;
> > +		       __entry->recout_w = pipe_state->recout_w;
> > +		       __entry->recout_h = pipe_state->recout_h;
> > +		       __entry->viewport_x = pipe_state->viewport_x;
> > +		       __entry->viewport_y = pipe_state->viewport_y;
> > +		       __entry->viewport_w = pipe_state->viewport_w;
> > +		       __entry->viewport_h = pipe_state->viewport_h;
> > +		       __entry->flip_immediate = pipe_state->flip_immediate;
> > +		       __entry->surface_pitch = pipe_state->surface_pitch;
> > +		       __entry->format = pipe_state->format;
> > +		       __entry->swizzle = pipe_state->swizzle;
> > +		       __entry->update_flags = pipe_state->update_flags;
> > +	),
> > +	TP_printk("pipe_idx=%d stream=%p rct(%d,%d) dst=(%d,%d,%d,%d) "
> > +		  "src=(%d,%d,%d,%d) clip=(%d,%d,%d,%d) recout=(%d,%d,%d,%d) "
> > +		  "viewport=(%d,%d,%d,%d) flip_immediate=%d pitch=%d "
> > +		  "format=%d swizzle=%d update_flags=%x",
> > +		  __entry->pipe_idx,
> > +		  __entry->stream,
> > +		  __entry->stream_w,
> > +		  __entry->stream_h,
> > +		  __entry->dst_x,
> > +		  __entry->dst_y,
> > +		  __entry->dst_w,
> > +		  __entry->dst_h,
> > +		  __entry->src_x,
> > +		  __entry->src_y,
> > +		  __entry->src_w,
> > +		  __entry->src_h,
> > +		  __entry->clip_x,
> > +		  __entry->clip_y,
> > +		  __entry->clip_w,
> > +		  __entry->clip_h,
> > +		  __entry->recout_x,
> > +		  __entry->recout_y,
> > +		  __entry->recout_w,
> > +		  __entry->recout_h,
> > +		  __entry->viewport_x,
> > +		  __entry->viewport_y,
> > +		  __entry->viewport_w,
> > +		  __entry->viewport_h,
> > +		  __entry->flip_immediate,
> > +		  __entry->surface_pitch,
> > +		  __entry->format,
> > +		  __entry->swizzle,
> > +		  __entry->update_flags
> > +	)
> > +);
> > +
> >   #endif /* _AMDGPU_DM_TRACE_H_ */
> >   #undef TRACE_INCLUDE_PATH
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > index dc463d99ef50..0c9f177e5827 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > @@ -2644,6 +2644,17 @@ void dc_commit_updates_for_stream(struct dc *dc,
> >   		}
> >   	}
> > +	for (i = 0; i < MAX_PIPES; ++i) {
> > +		struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];
> > +
> > +		if (pipe_ctx->plane_state) {
> > +			struct amdgpu_dm_trace_pipe_state pipe_state_trace;
> > +
> > +			fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx);
> > +			trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace);
> > +		}
> > +	}
> > +
> >   	commit_planes_for_stream(
> >   				dc,
> >   				srf_updates,
> > diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> > index 8ca94f506195..464d0ad093b9 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> > @@ -1020,15 +1020,22 @@ static bool dcn10_hw_wa_force_recovery(struct dc *dc)
> >   }
> > -
> >   void dcn10_verify_allow_pstate_change_high(struct dc *dc)
> >   {
> > -	static bool should_log_hw_state; /* prevent hw state log by default */
> > -
> >   	if (!hubbub1_verify_allow_pstate_change_high(dc->res_pool->hubbub)) {
> > -		if (should_log_hw_state) {
> > -			dcn10_log_hw_state(dc, NULL);
> > +		int i;
> > +
> > +		for (i = 0; i < MAX_PIPES; ++i) {
> > +			struct pipe_ctx *pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];
> > +
> > +			if (pipe_ctx->plane_state) {
> > +				struct amdgpu_dm_trace_pipe_state pipe_state_trace;
> > +
> > +				fill_out_trace_pipe_state(pipe_state_trace, pipe_ctx);
> > +				trace_amdgpu_dm_dc_pipe_state(&pipe_state_trace);
> > +			}
> >   		}
> > +
> >   		BREAK_TO_DEBUGGER();
> >   		if (dcn10_hw_wa_force_recovery(dc)) {
> >   		/*check again*/
> > 
> 

-- 
Rodrigo Siqueira
https://siqueira.tech

Attachment: signature.asc
Description: PGP signature

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux