Re: [PATCH] drm/msm: use proper memory barriers for updating tail/head

[Rob Clark <robdclark@gmail.com>]

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