> Date: Tue, 13 Nov 2012 16:15:59 +0100
> From: Martin Pieuchot <[email protected]>
>
> On 13/11/12(Tue) 13:45, Mark Kettenis wrote:
> > > Date: Tue, 13 Nov 2012 12:30:29 +0100
> > > From: Martin Pieuchot <[email protected]>
> > >
> > > 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);
> }
