Fixes intermittent corruption of cmdstream dump.
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c index 5c087a4228ff..918c65e19964 100644 --- a/drivers/gpu/drm/msm/msm_rd.c +++ b/drivers/gpu/drm/msm/msm_rd.c @@ -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);
On Mon, Oct 02, 2017 at 10:37:11AM -0400, Rob Clark wrote:
Fixes intermittent corruption of cmdstream dump.
Signed-off-by: Rob Clark robdclark@gmail.com
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
drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c index 5c087a4228ff..918c65e19964 100644 --- a/drivers/gpu/drm/msm/msm_rd.c +++ b/drivers/gpu/drm/msm/msm_rd.c @@ -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));@@ -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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Oct 3, 2017 at 4:08 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 02, 2017 at 10:37:11AM -0400, Rob Clark wrote:
Fixes intermittent corruption of cmdstream dump.
Signed-off-by: Rob Clark robdclark@gmail.com
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 :-)
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
-Daniel
drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c index 5c087a4228ff..918c65e19964 100644 --- a/drivers/gpu/drm/msm/msm_rd.c +++ b/drivers/gpu/drm/msm/msm_rd.c @@ -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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Oct 3, 2017 at 2:59 PM, Rob Clark robdclark@gmail.com wrote:
On Tue, Oct 3, 2017 at 4:08 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 02, 2017 at 10:37:11AM -0400, Rob Clark wrote:
Fixes intermittent corruption of cmdstream dump.
Signed-off-by: Rob Clark robdclark@gmail.com
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 :-)
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.
smp_load_acquire is the right name, and it's right there. In the example at least :-) -Daniel
BR, -R
-Daniel
drivers/gpu/drm/msm/msm_rd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c index 5c087a4228ff..918c65e19964 100644 --- a/drivers/gpu/drm/msm/msm_rd.c +++ b/drivers/gpu/drm/msm/msm_rd.c @@ -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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Rob Clark