All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 3/6] drm/radeon: Writeback endian fixes
@ 2011-07-13  6:28 Benjamin Herrenschmidt
  2011-07-13 14:42 ` Alex Deucher
  2011-07-14 15:19 ` Michel Dänzer
  0 siblings, 2 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2011-07-13  6:28 UTC (permalink / raw)
  To: Alex Deucher; +Cc: xorg-driver-ati, dri-devel, Cedric Cano

The writeback ring pointer and IH ring pointer are read using le32_to_cpu
so we do not want the chip to byteswap them on big-endian.

We still want to byteswap the ring itself and the IBs, so we don't touch
that but we remove setting of the byteswap bits in CP_RB_RPTR_ADDR and
IH_CNTL.

In general, for things like that where we control all the accessors easily,
we are better off doing the swap in SW rather than HW. Paradoxally, it does
keep the code closer to x86 and avoid using poorly tested HW features.

I also changed the use of RADEON_ to R600_ in a couple of cases to be more
consistent with the surrounding code.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

(resent adding dri-devel to the CC list to hit patchwork)

 drivers/gpu/drm/radeon/evergreen.c |    3 ---
 drivers/gpu/drm/radeon/r600.c      |    7 -------
 drivers/gpu/drm/radeon/r600_cp.c   |   23 +++++++++--------------
 3 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index e8a5ffb..23cf089 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1359,9 +1359,6 @@ int evergreen_cp_resume(struct radeon_device *rdev)
 
 	/* set the wb address wether it's enabled or not */
 	WREG32(CP_RB_RPTR_ADDR,
-#ifdef __BIG_ENDIAN
-	       RB_RPTR_SWAP(2) |
-#endif
 	       ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
 	WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
 	WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index f79d2cc..3c86b15 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -2212,9 +2212,6 @@ int r600_cp_resume(struct radeon_device *rdev)
 
 	/* set the wb address whether it's enabled or not */
 	WREG32(CP_RB_RPTR_ADDR,
-#ifdef __BIG_ENDIAN
-	       RB_RPTR_SWAP(2) |
-#endif
 	       ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
 	WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
 	WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
@@ -2993,10 +2990,6 @@ int r600_irq_init(struct radeon_device *rdev)
 	/* RPTR_REARM only works if msi's are enabled */
 	if (rdev->msi_enabled)
 		ih_cntl |= RPTR_REARM;
-
-#ifdef __BIG_ENDIAN
-	ih_cntl |= IH_MC_SWAP(IH_MC_SWAP_32BIT);
-#endif
 	WREG32(IH_CNTL, ih_cntl);
 
 	/* force the active interrupt state to all disabled */
diff --git a/drivers/gpu/drm/radeon/r600_cp.c b/drivers/gpu/drm/radeon/r600_cp.c
index c3ab959..45fd592 100644
--- a/drivers/gpu/drm/radeon/r600_cp.c
+++ b/drivers/gpu/drm/radeon/r600_cp.c
@@ -1802,8 +1802,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
 	/* Set ring buffer size */
 #ifdef __BIG_ENDIAN
 	RADEON_WRITE(R600_CP_RB_CNTL,
-		     RADEON_BUF_SWAP_32BIT |
-		     RADEON_RB_NO_UPDATE |
+		     R600_BUF_SWAP_32BIT |
+		     R600_RB_NO_UPDATE |
 		     (dev_priv->ring.rptr_update_l2qw << 8) |
 		     dev_priv->ring.size_l2qw);
 #else
@@ -1820,15 +1820,15 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
 
 #ifdef __BIG_ENDIAN
 	RADEON_WRITE(R600_CP_RB_CNTL,
-		     RADEON_BUF_SWAP_32BIT |
-		     RADEON_RB_NO_UPDATE |
-		     RADEON_RB_RPTR_WR_ENA |
+		     R600_BUF_SWAP_32BIT |
+		     R600_RB_NO_UPDATE |
+		     R600_RB_RPTR_WR_ENA |
 		     (dev_priv->ring.rptr_update_l2qw << 8) |
 		     dev_priv->ring.size_l2qw);
 #else
 	RADEON_WRITE(R600_CP_RB_CNTL,
-		     RADEON_RB_NO_UPDATE |
-		     RADEON_RB_RPTR_WR_ENA |
+		     R600_RB_NO_UPDATE |
+		     R600_RB_RPTR_WR_ENA |
 		     (dev_priv->ring.rptr_update_l2qw << 8) |
 		     dev_priv->ring.size_l2qw);
 #endif
@@ -1851,13 +1851,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
 			- ((unsigned long) dev->sg->virtual)
 			+ dev_priv->gart_vm_start;
 	}
-	RADEON_WRITE(R600_CP_RB_RPTR_ADDR,
-#ifdef __BIG_ENDIAN
-		     (2 << 0) |
-#endif
-		     (rptr_addr & 0xfffffffc));
-	RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI,
-		     upper_32_bits(rptr_addr));
+	RADEON_WRITE(R600_CP_RB_RPTR_ADDR, (rptr_addr & 0xfffffffc));
+	RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI, upper_32_bits(rptr_addr));
 
 #ifdef __BIG_ENDIAN
 	RADEON_WRITE(R600_CP_RB_CNTL,

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] drm/radeon: Writeback endian fixes
  2011-07-13  6:28 [PATCH 3/6] drm/radeon: Writeback endian fixes Benjamin Herrenschmidt
