> Date: Tue, 13 Nov 2012 16:15:59 +0100 > From: Martin Pieuchot <mpieuc...@nolizard.org> > > On 13/11/12(Tue) 13:45, Mark Kettenis wrote: > > > Date: Tue, 13 Nov 2012 12:30:29 +0100 > > > From: Martin Pieuchot <mpieuc...@nolizard.org> > > > > > > While experimenting with the agp(4) interface I found that if you > > > release the interface (AGPIOC_RELEASE) before closing its file > > > descriptor you'll end up with allocated but unbinded memory blocks. > > > > That behaviour is documented in agp(4). So if we change it, we should > > change the documentation as well. That said, the documentation also > > says that the AGPIOC_RELEASE ioctl doesn't unbind any allocated > > memory. And that doesn't seem to be true. > > > > > This behavior is due to the fact that the agp_release_helper() function > > > doesn't free the memory associated to each block and this is incoherent > > > with what it says it does: > > > > > > /* > > > * Clear out the aperture and free any > > > * outstanding memory blocks. > > > */ > > > ... > > > > > > So the diff below correct this by freeing all the memory block when > > > releasing the interface, this is what happens currently if you close > > > the file descriptor without releasing the interface. > > > > I;m not sure this is right. I think the idea here is that an > > application could release control to hand things over to drm, and > > later reacquire control. The comment in > > xenocara/xserver/hw/xfree86/os-support/bsd/bsd_agp.c:xf86ReleaseGART() > > suggests that this behaviour is desired. > > Fair enough, so here's a diff that removes the chunk of code unbinding > the memory block from the release routine. This makes the code match > what the manual says. > > Ok?
Hmm, I think it currently pretty much a bug if a process calls AGPIOC_RELEASE while it still has stuff bound to the aperture as we don't restore the bindings when we resume. So I think we should keep the printf's and perhaps keep unbinding the memory as well. I'm not sure how much damage stale mappings can do during a suspend/resume cycle. It seems that the xf86-video-intel driver is the only userland code that actually uses /dev/agp (through the xserver interfaces). And because of the "FreeBSD hack" I mentioned in my previous mail, I don't think it actually ever invokes the AGPIOC_RELEASE ioctl after it starts using the GART. It's a great bloody mess :(. But fortunately that usage goes away as soon as we implement KMS. > Index: agp.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/agp.c,v > retrieving revision 1.34 > diff -u -p -r1.34 agp.c > --- agp.c 26 Dec 2010 15:41:00 -0000 1.34 > +++ agp.c 13 Nov 2012 13:41:43 -0000 > @@ -658,25 +678,13 @@ int > agp_release_helper(void *dev, enum agp_acquire_state state) > { > struct agp_softc *sc = (struct agp_softc *)dev; > - struct agp_memory* mem; > > if (sc->sc_state == AGP_ACQUIRE_FREE) > return (0); > > - if (sc->sc_state != state) > + if (sc->sc_state != state) > return (EBUSY); > > - /* > - * Clear out the aperture and free any > - * outstanding memory blocks. > - */ > - TAILQ_FOREACH(mem, &sc->sc_memory, am_link) { > - if (mem->am_is_bound) { > - printf("agp_release_helper: mem %d is bound\n", > - mem->am_id); > - agp_unbind_memory(sc, mem); > - } > - } > sc->sc_state = AGP_ACQUIRE_FREE; > return (0); > }