Re: [Freedreno] [PATCH] drm/msm: devcoredump should dump MSM_SUBMIT_BO_DUMP buffers

[Jordan Crouse]

On Tue, Feb 18, 2020 at 01:00:21PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
> 
> Also log buffers with the DUMP flag set, to ensure we capture all useful
> cmdstream in crashdump state with modern mesa.
> 
> Otherwise we miss out on the contents of "state object" cmdstream
> buffers.

One nit, but with that:

Reviewed-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>

> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/msm_gem.h | 10 ++++++++++
>  drivers/gpu/drm/msm/msm_gpu.c | 28 +++++++++++++++++++++++-----
>  drivers/gpu/drm/msm/msm_rd.c  |  8 +-------
>  3 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 9e0953c2b7ce..22b4ccd7bb28 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -160,4 +160,14 @@ struct msm_gem_submit {
>  	} bos[0];
>  };
>  
> +/* helper to determine of a buffer in submit should be dumped, used for both
> + * devcoredump and debugfs cmdstream dumping:
> + */
> +static bool

Static inline? Surprised you didn't get an unused warning or two.

> +should_dump(struct msm_gem_submit *submit, int idx)
> +{
> +	extern bool rd_full;
> +	return rd_full || (submit->bos[idx].flags & MSM_SUBMIT_BO_DUMP);
> +}
> +
>  #endif /* __MSM_GEM_H__ */
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 18f3a5c53ffb..615c5cda5389 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -355,16 +355,34 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
>  	state->cmd = kstrdup(cmd, GFP_KERNEL);
>  
>  	if (submit) {
> -		int i;
> -
> -		state->bos = kcalloc(submit->nr_cmds,
> +		int i, nr = 0;
> +
> +		/* count # of buffers to dump: */
> +		for (i = 0; i < submit->nr_bos; i++)
> +			if (should_dump(submit, i))
> +				nr++;
> +		/* always dump cmd bo's, but don't double count them: */
> +		for (i = 0; i < submit->nr_cmds; i++)
> +			if (!should_dump(submit, submit->cmd[i].idx))
> +				nr++;
> +
> +		state->bos = kcalloc(nr,
>  			sizeof(struct msm_gpu_state_bo), GFP_KERNEL);
>  
> +		for (i = 0; i < submit->nr_bos; i++) {
> +			if (should_dump(submit, i)) {
> +				msm_gpu_crashstate_get_bo(state, submit->bos[i].obj,
> +					submit->bos[i].iova, submit->bos[i].flags);
> +			}
> +		}
> +
>  		for (i = 0; state->bos && i < submit->nr_cmds; i++) {
>  			int idx = submit->cmd[i].idx;
>  
> -			msm_gpu_crashstate_get_bo(state, submit->bos[idx].obj,
> -				submit->bos[idx].iova, submit->bos[idx].flags);
> +			if (!should_dump(submit, submit->cmd[i].idx)) {
> +				msm_gpu_crashstate_get_bo(state, submit->bos[idx].obj,
> +					submit->bos[idx].iova, submit->bos[idx].flags);
> +			}
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
> index af7ceb246c7c..732f65df5c4f 100644
> --- a/drivers/gpu/drm/msm/msm_rd.c
> +++ b/drivers/gpu/drm/msm/msm_rd.c
> @@ -43,7 +43,7 @@
>  #include "msm_gpu.h"
>  #include "msm_gem.h"
>  
> -static bool rd_full = false;
> +bool rd_full = false;
>  MODULE_PARM_DESC(rd_full, "If true, $debugfs/.../rd will snapshot all buffer contents");
>  module_param_named(rd_full, rd_full, bool, 0600);
>  
> @@ -336,12 +336,6 @@ static void snapshot_buf(struct msm_rd_state *rd,
>  	msm_gem_put_vaddr(&obj->base);
>  }
>  
> -static bool
> -should_dump(struct msm_gem_submit *submit, int idx)
> -{
> -	return rd_full || (submit->bos[idx].flags & MSM_SUBMIT_BO_DUMP);
> -}
> -
>  /* called under struct_mutex */
>  void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit,
>  		const char *fmt, ...)
> -- 
> 2.24.1
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project