@ 2011-07-13 14:42 ` Alex Deucher
       [not found]   ` <CADnq5_M2v5CdVNxGqsTfKYT37=p2A5dQ3zcP9NkH42rOVBHDEQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2011-07-14 15:19 ` Michel Dänzer
  1 sibling, 1 reply; 6+ messages in thread
From: Alex Deucher @ 2011-07-13 14:42 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: xorg-driver-ati, dri-devel, Cedric Cano

On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> The writeback ring pointer and IH ring pointer are read using le32_to_cpu
> so we do not want the chip to byteswap them on big-endian.
>
> We still want to byteswap the ring itself and the IBs, so we don't touch
> that but we remove setting of the byteswap bits in CP_RB_RPTR_ADDR and
> IH_CNTL.
>
> In general, for things like that where we control all the accessors easily,
> we are better off doing the swap in SW rather than HW. Paradoxally, it does
> keep the code closer to x86 and avoid using poorly tested HW features.
>
> I also changed the use of RADEON_ to R600_ in a couple of cases to be more
> consistent with the surrounding code.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

evergreen.c and ni.c will need similar fixes.

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

> ---
>
> (resent adding dri-devel to the CC list to hit patchwork)
>
>  drivers/gpu/drm/radeon/evergreen.c |    3 ---
>  drivers/gpu/drm/radeon/r600.c      |    7 -------
>  drivers/gpu/drm/radeon/r600_cp.c   |   23 +++++++++--------------
>  3 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index e8a5ffb..23cf089 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -1359,9 +1359,6 @@ int evergreen_cp_resume(struct radeon_device *rdev)
>
>        /* set the wb address wether it's enabled or not */
>        WREG32(CP_RB_RPTR_ADDR,
> -#ifdef __BIG_ENDIAN
> -              RB_RPTR_SWAP(2) |
> -#endif
>               ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
>        WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
>        WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index f79d2cc..3c86b15 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -2212,9 +2212,6 @@ int r600_cp_resume(struct radeon_device *rdev)
>
>        /* set the wb address whether it's enabled or not */
>        WREG32(CP_RB_RPTR_ADDR,
> -#ifdef __BIG_ENDIAN
> -              RB_RPTR_SWAP(2) |
> -#endif
>               ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
>        WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
>        WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
> @@ -2993,10 +2990,6 @@ int r600_irq_init(struct radeon_device *rdev)
>        /* RPTR_REARM only works if msi's are enabled */
>        if (rdev->msi_enabled)
>                ih_cntl |= RPTR_REARM;
> -
> -#ifdef __BIG_ENDIAN
> -       ih_cntl |= IH_MC_SWAP(IH_MC_SWAP_32BIT);
> -#endif
>        WREG32(IH_CNTL, ih_cntl);
>
>        /* force the active interrupt state to all disabled */
> diff --git a/drivers/gpu/drm/radeon/r600_cp.c b/drivers/gpu/drm/radeon/r600_cp.c
> index c3ab959..45fd592 100644
> --- a/drivers/gpu/drm/radeon/r600_cp.c
> +++ b/drivers/gpu/drm/radeon/r600_cp.c
> @@ -1802,8 +1802,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
>        /* Set ring buffer size */
>  #ifdef __BIG_ENDIAN
>        RADEON_WRITE(R600_CP_RB_CNTL,
> -                    RADEON_BUF_SWAP_32BIT |
> -                    RADEON_RB_NO_UPDATE |
> +                    R600_BUF_SWAP_32BIT |
> +                    R600_RB_NO_UPDATE |
>                     (dev_priv->ring.rptr_update_l2qw << 8) |
>                     dev_priv->ring.size_l2qw);
>  #else
> @@ -1820,15 +1820,15 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
>
>  #ifdef __BIG_ENDIAN
>        RADEON_WRITE(R600_CP_RB_CNTL,
> -                    RADEON_BUF_SWAP_32BIT |
> -                    RADEON_RB_NO_UPDATE |
> -                    RADEON_RB_RPTR_WR_ENA |
> +                    R600_BUF_SWAP_32BIT |
> +                    R600_RB_NO_UPDATE |
> +                    R600_RB_RPTR_WR_ENA |
>                     (dev_priv->ring.rptr_update_l2qw << 8) |
>                     dev_priv->ring.size_l2qw);
>  #else
>        RADEON_WRITE(R600_CP_RB_CNTL,
> -                    RADEON_RB_NO_UPDATE |
> -                    RADEON_RB_RPTR_WR_ENA |
> +                    R600_RB_NO_UPDATE |
> +                    R600_RB_RPTR_WR_ENA |
>                     (dev_priv->ring.rptr_update_l2qw << 8) |
>                     dev_priv->ring.size_l2qw);
>  #endif
> @@ -1851,13 +1851,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
>                        - ((unsigned long) dev->sg->virtual)
>                        + dev_priv->gart_vm_start;
>        }
> -       RADEON_WRITE(R600_CP_RB_RPTR_ADDR,
> -#ifdef __BIG_ENDIAN
> -                    (2 << 0) |
> -#endif
> -                    (rptr_addr & 0xfffffffc));
> -       RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI,
> -                    upper_32_bits(rptr_addr));
> +       RADEON_WRITE(R600_CP_RB_RPTR_ADDR, (rptr_addr & 0xfffffffc));
> +       RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI, upper_32_bits(rptr_addr));
>
>  #ifdef __BIG_ENDIAN
>        RADEON_WRITE(R600_CP_RB_CNTL,
>
>
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] drm/radeon: Writeback endian fixes
       [not found]   ` <CADnq5_M2v5CdVNxGqsTfKYT37=p2A5dQ3zcP9NkH42rOVBHDEQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-07-13 21:41     ` Benjamin Herrenschmidt
  2011-07-14  2:24       ` Alex Deucher
  0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2011-07-13 21:41 UTC (permalink / raw)
  To: Alex Deucher
  Cc: xorg-driver-ati-go0+a7rfsptAfugRpC6u6w,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Cedric Cano

On Wed, 2011-07-13 at 10:42 -0400, Alex Deucher wrote:
> On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
> <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> > The writeback ring pointer and IH ring pointer are read using le32_to_cpu
> > so we do not want the chip to byteswap them on big-endian.
> >
> > We still want to byteswap the ring itself and the IBs, so we don't touch
> > that but we remove setting of the byteswap bits in CP_RB_RPTR_ADDR and
> > IH_CNTL.
> >
> > In general, for things like that where we control all the accessors easily,
> > we are better off doing the swap in SW rather than HW. Paradoxally, it does
> > keep the code closer to x86 and avoid using poorly tested HW features.
> >
> > I also changed the use of RADEON_ to R600_ in a couple of cases to be more
> > consistent with the surrounding code.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> 
> evergreen.c and ni.c will need similar fixes.

evergreen.c -is- fixed in the patch :-) ni.c doesn't seem to set
swapping on the write back of the ring pointer (can you dbl check ?), it
only enables swapping on the ring itself which is correct as far as I
can tell.

Cheers,
Ben.

> Reviewed-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>
> 
> > ---
> >
> > (resent adding dri-devel to the CC list to hit patchwork)
> >
> >  drivers/gpu/drm/radeon/evergreen.c |    3 ---
> >  drivers/gpu/drm/radeon/r600.c      |    7 -------
> >  drivers/gpu/drm/radeon/r600_cp.c   |   23 +++++++++--------------
> >  3 files changed, 9 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> > index e8a5ffb..23cf089 100644
> > --- a/drivers/gpu/drm/radeon/evergreen.c
> > +++ b/drivers/gpu/drm/radeon/evergreen.c
> > @@ -1359,9 +1359,6 @@ int evergreen_cp_resume(struct radeon_device *rdev)
> >
> >        /* set the wb address wether it's enabled or not */
> >        WREG32(CP_RB_RPTR_ADDR,
> > -#ifdef __BIG_ENDIAN
> > -              RB_RPTR_SWAP(2) |
> > -#endif
> >               ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
> >        WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
> >        WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
> > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> > index f79d2cc..3c86b15 100644
> > --- a/drivers/gpu/drm/radeon/r600.c
> > +++ b/drivers/gpu/drm/radeon/r600.c
> > @@ -2212,9 +2212,6 @@ int r600_cp_resume(struct radeon_device *rdev)
> >
> >        /* set the wb address whether it's enabled or not */
> >        WREG32(CP_RB_RPTR_ADDR,
> > -#ifdef __BIG_ENDIAN
> > -              RB_RPTR_SWAP(2) |
> > -#endif
> >               ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
> >        WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
> >        WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
> > @@ -2993,10 +2990,6 @@ int r600_irq_init(struct radeon_device *rdev)
> >        /* RPTR_REARM only works if msi's are enabled */
> >        if (rdev->msi_enabled)
> >                ih_cntl |= RPTR_REARM;
> > -
> > -#ifdef __BIG_ENDIAN
> > -       ih_cntl |= IH_MC_SWAP(IH_MC_SWAP_32BIT);
> > -#endif
> >        WREG32(IH_CNTL, ih_cntl);
> >
> >        /* force the active interrupt state to all disabled */
> > diff --git a/drivers/gpu/drm/radeon/r600_cp.c b/drivers/gpu/drm/radeon/r600_cp.c
> > index c3ab959..45fd592 100644
> > --- a/drivers/gpu/drm/radeon/r600_cp.c
> > +++ b/drivers/gpu/drm/radeon/r600_cp.c
> > @@ -1802,8 +1802,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
> >        /* Set ring buffer size */
> >  #ifdef __BIG_ENDIAN
> >        RADEON_WRITE(R600_CP_RB_CNTL,
> > -                    RADEON_BUF_SWAP_32BIT |
> > -                    RADEON_RB_NO_UPDATE |
> > +                    R600_BUF_SWAP_32BIT |
> > +                    R600_RB_NO_UPDATE |
> >                     (dev_priv->ring.rptr_update_l2qw << 8) |
> >                     dev_priv->ring.size_l2qw);
> >  #else
> > @@ -1820,15 +1820,15 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
> >
> >  #ifdef __BIG_ENDIAN
> >        RADEON_WRITE(R600_CP_RB_CNTL,
> > -                    RADEON_BUF_SWAP_32BIT |
> > -                    RADEON_RB_NO_UPDATE |
> > -                    RADEON_RB_RPTR_WR_ENA |
> > +                    R600_BUF_SWAP_32BIT |
> > +                    R600_RB_NO_UPDATE |
> > +                    R600_RB_RPTR_WR_ENA |
> >                     (dev_priv->ring.rptr_update_l2qw << 8) |
> >                     dev_priv->ring.size_l2qw);
> >  #else
> >        RADEON_WRITE(R600_CP_RB_CNTL,
> > -                    RADEON_RB_NO_UPDATE |
> > -                    RADEON_RB_RPTR_WR_ENA |
> > +                    R600_RB_NO_UPDATE |
> > +                    R600_RB_RPTR_WR_ENA |
> >                     (dev_priv->ring.rptr_update_l2qw << 8) |
> >                     dev_priv->ring.size_l2qw);
> >  #endif
> > @@ -1851,13 +1851,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
> >                        - ((unsigned long) dev->sg->virtual)
> >                        + dev_priv->gart_vm_start;
> >        }
> > -       RADEON_WRITE(R600_CP_RB_RPTR_ADDR,
> > -#ifdef __BIG_ENDIAN
> > -                    (2 << 0) |
> > -#endif
> > -                    (rptr_addr & 0xfffffffc));
> > -       RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI,
> > -                    upper_32_bits(rptr_addr));
> > +       RADEON_WRITE(R600_CP_RB_RPTR_ADDR, (rptr_addr & 0xfffffffc));
> > +       RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI, upper_32_bits(rptr_addr));
> >
> >  #ifdef __BIG_ENDIAN
> >        RADEON_WRITE(R600_CP_RB_CNTL,
> >
> >
> >
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] drm/radeon: Writeback endian fixes
  2011-07-13 21:41     ` Benjamin Herrenschmidt
@ 2011-07-14  2:24       ` Alex Deucher
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2011-07-14  2:24 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: xorg-driver-ati, dri-devel, Cedric Cano

