[PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Benjamin Herrenschmidt]

We should have a read memory barrier between reading the WPTR from
memory and reading ring entries based on that value (ie, we need to
ensure both loads are done in order by the CPU).

It could be argued that the MMIO reads in r600_ack_irq() might be
enough to get that barrier but I prefer keeping an explicit one just
in case.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 
(resent adding dri-devel to the CC list to hit patchwork)

drivers/gpu/drm/radeon/r600.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index 3c86b15..7e5c801 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
 	}
 
 restart_ih:
+	/* Order reading of wptr vs. reading of IH ring data */
+	wmb();
+
 	/* display interrupts */
 	r600_irq_ack(rdev);

Re: [PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Alex Deucher]

On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> We should have a read memory barrier between reading the WPTR from
> memory and reading ring entries based on that value (ie, we need to
> ensure both loads are done in order by the CPU).
>
> It could be argued that the MMIO reads in r600_ack_irq() might be
> enough to get that barrier but I prefer keeping an explicit one just
> in case.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>
> (resent adding dri-devel to the CC list to hit patchwork)
>
> drivers/gpu/drm/radeon/r600.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index 3c86b15..7e5c801 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
>        }
>
>  restart_ih:
> +       /* Order reading of wptr vs. reading of IH ring data */
> +       wmb();
> +
>        /* display interrupts */
>        r600_irq_ack(rdev);
>
>
>
>
>

Re: [PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Alex Deucher]

On Wed, Jul 13, 2011 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> We should have a read memory barrier between reading the WPTR from
>> memory and reading ring entries based on that value (ie, we need to
>> ensure both loads are done in order by the CPU).
>>
>> It could be argued that the MMIO reads in r600_ack_irq() might be
>> enough to get that barrier but I prefer keeping an explicit one just
>> in case.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

evergreen.c will need a similar fix.

Alex

>
>> ---
>>
>> (resent adding dri-devel to the CC list to hit patchwork)
>>
>> drivers/gpu/drm/radeon/r600.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
>> index 3c86b15..7e5c801 100644
>> --- a/drivers/gpu/drm/radeon/r600.c
>> +++ b/drivers/gpu/drm/radeon/r600.c
>> @@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
>>        }
>>
>>  restart_ih:
>> +       /* Order reading of wptr vs. reading of IH ring data */
>> +       wmb();
>> +
>>        /* display interrupts */
>>        r600_irq_ack(rdev);
>>
>>
>>
>>
>>
>

Re: [PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Benjamin Herrenschmidt]

On Wed, 2011-07-13 at 10:48 -0400, Alex Deucher wrote:
> On Wed, Jul 13, 2011 at 10:43 AM, Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
> > <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> >> We should have a read memory barrier between reading the WPTR from
> >> memory and reading ring entries based on that value (ie, we need to
> >> ensure both loads are done in order by the CPU).
> >>
> >> It could be argued that the MMIO reads in r600_ack_irq() might be
> >> enough to get that barrier but I prefer keeping an explicit one just
> >> in case.
> >>
> >> Signed-off-by: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> >
> > Reviewed-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>
> 
> evergreen.c will need a similar fix.

Ok. I can do that.

Cheers,
Ben.

> Alex
> 
> >
> >> ---
> >>
> >> (resent adding dri-devel to the CC list to hit patchwork)
> >>
> >> drivers/gpu/drm/radeon/r600.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> >> index 3c86b15..7e5c801 100644
> >> --- a/drivers/gpu/drm/radeon/r600.c
> >> +++ b/drivers/gpu/drm/radeon/r600.c
> >> @@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
> >>        }
> >>
> >>  restart_ih:
> >> +       /* Order reading of wptr vs. reading of IH ring data */
> >> +       wmb();
> >> +
> >>        /* display interrupts */
> >>        r600_irq_ack(rdev);
> >>
> >>
> >>
> >>
> >>
> >

