> 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);
>  }

Reply via email to