Re: agp(4): release unbinded memory
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.
Re: agp(4): release unbinded memory
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? 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 - 1.34 +++ agp.c 13 Nov 2012 13:41:43 - @@ -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); }
Re: agp(4): release unbinded memory
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 - 1.34 +++ agp.c 13 Nov 2012 13:41:43 - @@ -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); }