Skip to contentSkip to conversation

drm/msm: Don't split an IB at the end of ring buffer.

PATCH In review Ganesan, Aravind 2 participants 2 messages Oct 31, 2014 – Nov 18, 2014

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 drm/msm: Don't split an IB at the end of ring buffer.

Splitting the command sequence for an IB1 submission at the end of the ring buffer can hang the GPU. To fix this, if there isn't enough contiguous space at the end to fit the full command sequence, insert NOPs at the end, and write the sequence at the start, as space becomes available.

---
Resend in patch-set format and with [email protected] on the CC.

 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 45
++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  8 +++---
 2 files changed, 46 insertions(+), 7 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 1fe7c8d..51901df 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.c +42 -3

52 lines not on this page · raw message

(quoted diff) +0 -0

b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 3fa06b3..47ba8f5 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.h +2 -2
@@ -247,7 +247,7 @@ void adreno_idle(struct msm_gpu *gpu);
 void adreno_show(struct msm_gpu *gpu, struct seq_file *m);
 #endif
 void adreno_dump(struct msm_gpu *gpu);
-void adreno_wait_ring(struct msm_gpu *gpu, uint32_t ndwords);
+void adreno_wait_ring_contiguous(struct msm_gpu *gpu, uint32_t ndwords);
 
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs);
@@ -259,7 +259,7 @@ void adreno_gpu_cleanup(struct adreno_gpu *gpu);
 static inline void
 OUT_PKT0(struct msm_ringbuffer *ring, uint16_t regindx, uint16_t cnt)
 {
-	adreno_wait_ring(ring->gpu, cnt+1);
+	adreno_wait_ring_contiguous(ring->gpu, cnt+1);
 	OUT_RING(ring, CP_TYPE0_PKT | ((cnt-1) << 16) | (regindx & 0x7FFF));
 }
 
@@ -267,14 +267,14 @@ OUT_PKT0(struct msm_ringbuffer *ring, uint16_t

regindx, uint16_t cnt)
static inline void
OUT_PKT2(struct msm_ringbuffer *ring)
{
- adreno_wait_ring(ring->gpu, 1);
+ adreno_wait_ring_contiguous(ring->gpu, 1);
OUT_RING(ring, CP_TYPE2_PKT);
}

static inline void
OUT_PKT3(struct msm_ringbuffer *ring, uint8_t opcode, uint16_t cnt) {
- adreno_wait_ring(ring->gpu, cnt+1);
+ adreno_wait_ring_contiguous(ring->gpu, cnt+1);
OUT_RING(ring, CP_TYPE3_PKT | ((cnt-1) << 16) | ((opcode & 0xFF) << 8));
}

Signature · 6 lines
1.8.5.2

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#1 lore raw

Hi Ganesan,

Quoted 27 lines · Aravind

Splitting the command sequence for an IB1 submission at the end of the ring buffer can hang the GPU. To fix this, if there isn't enough contiguous space at the end to fit the full command sequence, insert NOPs at the end, and write the sequence at the start, as space becomes available.

---
Resend in patch-set format and with [email protected] on the CC.

 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 45
++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  8 +++---
 2 files changed, 46 insertions(+), 7 deletions(-)
drivers/gpu/drm/msm/adreno/adreno_gpu.c +2 -1
index 1fe7c8d..51901df 100644
@@ -281,10 +281,49 @@ static uint32_t ring_freewords(struct msm_gpu *gpu)
 	return (rptr + (size - 1) - wptr) % size;
 }
 
-void adreno_wait_ring(struct msm_gpu *gpu, uint32_t ndwords)
+void adreno_wait_ring_contiguous(struct msm_gpu *gpu,
+		uint32_t ndwords)

drivers/gpu/drm/msm/adreno/adreno_gpu.c static uint32_t ring_freewords(struct msm_gpu *gpu) · line 293

Quoted 10 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +7 -2
 {
-	if (spin_until(ring_freewords(gpu) >= ndwords))
-		DRM_ERROR("%s: timeout waiting for ringbuffer space\n", gpu->name);
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	uint32_t size = gpu->rb->size/4;
+	uint32_t wptr;
+	uint32_t rptr;
+
+	/* Wait for free space and then check if they are contiguous */
+	if(spin_until(ring_freewords(gpu)>= ndwords)){

There are some checkpatch errors,

ERROR: spaces required around that '>=' (ctx:VxW) #42: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:293:
+ if(spin_until(ring_freewords(gpu)>= ndwords)){
^
ERROR: space required before the open brace '{' #42: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:293:
+ if(spin_until(ring_freewords(gpu)>= ndwords)){

ERROR: space required before the open parenthesis '(' #42: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:293:
+ if(spin_until(ring_freewords(gpu)>= ndwords)){

Quoted 7 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +7 -0
+		DRM_ERROR("%s: timeout waiting for ringbuffer space\n",
+				gpu->name);
+		return;
+	}
+
+	wptr = get_wptr(gpu->rb);
+	rptr = adreno_gpu->memptrs->rptr;

drivers/gpu/drm/msm/adreno/adreno_gpu.c static uint32_t ring_freewords(struct msm_gpu *gpu) · line 310

Quoted 10 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +10 -0
+
+	/* We have enough space in the ring for ndwords. Three conditions
+	 * indicates we have contigous space:
+	 * (1) wptr can be equal to size, ring has wrapped and wptr is 0
+	 * (see OUT_RING), meaning we have enough space.
+	 * (2) We have enough space in the ring, wptr < rptr indicates
+	 * enough contiguous space
+	 * (3) wptr + ndwords < size - 1 implies enough space in the ring.
+	 */
+	if((wptr == size) || (wptr < rptr) || (wptr + ndwords < size - 1))

ERROR: space required before the open parenthesis '(' #59: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:310:
+ if((wptr == size) || (wptr < rptr) || (wptr + ndwords < size - 1))

drivers/gpu/drm/msm/adreno/adreno_gpu.c static uint32_t ring_freewords(struct msm_gpu *gpu) · line 322

Quoted 12 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +12 -0
+		return;
+
+	/* Fill the end of ring with no-ops
+	 * */
+	OUT_RING(gpu->rb, CP_TYPE3_PKT | (((size - wptr - 1) - 1) << 16) |
+			((CP_NOP & 0xFF) << 8));
+	gpu->rb->cur = gpu->rb->start;
+
+	/* We have reset cur pointer to start. If ring_freewords returns
+	 * greater than ndwords, then we have contigous space.
+	 * */
+	if(spin_until(ring_freewords(gpu)>= ndwords)){

ERROR: spaces required around that '>=' (ctx:VxW) #71: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:322:
+ if(spin_until(ring_freewords(gpu)>= ndwords)){
^

ERROR: space required before the open brace '{' #71: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:322:
+ if(spin_until(ring_freewords(gpu)>= ndwords)){

ERROR: space required before the open parenthesis '(' #71: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.c:322:
+ if(spin_until(ring_freewords(gpu)>= ndwords)){

Quoted 31 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +4 -0
+		DRM_ERROR("%s: timeout waiting for ringbuffer space\n",
+				gpu->name);
+		return;
+	}
 }
 
 static const char *iommu_ports[] = {
drivers/gpu/drm/msm/adreno/adreno_gpu.h +2 -2
index 3fa06b3..47ba8f5 100644
@@ -247,7 +247,7 @@ void adreno_idle(struct msm_gpu *gpu);
 void adreno_show(struct msm_gpu *gpu, struct seq_file *m);
 #endif
 void adreno_dump(struct msm_gpu *gpu);
-void adreno_wait_ring(struct msm_gpu *gpu, uint32_t ndwords);
+void adreno_wait_ring_contiguous(struct msm_gpu *gpu, uint32_t ndwords);
 
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 		struct adreno_gpu *gpu, const struct adreno_gpu_funcs *funcs);
@@ -259,7 +259,7 @@ void adreno_gpu_cleanup(struct adreno_gpu *gpu);
 static inline void
 OUT_PKT0(struct msm_ringbuffer *ring, uint16_t regindx, uint16_t cnt)
 {
-	adreno_wait_ring(ring->gpu, cnt+1);
+	adreno_wait_ring_contiguous(ring->gpu, cnt+1);
 	OUT_RING(ring, CP_TYPE0_PKT | ((cnt-1) << 16) | (regindx & 0x7FFF));
 }
 

drivers/gpu/drm/msm/adreno/adreno_gpu.h OUT_PKT0(struct msm_ringbuffer *ring, uint16_t · line 267

Quoted 1 line
drivers/gpu/drm/msm/adreno/adreno_gpu.h +0 -0
@@ -267,14 +267,14 @@ OUT_PKT0(struct msm_ringbuffer *ring, uint16_t
 regindx, uint16_t cnt)

ERROR: patch seems to be corrupt (line wrapped?) #103: FILE: drivers/gpu/drm/msm/adreno/adreno_gpu.h:266:
regindx, uint16_t cnt)

Thanks,
--Kiran

Quoted 28 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.h +2 -3
 static inline void
 OUT_PKT2(struct msm_ringbuffer *ring)
 {
-	adreno_wait_ring(ring->gpu, 1);
+	adreno_wait_ring_contiguous(ring->gpu, 1);
 	OUT_RING(ring, CP_TYPE2_PKT);
 }
 
 static inline void
 OUT_PKT3(struct msm_ringbuffer *ring, uint8_t opcode, uint16_t cnt)
 {
-	adreno_wait_ring(ring->gpu, cnt+1);
+	adreno_wait_ring_contiguous(ring->gpu, cnt+1);
 	OUT_RING(ring, CP_TYPE3_PKT | ((cnt-1) << 16) | ((opcode & 0xFF) << 8));
 }
 
--

1.8.5.2

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html