Re: [PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Alex Deucher]

On Wed, Jul 13, 2011 at 5:42 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2011-07-13 at 10:48 -0400, Alex Deucher wrote:
>> On Wed, Jul 13, 2011 at 10:43 AM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> > On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
>> > <benh@kernel.crashing.org> wrote:
>> >> We should have a read memory barrier between reading the WPTR from
>> >> memory and reading ring entries based on that value (ie, we need to
>> >> ensure both loads are done in order by the CPU).
>> >>
>> >> It could be argued that the MMIO reads in r600_ack_irq() might be
>> >> enough to get that barrier but I prefer keeping an explicit one just
>> >> in case.
>> >>
>> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> >
>> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> evergreen.c will need a similar fix.
>
> Ok. I can do that.

Thanks!

>
> Cheers,
> Ben.
>
>> Alex
>>
>> >
>> >> ---
>> >>
>> >> (resent adding dri-devel to the CC list to hit patchwork)
>> >>
>> >> drivers/gpu/drm/radeon/r600.c |    3 +++
>> >>  1 files changed, 3 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
>> >> index 3c86b15..7e5c801 100644
>> >> --- a/drivers/gpu/drm/radeon/r600.c
>> >> +++ b/drivers/gpu/drm/radeon/r600.c
>> >> @@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
>> >>        }
>> >>
>> >>  restart_ih:
>> >> +       /* Order reading of wptr vs. reading of IH ring data */
>> >> +       wmb();
>> >> +
>> >>        /* display interrupts */
>> >>        r600_irq_ack(rdev);
>> >>
>> >>
>> >>
>> >>
>> >>
>> >
>
>
>

Re: [PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Matt Turner]

On Wed, Jul 13, 2011 at 6:28 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> We should have a read memory barrier between reading the WPTR from
> memory and reading ring entries based on that value (ie, we need to
> ensure both loads are done in order by the CPU).
>
> It could be argued that the MMIO reads in r600_ack_irq() might be
> enough to get that barrier but I prefer keeping an explicit one just
> in case.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> (resent adding dri-devel to the CC list to hit patchwork)
>
> drivers/gpu/drm/radeon/r600.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index 3c86b15..7e5c801 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
>        }
>
>  restart_ih:
> +       /* Order reading of wptr vs. reading of IH ring data */
> +       wmb();
> +
>        /* display interrupts */
>        r600_irq_ack(rdev);

The subject line says rmb(), but this says wmb(). Just want to verify
what you have is correct.

Matt

Re: [PATCH 4/6] drm/radeon: Add a rmb() in IH processing

[Benjamin Herrenschmidt]

On Fri, 2011-07-15 at 04:19 +0000, Matt Turner wrote:
> On Wed, Jul 13, 2011 at 6:28 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > We should have a read memory barrier between reading the WPTR from
> > memory and reading ring entries based on that value (ie, we need to
> > ensure both loads are done in order by the CPU).
> >
> > It could be argued that the MMIO reads in r600_ack_irq() might be
> > enough to get that barrier but I prefer keeping an explicit one just
> > in case.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >
> > (resent adding dri-devel to the CC list to hit patchwork)
> >
> > drivers/gpu/drm/radeon/r600.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> > index 3c86b15..7e5c801 100644
> > --- a/drivers/gpu/drm/radeon/r600.c
> > +++ b/drivers/gpu/drm/radeon/r600.c
> > @@ -3312,6 +3312,9 @@ int r600_irq_process(struct radeon_device *rdev)
> >        }
> >
> >  restart_ih:
> > +       /* Order reading of wptr vs. reading of IH ring data */
> > +       wmb();
> > +
> >        /* display interrupts */
> >        r600_irq_ack(rdev);
> 
> The subject line says rmb(), but this says wmb(). Just want to verify
> what you have is correct.

Nice spotting, it's a typo and should have been rmb(). I'll fix it and
respin.

Cheers,
Ben