Re: [PATCH 04/10] drm/msm/gpu: Convert the GPU show function to use the GPU state

[Sharat Masetty <smasetty-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 4/18/2018 4:14 AM, Jordan Crouse wrote:
> Convert the existing GPU show function to use the GPU state to
> dump the information rather than reading it directly from the hardware.
> This will require an additional step to capture the state before
> dumping it for the existing nodes but it will greatly facilitate reusing
> the same code for dumping a previously captured state from a GPU hang.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>   drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 11 +--
>   drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 12 +---
>   drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 18 +----
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 30 ++++----
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h |  4 +-
>   drivers/gpu/drm/msm/msm_debugfs.c       | 92 ++++++++++++++++++++++---
>   drivers/gpu/drm/msm/msm_gpu.h           |  3 +-
>   7 files changed, 104 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index b707b5bca9ab..4cffec2b6adc 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -411,15 +411,6 @@ static const unsigned int a3xx_registers[] = {
>   	~0   /* sentinel */
>   };
>   
> -#ifdef CONFIG_DEBUG_FS
> -static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
> -{
> -	seq_printf(m, "status:   %08x\n",
> -			gpu_read(gpu, REG_A3XX_RBBM_STATUS));
> -	adreno_show(gpu, m);
> -}
> -#endif
> -
>   /* would be nice to not have to duplicate the _show() stuff with printk(): */
>   static void a3xx_dump(struct msm_gpu *gpu)
>   {
> @@ -464,7 +455,7 @@ static const struct adreno_gpu_funcs funcs = {
>   		.irq = a3xx_irq,
>   		.destroy = a3xx_destroy,
>   #ifdef CONFIG_DEBUG_FS
> -		.show = a3xx_show,
> +		.show = adreno_show,
>   #endif
>   		.gpu_state_get = a3xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index 17e97ebc1077..95f08c22e8d7 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -455,16 +455,6 @@ static const unsigned int a4xx_registers[] = {
>   	~0 /* sentinel */
>   };
>   
> -#ifdef CONFIG_DEBUG_FS
> -static void a4xx_show(struct msm_gpu *gpu, struct seq_file *m)
> -{
> -	seq_printf(m, "status:   %08x\n",
> -			gpu_read(gpu, REG_A4XX_RBBM_STATUS));
> -	adreno_show(gpu, m);
> -
> -}
> -#endif
> -
>   static struct msm_gpu_state *a4xx_gpu_state_get(struct msm_gpu *gpu)
>   {
>   	struct msm_gpu_state *state = adreno_gpu_state_get(gpu);
> @@ -551,7 +541,7 @@ static const struct adreno_gpu_funcs funcs = {
>   		.irq = a4xx_irq,
>   		.destroy = a4xx_destroy,
>   #ifdef CONFIG_DEBUG_FS
> -		.show = a4xx_show,
> +		.show = adreno_show,
>   #endif
>   		.gpu_state_get = a4xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 08f25798abdb..b0910bbe5190 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1215,22 +1215,6 @@ static struct msm_gpu_state *a5xx_gpu_state_get(struct msm_gpu *gpu)
>   	return state;
>   }
>   
> -#ifdef CONFIG_DEBUG_FS
> -static void a5xx_show(struct msm_gpu *gpu, struct seq_file *m)
> -{
> -	seq_printf(m, "status:   %08x\n",
> -			gpu_read(gpu, REG_A5XX_RBBM_STATUS));
> -
> -	/*
> -	 * Temporarily disable hardware clock gating before going into
> -	 * adreno_show to avoid issues while reading the registers
> -	 */
> -	a5xx_set_hwcg(gpu, false);
> -	adreno_show(gpu, m);
> -	a5xx_set_hwcg(gpu, true);
> -}
> -#endif
> -
>   static struct msm_ringbuffer *a5xx_active_ring(struct msm_gpu *gpu)
>   {
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> @@ -1260,7 +1244,7 @@ static const struct adreno_gpu_funcs funcs = {
>   		.irq = a5xx_irq,
>   		.destroy = a5xx_destroy,
>   #ifdef CONFIG_DEBUG_FS
> -		.show = a5xx_show,
> +		.show = adreno_show,
>   		.debugfs_init = a5xx_debugfs_init,
>   #endif
>   		.gpu_busy = a5xx_gpu_busy,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index b2ccaf25767c..522d47ac36e1 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -423,38 +423,34 @@ void adreno_gpu_state_put(struct msm_gpu_state *state)
>   }
>   
>   #ifdef CONFIG_DEBUG_FS
> -void adreno_show(struct msm_gpu *gpu, struct seq_file *m)
> +void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> +		struct seq_file *m)
>   {
>   	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	int i;
>   
> +	if (IS_ERR_OR_NULL(state))
> +		return;
> +
> +	seq_printf(m, "status:   %08x\n", state->rbbm_status);
>   	seq_printf(m, "revision: %d (%d.%d.%d.%d)\n",
>   			adreno_gpu->info->revn, adreno_gpu->rev.core,
>   			adreno_gpu->rev.major, adreno_gpu->rev.minor,
>   			adreno_gpu->rev.patchid);
>   
>   	for (i = 0; i < gpu->nr_rings; i++) {
> -		struct msm_ringbuffer *ring = gpu->rb[i];
> -
>   		seq_printf(m, "rb %d: fence:    %d/%d\n", i,
> -			ring->memptrs->fence, ring->seqno);
> +			state->ring[i].fence, state->ring[i].seqno);
>   
> -		seq_printf(m, "      rptr:     %d\n",
> -			get_rptr(adreno_gpu, ring));
> -		seq_printf(m, "rb wptr:  %d\n", get_wptr(ring));
> +		seq_printf(m, "      rptr:     %d\n", state->ring[i].rptr);
> +		seq_printf(m, "rb wptr:  %d\n", state->ring[i].wptr);
>   	}
>   
> -	/* dump these out in a form that can be parsed by demsm: */
>   	seq_printf(m, "IO:region %s 00000000 00020000\n", gpu->name);
> -	for (i = 0; adreno_gpu->registers[i] != ~0; i += 2) {
> -		uint32_t start = adreno_gpu->registers[i];
> -		uint32_t end   = adreno_gpu->registers[i+1];
> -		uint32_t addr;
> -
> -		for (addr = start; addr <= end; addr++) {
> -			uint32_t val = gpu_read(gpu, addr);
> -			seq_printf(m, "IO:R %08x %08x\n", addr<<2, val);
> -		}
> +	for (i = 0; i < state->nr_registers; i++) {
> +		seq_printf(m, "IO:R %08x %08x\n",
> +			state->registers[i * 2] << 2,
> +			state->registers[(i * 2) + 1]);
>   	}
>   }
>   #endif
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 0beb078eb658..815ae98c7fd1 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -215,7 +215,8 @@ void adreno_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
>   void adreno_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring);
>   bool adreno_idle(struct msm_gpu *gpu, struct msm_ringbuffer *ring);
>   #ifdef CONFIG_DEBUG_FS
> -void adreno_show(struct msm_gpu *gpu, struct seq_file *m);
> +void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> +		struct seq_file *m);
>   #endif
>   void adreno_dump_info(struct msm_gpu *gpu);
>   void adreno_dump(struct msm_gpu *gpu);
> @@ -227,7 +228,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   		int nr_rings);
>   void adreno_gpu_cleanup(struct adreno_gpu *gpu);
>   
> -
>   struct msm_gpu_state *adreno_gpu_state_get(struct msm_gpu *gpu);
>   void adreno_gpu_state_put(struct msm_gpu_state *state);
>   
> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index ba74cb4f94df..ea20eb0ad747 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -16,26 +16,100 @@
>    */
>   
>   #ifdef CONFIG_DEBUG_FS
> +#include <linux/debugfs.h>
>   #include "msm_drv.h"
>   #include "msm_gpu.h"
>   #include "msm_kms.h"
>   #include "msm_debugfs.h"
>   
> -static int msm_gpu_show(struct drm_device *dev, struct seq_file *m)
> +struct msm_gpu_show_priv {
> +	struct msm_gpu_state *state;
> +	struct drm_device *dev;
> +};
> +
> +static int msm_gpu_show(struct seq_file *m, void *arg)
> +{
> +	struct msm_gpu_show_priv *show_priv = m->private;
> +	struct msm_drm_private *priv = show_priv->dev->dev_private;
> +	struct msm_gpu *gpu = priv->gpu;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> +	if (ret)
> +		return ret;
Why do we need a lock here? Shouldn't we just increase the ref count of 
the show_priv->state instead?
> +
> +	seq_printf(m, "%s Status:\n", gpu->name);
> +	gpu->funcs->show(gpu, show_priv->state, m);
> +
> +	mutex_unlock(&show_priv->dev->struct_mutex);
> +
> +	return 0;
> +}
> +
> +static int msm_gpu_release(struct inode *inode, struct file *file)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct msm_gpu_show_priv *show_priv = m->private;
> +	struct msm_drm_private *priv = show_priv->dev->dev_private;
> +	struct msm_gpu *gpu = priv->gpu;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&show_priv->dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	gpu->funcs->gpu_state_put(show_priv->state);
> +	mutex_unlock(&show_priv->dev->struct_mutex);
> +
> +	kfree(show_priv);
> +
> +	return single_release(inode, file);
> +}
> +
> +static int msm_gpu_open(struct inode *inode, struct file *file)
>   {
> +	struct drm_device *dev = inode->i_private;
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct msm_gpu *gpu = priv->gpu;
> +	struct msm_gpu_show_priv *show_priv;
> +	int ret;
>   
> -	if (gpu) {
> -		seq_printf(m, "%s Status:\n", gpu->name);
> -		pm_runtime_get_sync(&gpu->pdev->dev);
> -		gpu->funcs->show(gpu, m);
> -		pm_runtime_put_sync(&gpu->pdev->dev);
> +	if (!gpu)
> +		return -ENODEV;
> +
> +	show_priv = kmalloc(sizeof(*show_priv), GFP_KERNEL);
> +	if (!show_priv)
> +		return -ENOMEM;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_get_sync(&gpu->pdev->dev);
> +	show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +	pm_runtime_put_sync(&gpu->pdev->dev);
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	if (IS_ERR(show_priv->state)) {
> +		ret = PTR_ERR(show_priv->state);
> +		kfree(show_priv);
> +		return ret;
>   	}
>   
> -	return 0;
> +	show_priv->dev = dev;
> +
> +	return single_open(file, msm_gpu_show, show_priv);
>   }
>   
> +static const struct file_operations msm_gpu_fops = {
> +	.owner = THIS_MODULE,
> +	.open = msm_gpu_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = msm_gpu_release,
> +};
> +
>   static int msm_gem_show(struct drm_device *dev, struct seq_file *m)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
> @@ -105,7 +179,6 @@ static int show_locked(struct seq_file *m, void *arg)
>   }
>   
>   static struct drm_info_list msm_debugfs_list[] = {
> -		{"gpu", show_locked, 0, msm_gpu_show},
>   		{"gem", show_locked, 0, msm_gem_show},
>   		{ "mm", show_locked, 0, msm_mm_show },
>   		{ "fb", show_locked, 0, msm_fb_show },
> @@ -161,6 +234,9 @@ int msm_debugfs_init(struct drm_minor *minor)
>   		return ret;
>   	}
>   
> +	debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root,
> +		dev, &msm_gpu_fops);
> +
>   	if (priv->kms->funcs->debugfs_init) {
>   		ret = priv->kms->funcs->debugfs_init(priv->kms, minor);
>   		if (ret)
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 4be72a612bec..470f3bb5f834 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -65,7 +65,8 @@ struct msm_gpu_funcs {
>   	void (*destroy)(struct msm_gpu *gpu);
>   #ifdef CONFIG_DEBUG_FS
>   	/* show GPU status in debugfs: */
> -	void (*show)(struct msm_gpu *gpu, struct seq_file *m);
> +	void (*show)(struct msm_gpu *gpu, struct msm_gpu_state *state,
> +			struct seq_file *m);
>   	/* for generation specific debugfs: */
>   	int (*debugfs_init)(struct msm_gpu *gpu, struct drm_minor *minor);
>   #endif
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno