drm/msm: use proper memory barriers for updating tail/head
PATCH 1/1 drm/msm: use proper memory barriers for updating tail/head
Fixes intermittent corruption of cmdstream dump.
--- drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/msm_rd.c +2 -2
@@ -120,7 +120,7 @@ static void rd_write(struct msm_rd_state *rd, const void *buf, int sz) n =min (sz,circ_space_to_end (&rd->fifo));memcpy (fptr, ptr, n); - fifo->head = (fifo->head + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->head, (fifo->head + n) & (BUF_SZ -1 )); sz -= n; ptr += n;
@@ -157,7 +157,7 @@ static ssize_t rd_read(struct file *file, char __user *buf, goto out; } - fifo->tail = (fifo->tail + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->tail, (fifo->tail + n) & (BUF_SZ -1 )); *ppos += n;wake_up_all (&rd->fifo_event);
Signature · 1 line
2.13.5
Quoted 3 lines · Rob Clark
Fixes intermittent corruption of cmdstream dump.
Signed-off-by Rob Clark <[email protected]>
Rule of memory barriers: They always need to come in pairs, and you should put a comment to each one explaining where the other side is. I.e. for all the store_release ones you add here you need to put a few read_acquire ones somewhere else, and then explain how they link.
One side only doesn't really work with barriers - except when you're lucky and on x86 :-)
-Daniel
Quoted 33 lines
--- drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/gpu/drm/msm/msm_rd.c +2
-2@@ -120,7 +120,7 @@ static void rd_write(struct msm_rd_state *rd, const void *buf, int sz) n =min (sz,circ_space_to_end (&rd->fifo));memcpy (fptr, ptr, n); - fifo->head = (fifo->head + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->head, (fifo->head + n) & (BUF_SZ -1 )); sz -= n; ptr += n;@@ -157,7 +157,7 @@ static ssize_t rd_read(struct file *file, char __user *buf, goto out; } - fifo->tail = (fifo->tail + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->tail, (fifo->tail + n) & (BUF_SZ -1 )); *ppos += n;wake_up_all (&rd->fifo_event);Signature · 6 lines
2.13.5 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 5 lines · Daniel Vetter
Quoted 3 lines · Rob Clark
Fixes intermittent corruption of cmdstream dump.
Signed-off-by Rob Clark <[email protected]>
Quoted 7 lines
Rule of memory barriers: They always need to come in pairs, and you should put a comment to each one explaining where the other side is. I.e. for all the
store_releaseones you add here you need to put a fewread_acquireones somewhere else, and then explain how they link.One side only doesn't really work with barriers - except when you're lucky and on x86 :-)
fwiw, this is straight out of Documentation/circular-buffers.txt
I guess it could read_acquire when checking the avail/free space on the consumer/producer side.. otoh if we see an old value there it is harmless (ie. we just fall into a wait_event()). I assume that is why the documentation didn't bother with a read_acquire.
BR,
-R
Quoted 30 lines
-Daniel
Quoted 29 lines
--- drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/gpu/drm/msm/msm_rd.c +2
-3@@ -120,7 +120,7 @@ static void rd_write(struct msm_rd_state *rd, const void *buf, int sz) n =min (sz,circ_space_to_end (&rd->fifo));memcpy (fptr, ptr, n); - fifo->head = (fifo->head + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->head, (fifo->head + n) & (BUF_SZ -1 )); sz -= n; ptr += n;@@ -157,7 +157,7 @@ static ssize_t rd_read(struct file *file, char __user *buf, goto out; } - fifo->tail = (fifo->tail + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->tail, (fifo->tail + n) & (BUF_SZ -1 )); *ppos += n;wake_up_all (&rd->fifo_event); --2.13.5
Quoted 9 lines
Quoted 4 lines
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 14 lines · Rob Clark
Quoted 5 lines · Daniel Vetter
Quoted 3 lines · Rob Clark
Fixes intermittent corruption of cmdstream dump.
Signed-off-by Rob Clark <[email protected]>Quoted 7 lines
Rule of memory barriers: They always need to come in pairs, and you should put a comment to each one explaining where the other side is. I.e. for all the
store_releaseones you add here you need to put a fewread_acquireones somewhere else, and then explain how they link.One side only doesn't really work with barriers - except when you're lucky and on x86 :-)
Quoted 6 lines
fwiw, this is straight out of
Documentation/circular-buffers.txtI guess it could
read_acquirewhen checking the avail/free space on the consumer/producer side.. otoh if we see an old value there it is harmless (ie. we just fall into await_event()). I assume that is why the documentation didn't bother with aread_acquire.
smp_load_acquire is the right name, and it's right there. In the example at least :-)
-Daniel
Quoted 43 lines
BR,
-RQuoted 39 lines
-Daniel
Quoted 29 lines
--- drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/gpu/drm/msm/msm_rd.c +2
-3@@ -120,7 +120,7 @@ static void rd_write(struct msm_rd_state *rd, const void *buf, int sz) n =min (sz,circ_space_to_end (&rd->fifo));memcpy (fptr, ptr, n); - fifo->head = (fifo->head + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->head, (fifo->head + n) & (BUF_SZ -1 )); sz -= n; ptr += n;@@ -157,7 +157,7 @@ static ssize_t rd_read(struct file *file, char __user *buf, goto out; } - fifo->tail = (fifo->tail + n) & (BUF_SZ -1 ); +smp_store_release (&fifo->tail, (fifo->tail + n) & (BUF_SZ -1 )); *ppos += n;wake_up_all (&rd->fifo_event); --2.13.5
Quoted 4 lines
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel