On Mon, 2005-06-27 at 12:13 -0700, Ian Romanick wrote:
> -----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
> > @@ -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?
Addmap aligns to the same size as the allocation itself. I just assumed
that the warp_size was bigger than 256 bytes.
> > /* 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.
takedown is called during last close of the device (when X
exits/restarts), not rmmod. I was actually testing "cleanup" is called
on rmmod. I would love to have better naming of these functions. I was
actually testing with a pair of "startx; glxgears; exit;" for the tests
I listed. I just verified that MGA worked as intended (this DRM,
current ddx and dri driver, no olddmainit).
Here's how I justified to myself that leaving map uninit to takedown was
fine: You can only have one shm area with the hardware lock inited at a
time, determined by whether hw_lock is set. The unsetting of hw_lock
only occurs at takedown. Therefore, if takedown isn't occuring between
X Server instances, you won't get DRI access anyway.
> > - 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);
--
Eric Anholt [EMAIL PROTECTED]
http://people.freebsd.org/~anholt/ [EMAIL PROTECTED]
-------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
