[PATCH 00/10] drm/etnaviv: add readout support

[PATCH 00/10] drm/etnaviv: add readout support

[Christian Gmeiner]

In a perfect world we would be able to read GPU registers of interest
via the command stream with a 'read-register' command/package. For perf counters
it is a must to read them synchronized with the GPU to put the values in
relation to a draw command. As Vivante GPUs do not provide this functionality
we need to emulate it via software and do the reading of registers in the isr.

With the help of this patch series it is possible to read back register values
and store them in a defined bo at defined offsets. As we want to readback such
values within the context of the submit the struct drm_etnaviv_gem_submit gets
extend with the number of readbacks and a pointer to the readbacks array.

We are also in the lucky situation that we can not write every register via the
command stream - perf counter mux configuration - that there is a need for two
different types of readbacks:
 - normal ones
   Only read the defined register and store its value in the bo.
   
 - perf ones
   Provide an extra register/value pair to configure the mux before the register
   gets read. It is possible to reset all perf counters of a mux by selecting
   the 16 mux input. For this case we need to do a dummy read at the moment.

A very simple usage demo can be found here:
https://github.com/austriancoder/libdrm/tree/readback_v1

I am currently getting the mesa part for hw queries ready for review.

Christian Gmeiner (10):
  drm/etnaviv: add uapi for register read feature
  drm/etnaviv: add internal representation of readback
  drm/etnaviv: rework error handling
  drm/etnaviv: extend etnaviv_gpu_cmdbuf_new(..) with nr_readbacks
  drm/etnaviv: copy readbacks from userspace
  drm/etnaviv: store readback data in struct etnaviv_event
  drm/etnaviv: process readbacks in interrupt handler
  drm/etnaviv: make it possible to reconfigure perf counter
  drm/etnaviv: validate readback register address
  drm/etnaviv: add readout param

 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 82 +++++++++++++++++++++++++++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 54 +++++++++++++++---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        | 16 +++++-
 include/uapi/drm/etnaviv_drm.h               | 16 ++++++
 4 files changed, 158 insertions(+), 10 deletions(-)

-- 
2.9.3

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

[PATCH 01/10] drm/etnaviv: add uapi for register read feature

[Christian Gmeiner]

We need to readout some registers _after_ the submited command
buffer got executed in order to support perf counters.
There is no way to read register via command stream - even the
Vivante kernel driver does it via a special ioctl.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 include/uapi/drm/etnaviv_drm.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index 2584c1c..0d30604 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -150,6 +150,13 @@ struct drm_etnaviv_gem_submit_bo {
 	__u64 presumed;       /* in/out, presumed buffer address */
 };
 
+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 */
+};
+
 /* Each cmdstream submit consists of a table of buffers involved, and
  * one or more cmdstream buffers.  This allows for conditional execution
  * (context-restore), and IB buffers needed for per tile/bin draw cmds.
@@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
 	__u64 bos;            /* in, ptr to array of submit_bo's */
 	__u64 relocs;         /* in, ptr to array of submit_reloc's */
 	__u64 stream;         /* in, ptr to cmdstream */
+	__u64 readbacks;      /* in, ptr to array of submit_readback's */
+	__u32 nr_readbacks;   /* in, number of submit_readback's */
+	__u32 padding;
 };
 
 /* The normal way to synchronize with the GPU is just to CPU_PREP on
-- 
2.9.3

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

[PATCH 02/10] drm/etnaviv: add internal representation of readback

[Christian Gmeiner]

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 73c278d..6527ceb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -87,6 +87,12 @@ struct etnaviv_chip_identity {
 	u8 varyings_count;
 };
 
+struct etnaviv_readback {
+	u32 *bo_vma;
+	u32 offset;
+	u32 reg;
+};
+
 struct etnaviv_event {
 	bool used;
 	struct fence *fence;
@@ -168,6 +174,9 @@ struct etnaviv_cmdbuf {
 	u32 exec_state;
 	/* per GPU in-flight list */
 	struct list_head node;
+	/* readback's attached to this command buffer */
+	unsigned int nr_readbacks;
+	struct etnaviv_readback *readbacks;
 	/* BOs attached to this command buffer */
 	unsigned int nr_bos;
 	struct etnaviv_vram_mapping *bo_map[0];
-- 
2.9.3

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

[PATCH 03/10] drm/etnaviv: rework error handling

[Christian Gmeiner]

Prep work for following changes.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index b1254f8..09aa67a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1122,22 +1122,25 @@ struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu, u32 size,
 
 	cmdbuf = kzalloc(sz, GFP_KERNEL);
 	if (!cmdbuf)
-		return NULL;
+		goto fail;
 
 	if (gpu->mmu->version == ETNAVIV_IOMMU_V2)
 		size = ALIGN(size, SZ_4K);
 
 	cmdbuf->vaddr = dma_alloc_wc(gpu->dev, size, &cmdbuf->paddr,
 				     GFP_KERNEL);
-	if (!cmdbuf->vaddr) {
-		kfree(cmdbuf);
-		return NULL;
-	}
+	if (!cmdbuf->vaddr)
+		goto fail;
 
 	cmdbuf->gpu = gpu;
 	cmdbuf->size = size;
 
 	return cmdbuf;
+
+fail:
+	kfree(cmdbuf);
+
+	return NULL;
 }
 
 void etnaviv_gpu_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
-- 
2.9.3

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

[PATCH 04/10] drm/etnaviv: extend etnaviv_gpu_cmdbuf_new(..) with nr_readbacks

[Christian Gmeiner]

This commits extends etnaviv_gpu_cmdbuf_new(..) to define the size
of struct etnaviv_readback gets used.

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

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index afdd55d..ede5d71 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -333,7 +333,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	relocs = drm_malloc_ab(args->nr_relocs, sizeof(*relocs));
 	stream = drm_malloc_ab(1, args->stream_size);
 	cmdbuf = etnaviv_gpu_cmdbuf_new(gpu, ALIGN(args->stream_size, 8) + 8,
-					args->nr_bos);
+					args->nr_bos, 0);
 	if (!bos || !relocs || !stream || !cmdbuf) {
 		ret = -ENOMEM;
 		goto err_submit_cmds;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 09aa67a..2668723 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -655,7 +655,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
 	}
 
 	/* Create buffer: */
-	gpu->buffer = etnaviv_gpu_cmdbuf_new(gpu, PAGE_SIZE, 0);
+	gpu->buffer = etnaviv_gpu_cmdbuf_new(gpu, PAGE_SIZE, 0, 0);
 	if (!gpu->buffer) {
 		ret = -ENOMEM;
 		dev_err(gpu->dev, "could not create command buffer\n");
@@ -1114,9 +1114,10 @@ static void event_free(struct etnaviv_gpu *gpu, unsigned int event)
  */
 
 struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu, u32 size,
-	size_t nr_bos)
+	size_t nr_bos, size_t nr_readbacks)
 {
 	struct etnaviv_cmdbuf *cmdbuf;
+	struct etnaviv_readback *readbacks;
 	size_t sz = size_vstruct(nr_bos, sizeof(cmdbuf->bo_map[0]),
 				 sizeof(*cmdbuf));
 
@@ -1124,6 +1125,11 @@ struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu, u32 size,
 	if (!cmdbuf)
 		goto fail;
 
+	sz = sizeof(*readbacks) * nr_readbacks;
+	readbacks = kzalloc(sz, GFP_KERNEL);
+	if (!readbacks)
+		goto fail;
+
 	if (gpu->mmu->version == ETNAVIV_IOMMU_V2)
 		size = ALIGN(size, SZ_4K);
 
@@ -1134,11 +1140,14 @@ struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu, u32 size,
 
 	cmdbuf->gpu = gpu;
 	cmdbuf->size = size;
+	cmdbuf->readbacks = readbacks;
+	cmdbuf->nr_readbacks = nr_readbacks;
 
 	return cmdbuf;
 
 fail:
 	kfree(cmdbuf);
+	kfree(readbacks);
 
 	return NULL;
 }
@@ -1148,6 +1157,7 @@ void etnaviv_gpu_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
 	etnaviv_iommu_put_cmdbuf_va(cmdbuf->gpu, cmdbuf);
 	dma_free_wc(cmdbuf->gpu->dev, cmdbuf->size, cmdbuf->vaddr,
 		    cmdbuf->paddr);
+	kfree(cmdbuf->readbacks);
 	kfree(cmdbuf);
 }
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 6527ceb..61b36af 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -221,7 +221,7 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
 int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	struct etnaviv_gem_submit *submit, struct etnaviv_cmdbuf *cmdbuf);
 struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu,
-					      u32 size, size_t nr_bos);
+					      u32 size, size_t nr_bos, size_t nr_readbacks);
 void etnaviv_gpu_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf);
 int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu);
 void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
-- 
2.9.3

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

[PATCH 05/10] drm/etnaviv: copy readbacks from userspace

[Christian Gmeiner]

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 53 ++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index ede5d71..a69eff7 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -277,6 +277,40 @@ static int submit_reloc(struct etnaviv_gem_submit *submit, void *stream,
 	return 0;
 }
 
+static int submit_readback(struct etnaviv_gem_submit *submit,
+		struct etnaviv_cmdbuf *cmdbuf,
+		const struct drm_etnaviv_gem_submit_readback *readbacks,
+		u32 nr_readbacks)
+{
+	u32 i;
+
+	for (i = 0; i < nr_readbacks; i++) {
+		const struct drm_etnaviv_gem_submit_readback *r = readbacks + i;
+		struct etnaviv_gem_submit_bo *bo;
+		int ret;
+
+		ret = submit_bo(submit, r->readback_idx, &bo);
+		if (ret)
+			return ret;
+
+		if (r->readback_offset >= bo->obj->base.size - sizeof(u32)) {
+			DRM_ERROR("readback offset %u outside object", i);
+			return -EINVAL;
+		}
+
+		if (r->flags) {
+			DRM_ERROR("readback flags not 0");
+			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;
+	}
+
+	return 0;
+}
+
 static void submit_cleanup(struct etnaviv_gem_submit *submit)
 {
 	unsigned i;
@@ -298,6 +332,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct drm_etnaviv_gem_submit *args = data;
 	struct drm_etnaviv_gem_submit_reloc *relocs;
+	struct drm_etnaviv_gem_submit_readback *readbacks;
 	struct drm_etnaviv_gem_submit_bo *bos;
 	struct etnaviv_gem_submit *submit;
 	struct etnaviv_cmdbuf *cmdbuf;
@@ -331,10 +366,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	 */
 	bos = drm_malloc_ab(args->nr_bos, sizeof(*bos));
 	relocs = drm_malloc_ab(args->nr_relocs, sizeof(*relocs));
+	readbacks = drm_malloc_ab(args->nr_readbacks, sizeof(*readbacks));
 	stream = drm_malloc_ab(1, args->stream_size);
 	cmdbuf = etnaviv_gpu_cmdbuf_new(gpu, ALIGN(args->stream_size, 8) + 8,
-					args->nr_bos, 0);
-	if (!bos || !relocs || !stream || !cmdbuf) {
+					args->nr_bos, args->nr_readbacks);
+	if (!bos || !relocs || !readbacks || !stream || !cmdbuf) {
 		ret = -ENOMEM;
 		goto err_submit_cmds;
 	}
@@ -356,6 +392,13 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_cmds;
 	}
 
+	ret = copy_from_user(readbacks, u64_to_user_ptr(args->readbacks),
+			     args->nr_readbacks * sizeof(*readbacks));
+	if (ret) {
+		ret = -EFAULT;
+		goto err_submit_cmds;
+	}
+
 	ret = copy_from_user(stream, u64_to_user_ptr(args->stream),
 			     args->stream_size);
 	if (ret) {
@@ -396,6 +439,10 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	if (ret)
 		goto out;
 
+	ret = submit_readback(submit, cmdbuf, readbacks, args->nr_readbacks);
+	if (ret)
+		goto out;
+
 	memcpy(cmdbuf->vaddr, stream, args->stream_size);
 	cmdbuf->user_size = ALIGN(args->stream_size, 8);
 
@@ -429,6 +476,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		drm_free_large(bos);
 	if (relocs)
 		drm_free_large(relocs);
+	if (readbacks)
+		drm_free_large(readbacks);
 
 	return ret;
 }
-- 
2.9.3

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

[PATCH 06/10] drm/etnaviv: store readback data in struct etnaviv_event

[Christian Gmeiner]

In the interrupt handler struct etnaviv_event is the only
information source to process readbacks.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 3 +++
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 2668723..1fb5e37 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1321,7 +1321,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 
 	mutex_lock(&gpu->lock);
 
+	gpu->event[event].nr_readbacks = cmdbuf->nr_readbacks;
+	gpu->event[event].readbacks = cmdbuf->readbacks;
 	gpu->event[event].fence = fence;
+
 	submit->fence = fence->seqno;
 	gpu->active_fence = submit->fence;
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 61b36af..21a4314 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -95,6 +95,8 @@ struct etnaviv_readback {
 
 struct etnaviv_event {
 	bool used;
+	unsigned int nr_readbacks;
+	struct etnaviv_readback *readbacks;
 	struct fence *fence;
 };
 
-- 
2.9.3

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

[PATCH 07/10] drm/etnaviv: process readbacks in interrupt handler

[Christian Gmeiner]

We can not defere the readback of the registers as the values likely
getting changed by an other command buffer.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 1fb5e37..2aa1a26 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1369,6 +1369,20 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	return ret;
 }
 
+static void etnaviv_process_readbacks(struct etnaviv_gpu *gpu,
+		struct etnaviv_event *event)
+{
+	unsigned i;
+
+	for (i = 0; i < event->nr_readbacks; i++) {
+		const struct etnaviv_readback *readback = event->readbacks + i;
+		const u32 val = gpu_read(gpu, readback->reg);
+		u32 *bo = readback->bo_vma;
+
+		*(bo + readback->offset) = val;
+	}
+}
+
 /*
  * Init/Cleanup:
  */
@@ -1415,6 +1429,9 @@ static irqreturn_t irq_handler(int irq, void *data)
 
 			dev_dbg(gpu->dev, "event %u\n", event);
 
+			if (gpu->event[event].nr_readbacks)
+				etnaviv_process_readbacks(gpu, &gpu->event[event]);
+
 			fence = gpu->event[event].fence;
 			gpu->event[event].fence = NULL;
 			fence_signal(fence);
-- 
2.9.3

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

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

[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;
 	}
 
 	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
-- 
2.9.3

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

[PATCH 09/10] drm/etnaviv: validate readback register address

[Christian Gmeiner]

Reading some registers end in a system crash ala:

  Unhandled fault: external abort on non-linefetch (0x1028) at 0xfe641000
  Internal error: : 1028 [#1] PREEMPT ARM

Avoid those by register validation.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 08f9b3d..4383c0d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -277,6 +277,27 @@ static int submit_reloc(struct etnaviv_gem_submit *submit, void *stream,
 	return 0;
 }
 
+static int readback_reg_valid(unsigned reg)
+{
+	/*
+	 * 0x000..0x200:     ok
+	 * 0x200..0x400:     crash
+	 * 0x400..0x800:     ok
+	 * 0x800..0xa00:     crash
+	 * 0xa00..0xc00:     crash
+	 * 0xc00..0xe00:     crash
+	 * 0xe00..0x1000:    crash
+	 * everything above: crash
+	 */
+	if (reg >= 0x200 && reg < 400)
+		return 0;
+
+	if (reg >= 0x800)
+		return 0;
+
+	return 1;
+}
+
 static int submit_readback(struct etnaviv_gem_submit *submit,
 		struct etnaviv_cmdbuf *cmdbuf,
 		const struct drm_etnaviv_gem_submit_readback *readbacks,
@@ -303,6 +324,11 @@ static int submit_readback(struct etnaviv_gem_submit *submit,
 			return -EINVAL;
 		}
 
+		if (!readback_reg_valid(r->reg)) {
+			DRM_ERROR("invalid readback reg (would cause crash)");
+			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;
-- 
2.9.3

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

[PATCH 10/10] drm/etnaviv: add readout param

[Christian Gmeiner]

We need this for perf counters.

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

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index b22212c..b3eba08 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -122,6 +122,10 @@ int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32 param, u64 *value)
 		*value = gpu->identity.varyings_count;
 		break;
 
+	case ETNAVIV_PARAM_READOUT:
+		*value = 1;
+		break;
+
 	default:
 		DBG("%s: invalid param: %u", dev_name(gpu->dev), param);
 		return -EINVAL;
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index f2e24bb..cc4d076 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -67,6 +67,8 @@ struct drm_etnaviv_timespec {
 #define ETNAVIV_PARAM_GPU_NUM_CONSTANTS             0x19
 #define ETNAVIV_PARAM_GPU_NUM_VARYINGS              0x1a
 
+#define ETNAVIV_PARAM_READOUT                       0x20
+
 #define ETNA_MAX_PIPES 4
 
 struct drm_etnaviv_param {
-- 
2.9.3

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

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

[Wladimir J. van der Laan]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian Gmeiner]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature

[Lucas Stach]

Hi Christian,

Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> We need to readout some registers _after_ the submited command
> buffer got executed in order to support perf counters.
> There is no way to read register via command stream - even the
> Vivante kernel driver does it via a special ioctl.
> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  include/uapi/drm/etnaviv_drm.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index 2584c1c..0d30604 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -150,6 +150,13 @@ struct drm_etnaviv_gem_submit_bo {
>  	__u64 presumed;       /* in/out, presumed buffer address */
>  };
>  
> +struct drm_etnaviv_gem_submit_readback {
> +	__u32 readback_offset;/* in, offset from readback_bo */
> +	__u32 readback_idx;   /* in, index of readback_bo buffer */

Why do we need this readback BO? Wouldn't it be easier to just have the
space for the readback value allocated in this struct and have the
kernel fill it in? We are reading the values by using the CPU anyways,
so I don't see the need for this indirection with a BO. We removed the
command BO with the same justification.

Regards,
Lucas

> +	__u32 reg;            /* in, register to read */
> +	__u32 flags;          /* in, needs to be 0 */
> +};
> +
>  /* Each cmdstream submit consists of a table of buffers involved, and
>   * one or more cmdstream buffers.  This allows for conditional execution
>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> @@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
>  	__u64 bos;            /* in, ptr to array of submit_bo's */
>  	__u64 relocs;         /* in, ptr to array of submit_reloc's */
>  	__u64 stream;         /* in, ptr to cmdstream */
> +	__u64 readbacks;      /* in, ptr to array of submit_readback's */
> +	__u32 nr_readbacks;   /* in, number of submit_readback's */
> +	__u32 padding;
>  };
>  
>  /* The normal way to synchronize with the GPU is just to CPU_PREP on


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

Re: [PATCH 09/10] drm/etnaviv: validate readback register address

[Lucas Stach]

Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> Reading some registers end in a system crash ala:
> 
>   Unhandled fault: external abort on non-linefetch (0x1028) at 0xfe641000
>   Internal error: : 1028 [#1] PREEMPT ARM
> 
> Avoid those by register validation.

Avoiding crashes is one thing, but I believe this needs to go further
and avoid reads from any register that isn't a performance counter. This
probably isn't a big hole, but we want to avoid leaking the GPU state to
userspace.

Regards,
Lucas

> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 08f9b3d..4383c0d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -277,6 +277,27 @@ static int submit_reloc(struct etnaviv_gem_submit *submit, void *stream,
>  	return 0;
>  }
>  
> +static int readback_reg_valid(unsigned reg)
> +{
> +	/*
> +	 * 0x000..0x200:     ok
> +	 * 0x200..0x400:     crash
> +	 * 0x400..0x800:     ok
> +	 * 0x800..0xa00:     crash
> +	 * 0xa00..0xc00:     crash
> +	 * 0xc00..0xe00:     crash
> +	 * 0xe00..0x1000:    crash
> +	 * everything above: crash
> +	 */
> +	if (reg >= 0x200 && reg < 400)
> +		return 0;
> +
> +	if (reg >= 0x800)
> +		return 0;
> +
> +	return 1;
> +}
> +
>  static int submit_readback(struct etnaviv_gem_submit *submit,
>  		struct etnaviv_cmdbuf *cmdbuf,
>  		const struct drm_etnaviv_gem_submit_readback *readbacks,
> @@ -303,6 +324,11 @@ static int submit_readback(struct etnaviv_gem_submit *submit,
>  			return -EINVAL;
>  		}
>  
> +		if (!readback_reg_valid(r->reg)) {
> +			DRM_ERROR("invalid readback reg (would cause crash)");
> +			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;


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

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

[Lucas Stach]

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


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

Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1918 bytes --]

On Fri, Dec 09, 2016 at 12:21:22PM +0100, Christian Gmeiner wrote:
> We need to readout some registers _after_ the submited command
> buffer got executed in order to support perf counters.
> There is no way to read register via command stream - even the
> Vivante kernel driver does it via a special ioctl.
> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  include/uapi/drm/etnaviv_drm.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index 2584c1c..0d30604 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -150,6 +150,13 @@ struct drm_etnaviv_gem_submit_bo {
>  	__u64 presumed;       /* in/out, presumed buffer address */
>  };
>  
> +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 */
> +};
> +
>  /* Each cmdstream submit consists of a table of buffers involved, and
>   * one or more cmdstream buffers.  This allows for conditional execution
>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> @@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
>  	__u64 bos;            /* in, ptr to array of submit_bo's */
>  	__u64 relocs;         /* in, ptr to array of submit_reloc's */
>  	__u64 stream;         /* in, ptr to cmdstream */
> +	__u64 readbacks;      /* in, ptr to array of submit_readback's */
> +	__u32 nr_readbacks;   /* in, number of submit_readback's */
> +	__u32 padding;
>  };

What about ABI stability? How's this going to work when userspace uses
old headers but runs against a new kernel? What about userspace using
newer headers than the kernel?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 09/10] drm/etnaviv: validate readback register address

[Russell King - ARM Linux]

On Mon, Jan 30, 2017 at 11:58:32AM +0100, Lucas Stach wrote:
> Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> > Reading some registers end in a system crash ala:
> > 
> >   Unhandled fault: external abort on non-linefetch (0x1028) at 0xfe641000
> >   Internal error: : 1028 [#1] PREEMPT ARM
> > 
> > Avoid those by register validation.
> 
> Avoiding crashes is one thing, but I believe this needs to go further
> and avoid reads from any register that isn't a performance counter. This
> probably isn't a big hole, but we want to avoid leaking the GPU state to
> userspace.

We certainly don't want to let people use this to read stuff like the
ID registers, bypassing the kernel.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature

[Russell King - ARM Linux]

On Mon, Jan 30, 2017 at 09:18:25PM +0100, Thierry Reding wrote:
> On Fri, Dec 09, 2016 at 12:21:22PM +0100, Christian Gmeiner wrote:
> > @@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
> >  	__u64 bos;            /* in, ptr to array of submit_bo's */
> >  	__u64 relocs;         /* in, ptr to array of submit_reloc's */
> >  	__u64 stream;         /* in, ptr to cmdstream */
> > +	__u64 readbacks;      /* in, ptr to array of submit_readback's */
> > +	__u32 nr_readbacks;   /* in, number of submit_readback's */
> > +	__u32 padding;
> >  };
> 
> What about ABI stability? How's this going to work when userspace uses
> old headers but runs against a new kernel? What about userspace using
> newer headers than the kernel?

