Hi, this is continuation for the MMIO accessor rewrite and cleanup. I am currently running nv28 with these patches applied, but I cannot test on PPC. Please, review and comment. If the direction is good, I'll do the same to INSTANCE_{RD,WR} as I did for nv_{rd,wr}32, and change PRAMIN from drm_local_map to simple ioremap. Can the same be done for channel specific mappings, that is nvchan_{rd,wr}32? The macros nv_{in,out}32 are very close to removal now. It also looks like nouveau_load() is in need of serious error path review for resource leakage. And maybe refactorization, too, into small functions. - pq
Pekka Paalanen
2009-Jul-25 12:29 UTC
[Nouveau] [PATCH 1/4] drm/nouveau: change MMIO map to simple ioremap
This removes the ugliness of using drm_local_map. The use of ioremap() is identical to what drm_addmap()/drm_rmmap() did. Is ioremap() the right thing or should we use e.g. ioremap_uc()? The ioremap API has been reworked in the past, and some assumptions changed in the mapping caching type. Signed-off-by: Pekka Paalanen <pq at iki.fi> --- drivers/gpu/drm/nouveau/nouveau_drv.h | 14 +++++++------- drivers/gpu/drm/nouveau/nouveau_state.c | 21 +++++++++++---------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h index cba3b3b..90802dc 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h @@ -447,7 +447,7 @@ struct drm_nouveau_private { int chipset; int flags; - struct drm_local_map *mmio; + void __iomem *mmio; struct drm_local_map *fb; struct drm_local_map *ramin; @@ -975,38 +975,38 @@ extern int nouveau_gem_ioctl_info(struct drm_device *, void *, static inline u32 nv_rd32(struct drm_device *dev, unsigned reg) { struct drm_nouveau_private *dev_priv = dev->dev_private; - return in_be32((void __force __iomem *)dev_priv->mmio->handle + reg); + return in_be32(dev_priv->mmio + reg); } static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val) { struct drm_nouveau_private *dev_priv = dev->dev_private; - out_be32((void __force __iomem *)dev_priv->mmio->handle + reg, val); + out_be32(dev_priv->mmio + reg, val); } #else static inline u32 nv_rd32(struct drm_device *dev, unsigned reg) { struct drm_nouveau_private *dev_priv = dev->dev_private; - return readl((void __force __iomem *)dev_priv->mmio->handle + reg); + return readl(dev_priv->mmio + reg); } static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val) { struct drm_nouveau_private *dev_priv = dev->dev_private; - writel(val, (void __force __iomem *)dev_priv->mmio->handle + reg); + writel(val, dev_priv->mmio + reg); } #endif /* not __powerpc__ */ static inline u8 nv_rd08(struct drm_device *dev, unsigned reg) { struct drm_nouveau_private *dev_priv = dev->dev_private; - return readb((void __force __iomem *)dev_priv->mmio->handle + reg); + return readb(dev_priv->mmio + reg); } static inline void nv_wr08(struct drm_device *dev, unsigned reg, u8 val) { struct drm_nouveau_private *dev_priv = dev->dev_private; - writeb(val, (void __force __iomem *)dev_priv->mmio->handle + reg); + writeb(val, dev_priv->mmio + reg); } #define nv_wait(reg,mask,val) nouveau_wait_until(dev, 2000000000ULL, (reg), \ diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c index ae3d2d5..911dc7e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_state.c +++ b/drivers/gpu/drm/nouveau/nouveau_state.c @@ -454,6 +454,7 @@ int nouveau_load(struct drm_device *dev, unsigned long flags) #endif uint32_t reg0; uint8_t architecture = 0; + resource_size_t mmio_start_offs; int ret; dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL); @@ -474,16 +475,15 @@ int nouveau_load(struct drm_device *dev, unsigned long flags) /* resource 6 is bios */ /* map the mmio regs */ - ret = drm_addmap(dev, drm_get_resource_start(dev, 0), - 0x00800000, _DRM_REGISTERS, _DRM_READ_ONLY | - _DRM_DRIVER, &dev_priv->mmio); - if (ret) { - NV_ERROR(dev, "Unable to initialize the mmio mapping (%d). " - "Please report your setup to " DRIVER_EMAIL "\n", - ret); + mmio_start_offs = pci_resource_start(dev->pdev, 0); + dev_priv->mmio = ioremap(mmio_start_offs, 0x00800000); + if (!dev_priv->mmio) { + NV_ERROR(dev, "Unable to initialize the mmio mapping. " + "Please report your setup to " DRIVER_EMAIL "\n"); return -EINVAL; } - NV_DEBUG(dev, "regs mapped ok at 0x%lx\n", (unsigned long)dev_priv->mmio->offset); + NV_DEBUG(dev, "regs mapped ok at 0x%llx\n", + (unsigned long long)mmio_start_offs); #if defined(__powerpc__) /* Put the card in BE mode if it's not */ @@ -538,7 +538,8 @@ int nouveau_load(struct drm_device *dev, unsigned long flags) dev_priv->card_type = NV_UNKNOWN; } - NV_INFO(dev, "Detected an NV%d generation card (0x%08x)\n", dev_priv->card_type,reg0); + NV_INFO(dev, "Detected an NV%d generation card (0x%08x)\n", + dev_priv->card_type, reg0); if (dev_priv->card_type == NV_UNKNOWN) { return -EINVAL; @@ -653,7 +654,7 @@ int nouveau_unload(struct drm_device *dev) nouveau_close(dev); } - drm_rmmap(dev, dev_priv->mmio); + iounmap(dev_priv->mmio); drm_rmmap(dev, dev_priv->ramin); drm_rmmap(dev, dev_priv->fb); -- 1.6.3.3
Pekka Paalanen
2009-Jul-25 12:29 UTC
[Nouveau] [PATCH 2/4] drm/nouveau: use proper IO accessors on PPC
Use the generic io{read,write}32be() functions on big-endian instead of the PPC-specific private {in,out}_be32(), which are not meant to be used with PCI devices. While we are at it, change all #ifdef __powerpc__ that are really just for endianness into #ifdef __BIG_ENDIAN, and clean up a bit of style. On Wed, 22 Jul 2009 00:01:59 +0200 Arnd Bergmann <arnd at arndb.de> wrote:> The powerpc in_le32 style functions are a completely different > story, they are basically defined to operate only on on-chip > components, while ioread32 and readl do work on PCI devices.On Wed, 22 Jul 2009 23:20:58 +0200 Arnd Bergmann <arnd at arndb.de> wrote:> If it's a PCI/AGP/PCIe device, use iowrite32be(), otherwise it > may not work correctly on a pseries, celleb or qs20 machine. > > If it's connected over a different bus (IOIF on PS3), out_be32 > would be more appropriate, but AFAICT, iowrite32be should work > just as well.Signed-off-by: Pekka Paalanen <pq at iki.fi> --- drivers/gpu/drm/nouveau/nouveau_drv.h | 39 +++++++++++++++++------------- drivers/gpu/drm/nouveau/nouveau_state.c | 2 +- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h index 90802dc..7a37559 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h @@ -948,40 +948,45 @@ extern int nouveau_gem_ioctl_info(struct drm_device *, void *, struct drm_file *); #define NVDEV ((struct drm_nouveau_private *)dev->dev_private) -#if defined(__powerpc__) -#define nv_out32(map,reg,val) out_be32((void __iomem *)NVDEV->map->handle + (reg), (val)) -#define nv_out16(map,reg,val) out_be16((void __iomem *)NVDEV->map->handle + (reg), (val)) -#define nv_in32(map,reg) in_be32((void __iomem *)NVDEV->map->handle + (reg)) -#define nv_in16(map,reg) in_be16((void __iomem *)NVDEV->map->handle + (reg)) +#ifdef __BIG_ENDIAN +#define nv_out32(map, reg, val) \ + iowrite32be((val), (void __iomem *)NVDEV->map->handle + (reg)) +#define nv_out16(map, reg, val) \ + iowrite16be((val), (void __iomem *)NVDEV->map->handle + (reg)) +#define nv_in32(map, reg) \ + ioread32be((void __iomem *)NVDEV->map->handle + (reg)) +#define nv_in16(map, reg) \ + ioread16be((void __iomem *)NVDEV->map->handle + (reg)) #else -#define nv_out32(map,reg,val) DRM_WRITE32(NVDEV->map, (reg), (val)) -#define nv_out16(map,reg,val) DRM_WRITE16(NVDEV->map, (reg), (val)) -#define nv_in32(map,reg) DRM_READ32(NVDEV->map, (reg)) -#define nv_in16(map,reg) DRM_READ16(NVDEV->map, (reg)) +#define nv_out32(map, reg, val) DRM_WRITE32(NVDEV->map, (reg), (val)) +#define nv_out16(map, reg, val) DRM_WRITE16(NVDEV->map, (reg), (val)) +#define nv_in32(map, reg) DRM_READ32(NVDEV->map, (reg)) +#define nv_in16(map, reg) DRM_READ16(NVDEV->map, (reg)) #endif /* channel control reg access */ -#if defined(__powerpc__) -#define nvchan_wr32(reg,val) out_be32((void __iomem *)chan->user->handle + (reg), (val)) -#define nvchan_rd32(reg) in_be32((void __iomem *)chan->user->handle + (reg)) +#ifdef __BIG_ENDIAN +#define nvchan_wr32(reg, val) \ + iowrite32be((val), (void __iomem *)chan->user->handle + (reg)) +#define nvchan_rd32(reg) ioread32be((void __iomem *)chan->user->handle + (reg)) #else -#define nvchan_wr32(reg,val) DRM_WRITE32(chan->user, (reg), (val)) +#define nvchan_wr32(reg, val) DRM_WRITE32(chan->user, (reg), (val)) #define nvchan_rd32(reg) DRM_READ32(chan->user, (reg)) #endif /* register access */ -#if defined(__powerpc__) +#ifdef __BIG_ENDIAN static inline u32 nv_rd32(struct drm_device *dev, unsigned reg) { struct drm_nouveau_private *dev_priv = dev->dev_private; - return in_be32(dev_priv->mmio + reg); + return ioread32be(dev_priv->mmio + reg); } static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val) { struct drm_nouveau_private *dev_priv = dev->dev_private; - out_be32(dev_priv->mmio + reg, val); + iowrite32be(val, dev_priv->mmio + reg); } #else static inline u32 nv_rd32(struct drm_device *dev, unsigned reg) @@ -995,7 +1000,7 @@ static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val) struct drm_nouveau_private *dev_priv = dev->dev_private; writel(val, dev_priv->mmio + reg); } -#endif /* not __powerpc__ */ +#endif /* not __BIG_ENDIAN */ static inline u8 nv_rd08(struct drm_device *dev, unsigned reg) { diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c index 911dc7e..b70ec33 100644 --- a/drivers/gpu/drm/nouveau/nouveau_state.c +++ b/drivers/gpu/drm/nouveau/nouveau_state.c @@ -485,7 +485,7 @@ int nouveau_load(struct drm_device *dev, unsigned long flags) NV_DEBUG(dev, "regs mapped ok at 0x%llx\n", (unsigned long long)mmio_start_offs); -#if defined(__powerpc__) +#ifdef __BIG_ENDIAN /* Put the card in BE mode if it's not */ if (nv_rd32(dev, NV03_PMC_BOOT_1)) nv_wr32(dev, NV03_PMC_BOOT_1, 0x00000001); -- 1.6.3.3
Possibly Parallel Threads
- [PATCH] nouveau: safen up nouveau_device list usage against concurrent access
- [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.
- [PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests
- [PATCH v2 1/4] Add atomic_inc_return to atomics.
- [libdrm v2 01/14] nouveau: import and install a selection of nvif headers from the kernel