* [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
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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.