+1, this is unacceptable.  We went through a period of making sure
that the ABI was going to be stable once we merged the driver into
the kernel, and the rule is that we don't ever break userspace.  This
does exactly that, so it's not permissible.

If this change is necessary, it needs to be a new ioctl number for the
call with the new structure, and the old ioctl has to keep working.

There are other users of etnaviv other than mesa that will break.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature

[Daniel Vetter]

On Mon, Jan 30, 2017 at 11:45:59PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 30, 2017 at 09:18:25PM +0100, Thierry Reding wrote:
> > On Fri, Dec 09, 2016 at 12:21:22PM +0100, Christian Gmeiner wrote:
> > > @@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
> > >  	__u64 bos;            /* in, ptr to array of submit_bo's */
> > >  	__u64 relocs;         /* in, ptr to array of submit_reloc's */
> > >  	__u64 stream;         /* in, ptr to cmdstream */
> > > +	__u64 readbacks;      /* in, ptr to array of submit_readback's */
> > > +	__u32 nr_readbacks;   /* in, number of submit_readback's */
> > > +	__u32 padding;
> > >  };
> > 
> > What about ABI stability? How's this going to work when userspace uses
> > old headers but runs against a new kernel? What about userspace using
> > newer headers than the kernel?
> 
> +1, this is unacceptable.  We went through a period of making sure
> that the ABI was going to be stable once we merged the driver into
> the kernel, and the rule is that we don't ever break userspace.  This
> does exactly that, so it's not permissible.
> 
> If this change is necessary, it needs to be a new ioctl number for the
> call with the new structure, and the old ioctl has to keep working.
> 
> There are other users of etnaviv other than mesa that will break.

As long as you don't do it wrong (i.e. struct not used in arrays, and some
flags to indicate when the new fields should be looked at) then it's
perfectly safe to extend drm ioctl structs at the end. The core drm ioctl
functions zero-pad length mismatches in both directions if needed.

The only additional recommendation is to still do a new #define (the ioctl
number has changed anyway since it encodes the size), to make sure that
old userspace still uses the old structs even with upgraded headers.
Otherwise they might not properly clear the new fields, which is the one
case that breaks backwards/forwards compat (been there, done that, not
fun).

We should probably have a chapter in the drm-uapi.rst docs about this,
with a link to the botching-up-ioctls.txt file with the more general
recommendatations.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel