* [PATCH] drm/msm: use proper memory barriers for updating tail/head @ 2017-10-02 14:37 Rob Clark 2017-10-03 8:08 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Rob Clark @ 2017-10-02 14:37 UTC (permalink / raw) To: dri-devel; +Cc: freedreno, linux-arm-msm, Rob Clark 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); -- 2.13.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/msm: use proper memory barriers for updating tail/head 2017-10-02 14:37 [PATCH] drm/msm: use proper memory barriers for updating tail/head Rob Clark @ 2017-10-03 8:08 ` Daniel Vetter 2017-10-03 12:59 ` Rob Clark 0 siblings, 1 reply; 4+ messages in thread From: Daniel Vetter @ 2017-10-03 8:08 UTC (permalink / raw) To: Rob Clark; +Cc: linux-arm-msm, freedreno, dri-devel 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)); > 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 mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/msm: use proper memory barriers for updating tail/head 2017-10-03 8:08 ` Daniel Vetter @ 2017-10-03 12:59 ` Rob Clark 2017-10-03 16:11 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Rob Clark @ 2017-10-03 12:59 UTC (permalink / raw) To: Daniel Vetter; +Cc: linux-arm-msm, freedreno, dri-devel@lists.freedesktop.org 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/msm: use proper memory barriers for updating tail/head 2017-10-03 12:59 ` Rob Clark @ 2017-10-03 16:11 ` Daniel Vetter 0 siblings, 0 replies; 4+ messages in thread From: Daniel Vetter @ 2017-10-03 16:11 UTC (permalink / raw) To: Rob Clark; +Cc: linux-arm-msm, freedreno, dri-devel@lists.freedesktop.org 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-03 16:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-02 14:37 [PATCH] drm/msm: use proper memory barriers for updating tail/head Rob Clark 2017-10-03 8:08 ` Daniel Vetter 2017-10-03 12:59 ` Rob Clark 2017-10-03 16:11 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).