diff mbox

[08/10] drm/etnaviv: make it possible to reconfigure perf counter

Message ID 20161209112131.3924-9-christian.gmeiner@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Gmeiner Dec. 9, 2016, 11:21 a.m. UTC 
Each perf counter 'unit' consits of a multipler configuration register
and a register to read the selected value. Extend the uapi to handle
this case gracefully. Before the readback is done the mux (config_reg)
get reconfigured (vale).

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 7 +++++--
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 3 +++
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        | 3 +++
 include/uapi/drm/etnaviv_drm.h               | 6 +++++-
 4 files changed, 16 insertions(+), 3 deletions(-)

Comments

Wladimir J. van der Laan Dec. 9, 2016, 3:48 p.m. UTC | #1 
On Fri, Dec 09, 2016 at 12:21:29PM +0100, Christian Gmeiner wrote:
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1379,6 +1379,9 @@ static void etnaviv_process_readbacks(struct etnaviv_gpu *gpu,
>  		const u32 val = gpu_read(gpu, readback->reg);
>  		u32 *bo = readback->bo_vma;
>  
> +		if (readback->flags & ETNA_READBACK_PERF)
> +			gpu_write(gpu, readback->perf_reg, readback->perf_value);
> +
>  		*(bo + readback->offset) = val;
>  	}
>  }

This is the wrong order. First write the selection register, then read the
counter. Currently this causes the reported registers to be off by one,
if they're read in sequential order.

Wladimir
Christian Gmeiner Dec. 9, 2016, 5:56 p.m. UTC | #2 
Hi Wladimir,

2016-12-09 16:48 GMT+01:00 Wladimir J. van der Laan <laanwj@gmail.com>:
> On Fri, Dec 09, 2016 at 12:21:29PM +0100, Christian Gmeiner wrote:
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -1379,6 +1379,9 @@ static void etnaviv_process_readbacks(struct etnaviv_gpu *gpu,
>>               const u32 val = gpu_read(gpu, readback->reg);
>>               u32 *bo = readback->bo_vma;
>>
>> +             if (readback->flags & ETNA_READBACK_PERF)
>> +                     gpu_write(gpu, readback->perf_reg, readback->perf_value);
>> +
>>               *(bo + readback->offset) = val;
>>       }
>>  }
>
> This is the wrong order. First write the selection register, then read the
> counter. Currently this causes the reported registers to be off by one,
> if they're read in sequential order.
>

Good catch .. will be fixed in v2.