On Wed, Jul 13, 2011 at 5:41 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2011-07-13 at 10:42 -0400, Alex Deucher wrote:
>> On Wed, Jul 13, 2011 at 2:28 AM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>> > The writeback ring pointer and IH ring pointer are read using le32_to_cpu
>> > so we do not want the chip to byteswap them on big-endian.
>> >
>> > We still want to byteswap the ring itself and the IBs, so we don't touch
>> > that but we remove setting of the byteswap bits in CP_RB_RPTR_ADDR and
>> > IH_CNTL.
>> >
>> > In general, for things like that where we control all the accessors easily,
>> > we are better off doing the swap in SW rather than HW. Paradoxally, it does
>> > keep the code closer to x86 and avoid using poorly tested HW features.
>> >
>> > I also changed the use of RADEON_ to R600_ in a couple of cases to be more
>> > consistent with the surrounding code.
>> >
>> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> evergreen.c and ni.c will need similar fixes.
>
> evergreen.c -is- fixed in the patch :-) ni.c doesn't seem to set
> swapping on the write back of the ring pointer (can you dbl check ?), it
> only enables swapping on the ring itself which is correct as far as I
> can tell.

You're right.  Looks good.

Alex


>
> Cheers,
> Ben.
>
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> > ---
>> >
>> > (resent adding dri-devel to the CC list to hit patchwork)
>> >
>> >  drivers/gpu/drm/radeon/evergreen.c |    3 ---
>> >  drivers/gpu/drm/radeon/r600.c      |    7 -------
>> >  drivers/gpu/drm/radeon/r600_cp.c   |   23 +++++++++--------------
>> >  3 files changed, 9 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>> > index e8a5ffb..23cf089 100644
>> > --- a/drivers/gpu/drm/radeon/evergreen.c
>> > +++ b/drivers/gpu/drm/radeon/evergreen.c
>> > @@ -1359,9 +1359,6 @@ int evergreen_cp_resume(struct radeon_device *rdev)
>> >
>> >        /* set the wb address wether it's enabled or not */
>> >        WREG32(CP_RB_RPTR_ADDR,
>> > -#ifdef __BIG_ENDIAN
>> > -              RB_RPTR_SWAP(2) |
>> > -#endif
>> >               ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
>> >        WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
>> >        WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
>> > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
>> > index f79d2cc..3c86b15 100644
>> > --- a/drivers/gpu/drm/radeon/r600.c
>> > +++ b/drivers/gpu/drm/radeon/r600.c
>> > @@ -2212,9 +2212,6 @@ int r600_cp_resume(struct radeon_device *rdev)
>> >
>> >        /* set the wb address whether it's enabled or not */
>> >        WREG32(CP_RB_RPTR_ADDR,
>> > -#ifdef __BIG_ENDIAN
>> > -              RB_RPTR_SWAP(2) |
>> > -#endif
>> >               ((rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFFFFFFFC));
>> >        WREG32(CP_RB_RPTR_ADDR_HI, upper_32_bits(rdev->wb.gpu_addr + RADEON_WB_CP_RPTR_OFFSET) & 0xFF);
>> >        WREG32(SCRATCH_ADDR, ((rdev->wb.gpu_addr + RADEON_WB_SCRATCH_OFFSET) >> 8) & 0xFFFFFFFF);
>> > @@ -2993,10 +2990,6 @@ int r600_irq_init(struct radeon_device *rdev)
>> >        /* RPTR_REARM only works if msi's are enabled */
>> >        if (rdev->msi_enabled)
>> >                ih_cntl |= RPTR_REARM;
>> > -
>> > -#ifdef __BIG_ENDIAN
>> > -       ih_cntl |= IH_MC_SWAP(IH_MC_SWAP_32BIT);
>> > -#endif
>> >        WREG32(IH_CNTL, ih_cntl);
>> >
>> >        /* force the active interrupt state to all disabled */
>> > diff --git a/drivers/gpu/drm/radeon/r600_cp.c b/drivers/gpu/drm/radeon/r600_cp.c
>> > index c3ab959..45fd592 100644
>> > --- a/drivers/gpu/drm/radeon/r600_cp.c
>> > +++ b/drivers/gpu/drm/radeon/r600_cp.c
>> > @@ -1802,8 +1802,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
>> >        /* Set ring buffer size */
>> >  #ifdef __BIG_ENDIAN
>> >        RADEON_WRITE(R600_CP_RB_CNTL,
>> > -                    RADEON_BUF_SWAP_32BIT |
>> > -                    RADEON_RB_NO_UPDATE |
>> > +                    R600_BUF_SWAP_32BIT |
>> > +                    R600_RB_NO_UPDATE |
>> >                     (dev_priv->ring.rptr_update_l2qw << 8) |
>> >                     dev_priv->ring.size_l2qw);
>> >  #else
>> > @@ -1820,15 +1820,15 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
>> >
>> >  #ifdef __BIG_ENDIAN
>> >        RADEON_WRITE(R600_CP_RB_CNTL,
>> > -                    RADEON_BUF_SWAP_32BIT |
>> > -                    RADEON_RB_NO_UPDATE |
>> > -                    RADEON_RB_RPTR_WR_ENA |
>> > +                    R600_BUF_SWAP_32BIT |
>> > +                    R600_RB_NO_UPDATE |
>> > +                    R600_RB_RPTR_WR_ENA |
>> >                     (dev_priv->ring.rptr_update_l2qw << 8) |
>> >                     dev_priv->ring.size_l2qw);
>> >  #else
>> >        RADEON_WRITE(R600_CP_RB_CNTL,
>> > -                    RADEON_RB_NO_UPDATE |
>> > -                    RADEON_RB_RPTR_WR_ENA |
>> > +                    R600_RB_NO_UPDATE |
>> > +                    R600_RB_RPTR_WR_ENA |
>> >                     (dev_priv->ring.rptr_update_l2qw << 8) |
>> >                     dev_priv->ring.size_l2qw);
>> >  #endif
>> > @@ -1851,13 +1851,8 @@ static void r600_cp_init_ring_buffer(struct drm_device *dev,
>> >                        - ((unsigned long) dev->sg->virtual)
>> >                        + dev_priv->gart_vm_start;
>> >        }
>> > -       RADEON_WRITE(R600_CP_RB_RPTR_ADDR,
>> > -#ifdef __BIG_ENDIAN
>> > -                    (2 << 0) |
>> > -#endif
>> > -                    (rptr_addr & 0xfffffffc));
>> > -       RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI,
>> > -                    upper_32_bits(rptr_addr));
>> > +       RADEON_WRITE(R600_CP_RB_RPTR_ADDR, (rptr_addr & 0xfffffffc));
>> > +       RADEON_WRITE(R600_CP_RB_RPTR_ADDR_HI, upper_32_bits(rptr_addr));
>> >
>> >  #ifdef __BIG_ENDIAN
>> >        RADEON_WRITE(R600_CP_RB_CNTL,
>> >
>> >
>> >
>> >
>
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] drm/radeon: Writeback endian fixes
  2011-07-13  6:28 [PATCH 3/6] drm/radeon: Writeback endian fixes Benjamin Herrenschmidt
  2011-07-13 14:42 ` Alex Deucher
@ 2011-07-14 15:19 ` Michel Dänzer
  2011-07-14 15:34   ` Benjamin Herrenschmidt
  1 sibling, 1 reply; 6+ messages in thread
From: Michel Dänzer @ 2011-07-14 15:19 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: xorg-driver-ati, Cedric Cano, dri-devel

On Mit, 2011-07-13 at 16:28 +1000, Benjamin Herrenschmidt wrote:
> The writeback ring pointer and IH ring pointer are read using le32_to_cpu
> so we do not want the chip to byteswap them on big-endian.
> 
> We still want to byteswap the ring itself and the IBs, so we don't touch
> that but we remove setting of the byteswap bits in CP_RB_RPTR_ADDR and
> IH_CNTL.
> 
> In general, for things like that where we control all the accessors easily,
> we are better off doing the swap in SW rather than HW. Paradoxally, it does
> keep the code closer to x86 and avoid using poorly tested HW features.

Absolutely. Unfortunately, when I fixed the CP writeback code to use
le32_to_cpu(), I didn't realize the code for some GPU families was
already using HW swappers for this. 


> I also changed the use of RADEON_ to R600_ in a couple of cases to be more
> consistent with the surrounding code.

That should probably be in a separate patch. Either way, though:

Reviewed-by: Michel Dänzer <michel@daenzer.net>


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 3/6] drm/radeon: Writeback endian fixes
  2011-07-14 15:19 ` Michel Dänzer
@ 2011-07-14 15:34   ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2011-07-14 15:34 UTC (permalink / raw)
  To: Michel Dänzer; +Cc: xorg-driver-ati, Cedric Cano, dri-devel

On Thu, 2011-07-14 at 17:19 +0200, Michel Dänzer wrote:
> On Mit, 2011-07-13 at 16:28 +1000, Benjamin Herrenschmidt wrote:
> > The writeback ring pointer and IH ring pointer are read using le32_to_cpu
> > so we do not want the chip to byteswap them on big-endian.
> > 
> > We still want to byteswap the ring itself and the IBs, so we don't touch
> > that but we remove setting of the byteswap bits in CP_RB_RPTR_ADDR and
> > IH_CNTL.
> > 
> > In general, for things like that where we control all the accessors easily,
> > we are better off doing the swap in SW rather than HW. Paradoxally, it does
> > keep the code closer to x86 and avoid using poorly tested HW features.
> 
> Absolutely. Unfortunately, when I fixed the CP writeback code to use
> le32_to_cpu(), I didn't realize the code for some GPU families was
> already using HW swappers for this. 
> 
> 
> > I also changed the use of RADEON_ to R600_ in a couple of cases to be more
> > consistent with the surrounding code.
> 
> That should probably be in a separate patch. Either way, though:

I thought about it and decided it was trivial enough not to bother
re-doing the patches.

Alex/Dave/whoever's in charge, feel free to apply the current batch,
I'll send further cleanups/fixes as separate patches, possibly not
before next week or so.

Cheers,
Ben.

> Reviewed-by: Michel Dänzer <michel@daenzer.net>



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-14 15:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-13  6:28 [PATCH 3/6] drm/radeon: Writeback endian fixes Benjamin Herrenschmidt
2011-07-13 14:42 ` Alex Deucher
     [not found]   ` <CADnq5_M2v5CdVNxGqsTfKYT37=p2A5dQ3zcP9NkH42rOVBHDEQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-07-13 21:41     ` Benjamin Herrenschmidt
2011-07-14  2:24       ` Alex Deucher
2011-07-14 15:19 ` Michel Dänzer
2011-07-14 15:34   ` Benjamin Herrenschmidt

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.