-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Eric Anholt wrote:
> The diff is about +190, -390 lines. Anyone want to review it first,
> since I have a tendency to under-test on linux?
I've looked over the changes, and I'm generally happy with them. There
are a few comments below. I'm pretty much always happy to see lines of
code go away. :) I should be able to run-test it on MGA on x86 and
PowerPC Linux tomorrow.
> Index: shared-core/mga_dma.c
> ===================================================================
> RCS file: /cvs/dri/drm/shared-core/mga_dma.c,v
> retrieving revision 1.26
> diff -u -r1.26 mga_dma.c
> --- shared-core/mga_dma.c 17 Jun 2005 09:09:17 -0000 1.26
> +++ shared-core/mga_dma.c 26 Jun 2005 22:57:10 -0000
> @@ -553,44 +553,6 @@
> }
>
> /**
> - * Create a "fake" drm_map_t for a pre-mapped range of PCI consistent memory.
> - *
> - * Unlike \c drm_addmap, this function just creates a \c drm_map_t wrapper
> for
> - * a block of PCI consistent memory. \c drm_addmap, basically, converts a
> bus
> - * address to a virtual address. However, \c drm_pci_alloc gives both the
> bus
> - * address and the virtual address for the memory region. Not only is there
> - * no need to map it again, but mapping it again will cause problems.
> - *
> - * \param dmah DRM DMA handle returned by \c drm_pci_alloc.
> - * \param map_ptr Location to store a pointer to the \c drm_map_t.
> - *
> - * \returns
> - * On success, zero is returned. Otherwise and error code suitable for
> - * returning from an ioctl is returned.
> - */
> -static int mga_fake_addmap(drm_dma_handle_t * dmah, drm_map_t ** map_ptr)
> -{
> - drm_map_t * map;
> -
> -
> - map = drm_alloc(sizeof(drm_map_t), DRM_MEM_DRIVER);
> - if (map == NULL) {
> - return DRM_ERR(ENOMEM);
> - }
> -
> - map->offset = dmah->busaddr;
> - map->size = dmah->size;
> - map->type = _DRM_CONSISTENT;
> - map->flags = _DRM_READ_ONLY;
> - map->handle = dmah->vaddr;
> - map->mtrr = 0;
> -
> - *map_ptr = map;
> -
> - return 0;
> -}
> -
> -/**
> * Bootstrap the driver for PCI DMA.
> *
> * \todo
> @@ -620,52 +582,41 @@
> return DRM_ERR(EFAULT);
> }
>
> -
> - /* The WARP microcode base address must be 256-byte aligned.
> - */
> - dev_priv->warp_dmah = drm_pci_alloc(dev, warp_size, 0x100, 0x7fffffff);
> - err = mga_fake_addmap(dev_priv->warp_dmah, & dev_priv->warp);
> - if (err) {
> - DRM_ERROR("Unable to map WARP microcode\n");
> + /* The proper alignment is 0x100 for this mapping */
> + err = drm_addmap(dev, 0, warp_size, _DRM_CONSISTENT,
> + _DRM_READ_ONLY, &dev_priv->warp);
> + if (err != 0) {
> + DRM_ERROR("Unable to create mapping for WARP microcode\n");
> return err;
> }
Does this maintain the 256-byte alignment requirement for the WARP
microcode?
>
> -
> /* Other than the bottom two bits being used to encode other
> * information, there don't appear to be any restrictions on the
> * alignment of the primary or secondary DMA buffers.
> */
>
> - dev_priv->primary_dmah = NULL;
> for ( primary_size = dma_bs->primary_size
> ; primary_size != 0
> ; primary_size >>= 1 ) {
> - dev_priv->primary_dmah = drm_pci_alloc(dev, primary_size,
> - 0x04, 0x7fffffff);
> - if (dev_priv->primary_dmah != NULL) {
> + /* The proper alignment for this mapping is 0x04 */
> + err = drm_addmap(dev, 0, primary_size, _DRM_CONSISTENT,
> + _DRM_READ_ONLY, &dev_priv->primary);
> + if (!err)
> break;
> - }
> }
>
> - if (dev_priv->primary_dmah == NULL) {
> + if (err != 0) {
> DRM_ERROR("Unable to allocate primary DMA region\n");
> return DRM_ERR(ENOMEM);
> }
>
> - if (dev_priv->primary_dmah->size != dma_bs->primary_size) {
> + if (dev_priv->primary->size != dma_bs->primary_size) {
> DRM_INFO("Primary DMA buffer size reduced from %u to %u.\n",
> dma_bs->primary_size,
> - (unsigned) dev_priv->primary_dmah->size);
> - dma_bs->primary_size = dev_priv->primary_dmah->size;
> - }
> -
> - err = mga_fake_addmap(dev_priv->primary_dmah, & dev_priv->primary);
> - if (err) {
> - DRM_ERROR("Unable to map primary DMA region\n");
> - return err;
> + (unsigned) dev_priv->primary->size);
> + dma_bs->primary_size = dev_priv->primary->size;
> }
>
> -
> for ( bin_count = dma_bs->secondary_bin_count
> ; bin_count > 0
> ; bin_count-- ) {
> @@ -970,47 +921,8 @@
> drm_core_ioremapfree(dev->agp_buffer_map, dev);
>
> if (dev_priv->used_new_dma_init) {
The following block of code is called during driver takedown (i.e., at
rmmod time on Linux) *and* when the DDX calls mga_dma_init with func =
MGA_CLEANUP_MGA. Basically, the DDX bootstraps and inits DMA at
start-up, and tears it down when it exists. I think that starting X,
exiting X, and re-starting X will fail with this code removed.
I think we need some better documentation in drmP.h for *when* all the
pre / post functions are called. I wasted a lot of time on my MGA
changes doing guess-and-check to get it right. It also seems like some
of that could be refacted into shared-core. There *is* a common sub-set
between linux-core and bsd-core.
> - if (dev_priv->warp != NULL) {
> - drm_rmmap(dev, (void *) dev_priv->warp->offset);
> - }
> -
> - if (dev_priv->primary != NULL) {
> - if (dev_priv->primary->type != _DRM_CONSISTENT)
> {
> - drm_rmmap(dev, (void *)
> dev_priv->primary->offset);
> - }
> - else {
> - drm_free(dev_priv->primary,
> sizeof(drm_map_t), DRM_MEM_DRIVER);
> - }
> - }
> -
> - if (dev_priv->warp_dmah != NULL) {
> - drm_pci_free(dev, dev_priv->warp_dmah);
> - dev_priv->warp_dmah = NULL;
> - }
> -
> - if (dev_priv->primary_dmah != NULL) {
> - drm_pci_free(dev, dev_priv->primary_dmah);
> - dev_priv->primary_dmah = NULL;
> - }
> -
> - if (dev_priv->mmio != NULL) {
> - drm_rmmap(dev, (void *) dev_priv->mmio->offset);
> - }
> -
> - if (dev_priv->status != NULL) {
> - drm_rmmap(dev, (void *)
> dev_priv->status->offset);
> - }
> -
> if (dev_priv->agp_mem != NULL) {
> - if (dev->agp_buffer_map != NULL) {
> - drm_rmmap(dev, (void *)
> dev->agp_buffer_map->offset);
> - }
> -
> - if (dev_priv->agp_textures != NULL) {
> - drm_rmmap(dev, (void *)
> dev_priv->agp_textures->offset);
> - dev_priv->agp_textures = NULL;
> - }
> -
> + dev_priv->agp_textures = NULL;
> drm_unbind_agp(dev_priv->agp_mem);
>
> drm_free_agp(dev_priv->agp_mem,
> dev_priv->agp_pages);
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFCwE/GX1gOwKyEAw8RAnoZAJ9u2kWj6rehMzimcmLTMMvGGaFYtACePMP9
GN8qwWtx1/vxw+ba4imnyBg=
=MlmF
-----END PGP SIGNATURE-----
-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
