Re: agp(4): release unbinded memory

2012-11-13 Thread Mark Kettenis
 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

2012-11-13 Thread Martin Pieuchot
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

2012-11-13 Thread Mark Kettenis
 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);
  }