thanks
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner
Lucas Stach Jan. 30, 2017, 11:01 a.m. UTC | #3 
Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> Each perf counter 'unit' consits of a multipler configuration register
> and a register to read the selected value. Extend the uapi to handle
> this case gracefully. Before the readback is done the mux (config_reg)
> get reconfigured (vale).
> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 7 +++++--
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 3 +++
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h        | 3 +++
>  include/uapi/drm/etnaviv_drm.h               | 6 +++++-
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index a69eff7..08f9b3d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -298,14 +298,17 @@ static int submit_readback(struct etnaviv_gem_submit *submit,
>  			return -EINVAL;
>  		}
>  
> -		if (r->flags) {
> -			DRM_ERROR("readback flags not 0");
> +		if (r->flags > ETNA_READBACK_PERF) {
> +			DRM_ERROR("invalid readback flags");
>  			return -EINVAL;
>  		}
>  
>  		cmdbuf->readbacks[i].bo_vma = etnaviv_gem_vmap(&bo->obj->base);
>  		cmdbuf->readbacks[i].offset = r->readback_offset;
>  		cmdbuf->readbacks[i].reg = r->reg;
> +		cmdbuf->readbacks[i].flags = r->flags;
> +		cmdbuf->readbacks[i].perf_reg = r->perf_reg;
> +		cmdbuf->readbacks[i].perf_value = r->perf_value;

Woha, this absolutely _needs_ validation. We can't allow userspace to
freely mess with the GPU state like this.

Regards,
Lucas

>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 2aa1a26..b22212c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1379,6 +1379,9 @@ static void etnaviv_process_readbacks(struct etnaviv_gpu *gpu,
>  		const u32 val = gpu_read(gpu, readback->reg);
>  		u32 *bo = readback->bo_vma;
>  
> +		if (readback->flags & ETNA_READBACK_PERF)
> +			gpu_write(gpu, readback->perf_reg, readback->perf_value);
> +
>  		*(bo + readback->offset) = val;
>  	}
>  }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 21a4314..02f15c0 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -91,6 +91,9 @@ struct etnaviv_readback {
>  	u32 *bo_vma;
>  	u32 offset;
>  	u32 reg;
> +	u32 flags;
> +	u32 perf_reg;
> +	u32 perf_value;
>  };
>  
>  struct etnaviv_event {
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index 0d30604..f2e24bb 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -150,11 +150,15 @@ struct drm_etnaviv_gem_submit_bo {
>  	__u64 presumed;       /* in/out, presumed buffer address */
>  };
>  
> +#define ETNA_READBACK_ONLY     0x0000
> +#define ETNA_READBACK_PERF     0x0001
>  struct drm_etnaviv_gem_submit_readback {
>  	__u32 readback_offset;/* in, offset from readback_bo */
>  	__u32 readback_idx;   /* in, index of readback_bo buffer */
>  	__u32 reg;            /* in, register to read */
> -	__u32 flags;          /* in, needs to be 0 */
> +	__u32 flags;          /* in, ETNA_READBACK_* */
> +	__u32 perf_reg;       /* in, register to write */
> +	__u32 perf_value;     /* in, value to write */
>  };
>  
>  /* Each cmdstream submit consists of a table of buffers involved, and
Russell King (Oracle) Jan. 30, 2017, 11:40 p.m. UTC | #4 
On Mon, Jan 30, 2017 at 12:01:18PM +0100, Lucas Stach wrote:
> Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> > -		if (r->flags) {
> > -			DRM_ERROR("readback flags not 0");
> > +		if (r->flags > ETNA_READBACK_PERF) {
> > +			DRM_ERROR("invalid readback flags");

This test should be more robust - while ETNA_READBACK_PERF may have a
value of '1', this is not how we should be checking for invalid _flags_.
It may work, but it's not the best solution.
diff mbox

Patch

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index a69eff7..08f9b3d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -298,14 +298,17 @@  static int submit_readback(struct etnaviv_gem_submit *submit,
 			return -EINVAL;
 		}
 
-		if (r->flags) {
-			DRM_ERROR("readback flags not 0");
+		if (r->flags > ETNA_READBACK_PERF) {
+			DRM_ERROR("invalid readback flags");
 			return -EINVAL;
 		}
 
 		cmdbuf->readbacks[i].bo_vma = etnaviv_gem_vmap(&bo->obj->base);
 		cmdbuf->readbacks[i].offset = r->readback_offset;
 		cmdbuf->readbacks[i].reg = r->reg;
+		cmdbuf->readbacks[i].flags = r->flags;
+		cmdbuf->readbacks[i].perf_reg = r->perf_reg;
+		cmdbuf->readbacks[i].perf_value = r->perf_value;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 2aa1a26..b22212c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1379,6 +1379,9 @@  static void etnaviv_process_readbacks(struct etnaviv_gpu *gpu,
 		const u32 val = gpu_read(gpu, readback->reg);
 		u32 *bo = readback->bo_vma;
 
+		if (readback->flags & ETNA_READBACK_PERF)
+			gpu_write(gpu, readback->perf_reg, readback->perf_value);
+
 		*(bo + readback->offset) = val;
 	}
 }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 21a4314..02f15c0 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -91,6 +91,9 @@  struct etnaviv_readback {
 	u32 *bo_vma;
 	u32 offset;
 	u32 reg;
+	u32 flags;
+	u32 perf_reg;
+	u32 perf_value;
 };
 
 struct etnaviv_event {
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index 0d30604..f2e24bb 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -150,11 +150,15 @@  struct drm_etnaviv_gem_submit_bo {
 	__u64 presumed;       /* in/out, presumed buffer address */
 };
 
+#define ETNA_READBACK_ONLY     0x0000
+#define ETNA_READBACK_PERF     0x0001
 struct drm_etnaviv_gem_submit_readback {
 	__u32 readback_offset;/* in, offset from readback_bo */
 	__u32 readback_idx;   /* in, index of readback_bo buffer */
 	__u32 reg;            /* in, register to read */
-	__u32 flags;          /* in, needs to be 0 */
+	__u32 flags;          /* in, ETNA_READBACK_* */
+	__u32 perf_reg;       /* in, register to write */
+	__u32 perf_value;     /* in, value to write */
 };
 
 /* Each cmdstream submit consists of a table of buffers involved, and