Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

On Wed, 27 Sep 2017 16:33:23 -0700
Eric Anholt  wrote:

> Boris Brezillon  writes:
> 
> > On Wed, 27 Sep 2017 12:41:52 -0700
> > Eric Anholt  wrote:
> >  
> >> Boris Brezillon  writes:
> >>   
> >> > On Wed, 27 Sep 2017 10:15:23 -0700
> >> > Eric Anholt  wrote:
> >> >
> >> >> Boris Brezillon  writes:
> >> >> 
> >> >> > On Wed, 27 Sep 2017 15:24:16 +0100
> >> >> > Chris Wilson  wrote:
> >> >> >  
> >> >> >> Quoting Boris Brezillon (2017-09-27 15:06:53)  
> >> >> >> > On Wed, 27 Sep 2017 14:50:10 +0100
> >> >> >> > Chris Wilson  wrote:
> >> >> >> > 
> >> >> >> > > Quoting Boris Brezillon (2017-09-27 14:45:17)
> >> >> >> > > >  static struct vc4_bo *
> >> >> >> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, 
> >> >> >> > > > const char *name)
> >> >> >> > > >  {
> >> >> >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen 
> >> >> >> > > > *screen, uint32_t size, const char *name)
> >> >> >> > > >  return NULL;
> >> >> >> > > >  }
> >> >> >> > > >  
> >> >> >> > > > +if (vc4_bo_purgeable(bo, false)) {
> >> >> >> > > > +mtx_unlock(>lock);
> >> >> >> > > > +return NULL;  
> >> >> >> > > 
> >> >> >> > > So this would just mean that the bo was purged in the meantime. 
> >> >> >> > > Why not
> >> >> >> > > just try to use the next one in the cache or allocate afresh?
> >> >> >> > > 
> >> >> >> > 
> >> >> >> > No, this means the BO was purged and the kernel failed to allocate 
> >> >> >> > the
> >> >> >> > memory back. We don't care about the retained status here, because 
> >> >> >> > we
> >> >> >> > don't need to restore BO's content, that's why we're not checking
> >> >> >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is 
> >> >> >> > likely to
> >> >> >> > fail with the same ENOMEM error because both path use the CMA mem. 
> >> >> >> >
> >> >> >> 
> >> >> >> Hmm, you don't treat purging as permanent. But you do track the lose 
> >> >> >> of
> >> >> >> contents, so retained is false?  
> >> >> >
> >> >> > vc4_bo_purgeable() is not reporting the retained status, it just
> >> >> > reports whether the BO can be used or not. I can change
> >> >> > vc4_bo_purgeable() semantic to return 1 if the BO content was 
> >> >> > retained,
> >> >> > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
> >> >> > if you prefer, but in the end, all I'll check here is
> >> >> > 'vc4_bo_purgeable() >= 0' because I don't don't care about the 
> >> >> > retained
> >> >> > status in this specific use case, all I care about is whether the BO 
> >> >> > can
> >> >> > be re-used or not (IOW, is there a valid CMA region attached to the 
> >> >> > BO).
> >> >> >  
> >> >> >> 
> >> >> >> I took a harder line, and said that userspace should recreate the 
> >> >> >> object
> >> >> >> from scratch after it was purged. I thought that would be easier
> >> >> >> overall. But maybe not.:)  
> >> >> >
> >> >> > Well, maybe I'm wrong in how I implemented this
> >> >> > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
> >> >> > purged and someone marks it back as unpurgeable I'm trying to
> >> >> > re-allocate BO's buffer in the ioctl path, and if the CMA allocation
> >> >> > fails I return -ENOMEM. I could move the allocation in the fault
> >> >> > handler, but this would result in pretty much the same behavior except
> >> >> > it would require an extra page-fault to realize the memory is not
> >> >> > available or force us to check the retained status and decide to
> >> >> > release the BO object from the BO cache.  
> >> >> 
> >> >> Hmm.  The downside I see to this plan is if we eventually decide to have
> >> >> the purge operation not clear all the BOs, then we would probably rather
> >> >> have userspace freeing objects that had been purged until it finds one
> >> >> in the cache that hadn't been purged, rather than forcing reallocation
> >> >> of this BO now (and possibly then purging something from elsewhere in
> >> >> the cache).
> >> >
> >> > Okay, that's a good reason to move dma_alloc_wc() in the page-fault
> >> > path. I need to change a bit the implementation to check cma_gem->vaddr
> >> > value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we
> >> > might pass a non-allocated BO to the GPU/Display-Engine.
> >> 
> >> Huh, allocation in the page-fault path?  We would need the storage to be
> >> definitely be available at the point that we've set it back to WILLNEED.
> >> Otherwise I'll "allocate" the BO from the cache, go to fill it through
> >> my mapping, and sigbus when CMA says we're out of memory.  
> >
> > Yep, I find that weird too, but that's 

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Eric Anholt]

Boris Brezillon  writes:

> On Wed, 27 Sep 2017 12:41:52 -0700
> Eric Anholt  wrote:
>
>> Boris Brezillon  writes:
>> 
>> > On Wed, 27 Sep 2017 10:15:23 -0700
>> > Eric Anholt  wrote:
>> >  
>> >> Boris Brezillon  writes:
>> >>   
>> >> > On Wed, 27 Sep 2017 15:24:16 +0100
>> >> > Chris Wilson  wrote:
>> >> >
>> >> >> Quoting Boris Brezillon (2017-09-27 15:06:53)
>> >> >> > On Wed, 27 Sep 2017 14:50:10 +0100
>> >> >> > Chris Wilson  wrote:
>> >> >> >   
>> >> >> > > Quoting Boris Brezillon (2017-09-27 14:45:17)  
>> >> >> > > >  static struct vc4_bo *
>> >> >> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, 
>> >> >> > > > const char *name)
>> >> >> > > >  {
>> >> >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen 
>> >> >> > > > *screen, uint32_t size, const char *name)
>> >> >> > > >  return NULL;
>> >> >> > > >  }
>> >> >> > > >  
>> >> >> > > > +if (vc4_bo_purgeable(bo, false)) {
>> >> >> > > > +mtx_unlock(>lock);
>> >> >> > > > +return NULL;
>> >> >> > > 
>> >> >> > > So this would just mean that the bo was purged in the meantime. 
>> >> >> > > Why not
>> >> >> > > just try to use the next one in the cache or allocate afresh?  
>> >> >> > 
>> >> >> > No, this means the BO was purged and the kernel failed to allocate 
>> >> >> > the
>> >> >> > memory back. We don't care about the retained status here, because we
>> >> >> > don't need to restore BO's content, that's why we're not checking
>> >> >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely 
>> >> >> > to
>> >> >> > fail with the same ENOMEM error because both path use the CMA mem.   
>> >> >> >
>> >> >> 
>> >> >> Hmm, you don't treat purging as permanent. But you do track the lose of
>> >> >> contents, so retained is false?
>> >> >
>> >> > vc4_bo_purgeable() is not reporting the retained status, it just
>> >> > reports whether the BO can be used or not. I can change
>> >> > vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
>> >> > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
>> >> > if you prefer, but in the end, all I'll check here is
>> >> > 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
>> >> > status in this specific use case, all I care about is whether the BO can
>> >> > be re-used or not (IOW, is there a valid CMA region attached to the BO).
>> >> >
>> >> >> 
>> >> >> I took a harder line, and said that userspace should recreate the 
>> >> >> object
>> >> >> from scratch after it was purged. I thought that would be easier
>> >> >> overall. But maybe not.:)
>> >> >
>> >> > Well, maybe I'm wrong in how I implemented this
>> >> > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
>> >> > purged and someone marks it back as unpurgeable I'm trying to
>> >> > re-allocate BO's buffer in the ioctl path, and if the CMA allocation
>> >> > fails I return -ENOMEM. I could move the allocation in the fault
>> >> > handler, but this would result in pretty much the same behavior except
>> >> > it would require an extra page-fault to realize the memory is not
>> >> > available or force us to check the retained status and decide to
>> >> > release the BO object from the BO cache.
>> >> 
>> >> Hmm.  The downside I see to this plan is if we eventually decide to have
>> >> the purge operation not clear all the BOs, then we would probably rather
>> >> have userspace freeing objects that had been purged until it finds one
>> >> in the cache that hadn't been purged, rather than forcing reallocation
>> >> of this BO now (and possibly then purging something from elsewhere in
>> >> the cache).  
>> >
>> > Okay, that's a good reason to move dma_alloc_wc() in the page-fault
>> > path. I need to change a bit the implementation to check cma_gem->vaddr
>> > value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we
>> > might pass a non-allocated BO to the GPU/Display-Engine.  
>> 
>> Huh, allocation in the page-fault path?  We would need the storage to be
>> definitely be available at the point that we've set it back to WILLNEED.
>> Otherwise I'll "allocate" the BO from the cache, go to fill it through
>> my mapping, and sigbus when CMA says we're out of memory.
>
> Yep, I find that weird too, but that's unfortunately the only way we can
> achieve what you want to do.
>
> The only solution to know the ->retained status is by asking the the DRM
> driver to put the BO in WILLNEED or DONTNEED state. If you send ->madv
> = DONTNEED, and the kernel returns ->retained = true, this ->retained
> state may not be valid anymore when you get back to the application,
> because someone else 

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

On Wed, 27 Sep 2017 22:03:15 +0200
Boris Brezillon  wrote:

> On Wed, 27 Sep 2017 12:41:52 -0700
> Eric Anholt  wrote:
> 
> > Boris Brezillon  writes:
> >   
> > > On Wed, 27 Sep 2017 10:15:23 -0700
> > > Eric Anholt  wrote:
> > >
> > >> Boris Brezillon  writes:
> > >> 
> > >> > On Wed, 27 Sep 2017 15:24:16 +0100
> > >> > Chris Wilson  wrote:
> > >> >  
> > >> >> Quoting Boris Brezillon (2017-09-27 15:06:53)  
> > >> >> > On Wed, 27 Sep 2017 14:50:10 +0100
> > >> >> > Chris Wilson  wrote:
> > >> >> > 
> > >> >> > > Quoting Boris Brezillon (2017-09-27 14:45:17)
> > >> >> > > >  static struct vc4_bo *
> > >> >> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, 
> > >> >> > > > const char *name)
> > >> >> > > >  {
> > >> >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen 
> > >> >> > > > *screen, uint32_t size, const char *name)
> > >> >> > > >  return NULL;
> > >> >> > > >  }
> > >> >> > > >  
> > >> >> > > > +if (vc4_bo_purgeable(bo, false)) {
> > >> >> > > > +mtx_unlock(>lock);
> > >> >> > > > +return NULL;  
> > >> >> > > 
> > >> >> > > So this would just mean that the bo was purged in the meantime. 
> > >> >> > > Why not
> > >> >> > > just try to use the next one in the cache or allocate afresh? 
> > >> >> > >
> > >> >> > 
> > >> >> > No, this means the BO was purged and the kernel failed to allocate 
> > >> >> > the
> > >> >> > memory back. We don't care about the retained status here, because 
> > >> >> > we
> > >> >> > don't need to restore BO's content, that's why we're not checking
> > >> >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely 
> > >> >> > to
> > >> >> > fail with the same ENOMEM error because both path use the CMA mem.  
> > >> >> >   
> > >> >> 
> > >> >> Hmm, you don't treat purging as permanent. But you do track the lose 
> > >> >> of
> > >> >> contents, so retained is false?  
> > >> >
> > >> > vc4_bo_purgeable() is not reporting the retained status, it just
> > >> > reports whether the BO can be used or not. I can change
> > >> > vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
> > >> > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
> > >> > if you prefer, but in the end, all I'll check here is
> > >> > 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
> > >> > status in this specific use case, all I care about is whether the BO 
> > >> > can
> > >> > be re-used or not (IOW, is there a valid CMA region attached to the 
> > >> > BO).
> > >> >  
> > >> >> 
> > >> >> I took a harder line, and said that userspace should recreate the 
> > >> >> object
> > >> >> from scratch after it was purged. I thought that would be easier
> > >> >> overall. But maybe not.:)  
> > >> >
> > >> > Well, maybe I'm wrong in how I implemented this
> > >> > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
> > >> > purged and someone marks it back as unpurgeable I'm trying to
> > >> > re-allocate BO's buffer in the ioctl path, and if the CMA allocation
> > >> > fails I return -ENOMEM. I could move the allocation in the fault
> > >> > handler, but this would result in pretty much the same behavior except
> > >> > it would require an extra page-fault to realize the memory is not
> > >> > available or force us to check the retained status and decide to
> > >> > release the BO object from the BO cache.  
> > >> 
> > >> Hmm.  The downside I see to this plan is if we eventually decide to have
> > >> the purge operation not clear all the BOs, then we would probably rather
> > >> have userspace freeing objects that had been purged until it finds one
> > >> in the cache that hadn't been purged, rather than forcing reallocation
> > >> of this BO now (and possibly then purging something from elsewhere in
> > >> the cache).
> > >
> > > Okay, that's a good reason to move dma_alloc_wc() in the page-fault
> > > path. I need to change a bit the implementation to check cma_gem->vaddr
> > > value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we
> > > might pass a non-allocated BO to the GPU/Display-Engine.
> > 
> > Huh, allocation in the page-fault path?  We would need the storage to be
> > definitely be available at the point that we've set it back to WILLNEED.
> > Otherwise I'll "allocate" the BO from the cache, go to fill it through
> > my mapping, and sigbus when CMA says we're out of memory.  
> 
> Yep, I find that weird too, but that's unfortunately the only way we can
> achieve what you want to do.
> 
> The only solution to know the ->retained status is by asking the the DRM
> driver to put the BO 

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

On Wed, 27 Sep 2017 12:41:52 -0700
Eric Anholt  wrote:

> Boris Brezillon  writes:
> 
> > On Wed, 27 Sep 2017 10:15:23 -0700
> > Eric Anholt  wrote:
> >  
> >> Boris Brezillon  writes:
> >>   
> >> > On Wed, 27 Sep 2017 15:24:16 +0100
> >> > Chris Wilson  wrote:
> >> >
> >> >> Quoting Boris Brezillon (2017-09-27 15:06:53)
> >> >> > On Wed, 27 Sep 2017 14:50:10 +0100
> >> >> > Chris Wilson  wrote:
> >> >> >   
> >> >> > > Quoting Boris Brezillon (2017-09-27 14:45:17)  
> >> >> > > >  static struct vc4_bo *
> >> >> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, 
> >> >> > > > const char *name)
> >> >> > > >  {
> >> >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, 
> >> >> > > > uint32_t size, const char *name)
> >> >> > > >  return NULL;
> >> >> > > >  }
> >> >> > > >  
> >> >> > > > +if (vc4_bo_purgeable(bo, false)) {
> >> >> > > > +mtx_unlock(>lock);
> >> >> > > > +return NULL;
> >> >> > > 
> >> >> > > So this would just mean that the bo was purged in the meantime. Why 
> >> >> > > not
> >> >> > > just try to use the next one in the cache or allocate afresh?  
> >> >> > 
> >> >> > No, this means the BO was purged and the kernel failed to allocate the
> >> >> > memory back. We don't care about the retained status here, because we
> >> >> > don't need to restore BO's content, that's why we're not checking
> >> >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
> >> >> > fail with the same ENOMEM error because both path use the CMA mem.
> >> >> >   
> >> >> 
> >> >> Hmm, you don't treat purging as permanent. But you do track the lose of
> >> >> contents, so retained is false?
> >> >
> >> > vc4_bo_purgeable() is not reporting the retained status, it just
> >> > reports whether the BO can be used or not. I can change
> >> > vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
> >> > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
> >> > if you prefer, but in the end, all I'll check here is
> >> > 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
> >> > status in this specific use case, all I care about is whether the BO can
> >> > be re-used or not (IOW, is there a valid CMA region attached to the BO).
> >> >
> >> >> 
> >> >> I took a harder line, and said that userspace should recreate the object
> >> >> from scratch after it was purged. I thought that would be easier
> >> >> overall. But maybe not.:)
> >> >
> >> > Well, maybe I'm wrong in how I implemented this
> >> > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
> >> > purged and someone marks it back as unpurgeable I'm trying to
> >> > re-allocate BO's buffer in the ioctl path, and if the CMA allocation
> >> > fails I return -ENOMEM. I could move the allocation in the fault
> >> > handler, but this would result in pretty much the same behavior except
> >> > it would require an extra page-fault to realize the memory is not
> >> > available or force us to check the retained status and decide to
> >> > release the BO object from the BO cache.
> >> 
> >> Hmm.  The downside I see to this plan is if we eventually decide to have
> >> the purge operation not clear all the BOs, then we would probably rather
> >> have userspace freeing objects that had been purged until it finds one
> >> in the cache that hadn't been purged, rather than forcing reallocation
> >> of this BO now (and possibly then purging something from elsewhere in
> >> the cache).  
> >
> > Okay, that's a good reason to move dma_alloc_wc() in the page-fault
> > path. I need to change a bit the implementation to check cma_gem->vaddr
> > value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we
> > might pass a non-allocated BO to the GPU/Display-Engine.  
> 
> Huh, allocation in the page-fault path?  We would need the storage to be
> definitely be available at the point that we've set it back to WILLNEED.
> Otherwise I'll "allocate" the BO from the cache, go to fill it through
> my mapping, and sigbus when CMA says we're out of memory.

Yep, I find that weird too, but that's unfortunately the only way we can
achieve what you want to do.

The only solution to know the ->retained status is by asking the the DRM
driver to put the BO in WILLNEED or DONTNEED state. If you send ->madv
= DONTNEED, and the kernel returns ->retained = true, this ->retained
state may not be valid anymore when you get back to the application,
because someone else may have triggered a purge. If you send ->madv =
WILLNEED then the ->retained state is guaranteed to be valid until you
explicitly switch back to DONTNEED, but that also means the driver has
already 

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Eric Anholt]

Boris Brezillon  writes:

> On Wed, 27 Sep 2017 10:15:23 -0700
> Eric Anholt  wrote:
>
>> Boris Brezillon  writes:
>> 
>> > On Wed, 27 Sep 2017 15:24:16 +0100
>> > Chris Wilson  wrote:
>> >  
>> >> Quoting Boris Brezillon (2017-09-27 15:06:53)  
>> >> > On Wed, 27 Sep 2017 14:50:10 +0100
>> >> > Chris Wilson  wrote:
>> >> > 
>> >> > > Quoting Boris Brezillon (2017-09-27 14:45:17)
>> >> > > >  static struct vc4_bo *
>> >> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const 
>> >> > > > char *name)
>> >> > > >  {
>> >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, 
>> >> > > > uint32_t size, const char *name)
>> >> > > >  return NULL;
>> >> > > >  }
>> >> > > >  
>> >> > > > +if (vc4_bo_purgeable(bo, false)) {
>> >> > > > +mtx_unlock(>lock);
>> >> > > > +return NULL;  
>> >> > > 
>> >> > > So this would just mean that the bo was purged in the meantime. Why 
>> >> > > not
>> >> > > just try to use the next one in the cache or allocate afresh?
>> >> > 
>> >> > No, this means the BO was purged and the kernel failed to allocate the
>> >> > memory back. We don't care about the retained status here, because we
>> >> > don't need to restore BO's content, that's why we're not checking
>> >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
>> >> > fail with the same ENOMEM error because both path use the CMA mem.
>> >> 
>> >> Hmm, you don't treat purging as permanent. But you do track the lose of
>> >> contents, so retained is false?  
>> >
>> > vc4_bo_purgeable() is not reporting the retained status, it just
>> > reports whether the BO can be used or not. I can change
>> > vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
>> > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
>> > if you prefer, but in the end, all I'll check here is
>> > 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
>> > status in this specific use case, all I care about is whether the BO can
>> > be re-used or not (IOW, is there a valid CMA region attached to the BO).
>> >  
>> >> 
>> >> I took a harder line, and said that userspace should recreate the object
>> >> from scratch after it was purged. I thought that would be easier
>> >> overall. But maybe not.:)  
>> >
>> > Well, maybe I'm wrong in how I implemented this
>> > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
>> > purged and someone marks it back as unpurgeable I'm trying to
>> > re-allocate BO's buffer in the ioctl path, and if the CMA allocation
>> > fails I return -ENOMEM. I could move the allocation in the fault
>> > handler, but this would result in pretty much the same behavior except
>> > it would require an extra page-fault to realize the memory is not
>> > available or force us to check the retained status and decide to
>> > release the BO object from the BO cache.  
>> 
>> Hmm.  The downside I see to this plan is if we eventually decide to have
>> the purge operation not clear all the BOs, then we would probably rather
>> have userspace freeing objects that had been purged until it finds one
>> in the cache that hadn't been purged, rather than forcing reallocation
>> of this BO now (and possibly then purging something from elsewhere in
>> the cache).
>
> Okay, that's a good reason to move dma_alloc_wc() in the page-fault
> path. I need to change a bit the implementation to check cma_gem->vaddr
> value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we
> might pass a non-allocated BO to the GPU/Display-Engine.

Huh, allocation in the page-fault path?  We would need the storage to be
definitely be available at the point that we've set it back to WILLNEED.
Otherwise I'll "allocate" the BO from the cache, go to fill it through
my mapping, and sigbus when CMA says we're out of memory.



signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

On Wed, 27 Sep 2017 10:15:23 -0700
Eric Anholt  wrote:

> Boris Brezillon  writes:
> 
> > On Wed, 27 Sep 2017 15:24:16 +0100
> > Chris Wilson  wrote:
> >  
> >> Quoting Boris Brezillon (2017-09-27 15:06:53)  
> >> > On Wed, 27 Sep 2017 14:50:10 +0100
> >> > Chris Wilson  wrote:
> >> > 
> >> > > Quoting Boris Brezillon (2017-09-27 14:45:17)
> >> > > >  static struct vc4_bo *
> >> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const 
> >> > > > char *name)
> >> > > >  {
> >> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, 
> >> > > > uint32_t size, const char *name)
> >> > > >  return NULL;
> >> > > >  }
> >> > > >  
> >> > > > +if (vc4_bo_purgeable(bo, false)) {
> >> > > > +mtx_unlock(>lock);
> >> > > > +return NULL;  
> >> > > 
> >> > > So this would just mean that the bo was purged in the meantime. Why not
> >> > > just try to use the next one in the cache or allocate afresh?
> >> > 
> >> > No, this means the BO was purged and the kernel failed to allocate the
> >> > memory back. We don't care about the retained status here, because we
> >> > don't need to restore BO's content, that's why we're not checking
> >> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
> >> > fail with the same ENOMEM error because both path use the CMA mem.
> >> 
> >> Hmm, you don't treat purging as permanent. But you do track the lose of
> >> contents, so retained is false?  
> >
> > vc4_bo_purgeable() is not reporting the retained status, it just
> > reports whether the BO can be used or not. I can change
> > vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
> > 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
> > if you prefer, but in the end, all I'll check here is
> > 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
> > status in this specific use case, all I care about is whether the BO can
> > be re-used or not (IOW, is there a valid CMA region attached to the BO).
> >  
> >> 
> >> I took a harder line, and said that userspace should recreate the object
> >> from scratch after it was purged. I thought that would be easier
> >> overall. But maybe not.:)  
> >
> > Well, maybe I'm wrong in how I implemented this
> > DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
> > purged and someone marks it back as unpurgeable I'm trying to
> > re-allocate BO's buffer in the ioctl path, and if the CMA allocation
> > fails I return -ENOMEM. I could move the allocation in the fault
> > handler, but this would result in pretty much the same behavior except
> > it would require an extra page-fault to realize the memory is not
> > available or force us to check the retained status and decide to
> > release the BO object from the BO cache.  
> 
> Hmm.  The downside I see to this plan is if we eventually decide to have
> the purge operation not clear all the BOs, then we would probably rather
> have userspace freeing objects that had been purged until it finds one
> in the cache that hadn't been purged, rather than forcing reallocation
> of this BO now (and possibly then purging something from elsewhere in
> the cache).

Okay, that's a good reason to move dma_alloc_wc() in the page-fault
path. I need to change a bit the implementation to check cma_gem->vaddr
value instead of checking bo->madv != __VC4_MADV_PURGED, otherwise we
might pass a non-allocated BO to the GPU/Display-Engine.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Eric Anholt]

Boris Brezillon  writes:

> On Wed, 27 Sep 2017 15:24:16 +0100
> Chris Wilson  wrote:
>
>> Quoting Boris Brezillon (2017-09-27 15:06:53)
>> > On Wed, 27 Sep 2017 14:50:10 +0100
>> > Chris Wilson  wrote:
>> >   
>> > > Quoting Boris Brezillon (2017-09-27 14:45:17)  
>> > > >  static struct vc4_bo *
>> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const 
>> > > > char *name)
>> > > >  {
>> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, 
>> > > > uint32_t size, const char *name)
>> > > >  return NULL;
>> > > >  }
>> > > >  
>> > > > +if (vc4_bo_purgeable(bo, false)) {
>> > > > +mtx_unlock(>lock);
>> > > > +return NULL;
>> > > 
>> > > So this would just mean that the bo was purged in the meantime. Why not
>> > > just try to use the next one in the cache or allocate afresh?  
>> > 
>> > No, this means the BO was purged and the kernel failed to allocate the
>> > memory back. We don't care about the retained status here, because we
>> > don't need to restore BO's content, that's why we're not checking
>> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
>> > fail with the same ENOMEM error because both path use the CMA mem.  
>> 
>> Hmm, you don't treat purging as permanent. But you do track the lose of
>> contents, so retained is false?
>
> vc4_bo_purgeable() is not reporting the retained status, it just
> reports whether the BO can be used or not. I can change
> vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
> 0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
> if you prefer, but in the end, all I'll check here is
> 'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
> status in this specific use case, all I care about is whether the BO can
> be re-used or not (IOW, is there a valid CMA region attached to the BO).
>
>> 
>> I took a harder line, and said that userspace should recreate the object
>> from scratch after it was purged. I thought that would be easier
>> overall. But maybe not.:)
>
> Well, maybe I'm wrong in how I implemented this
> DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
> purged and someone marks it back as unpurgeable I'm trying to
> re-allocate BO's buffer in the ioctl path, and if the CMA allocation
> fails I return -ENOMEM. I could move the allocation in the fault
> handler, but this would result in pretty much the same behavior except
> it would require an extra page-fault to realize the memory is not
> available or force us to check the retained status and decide to
> release the BO object from the BO cache.

Hmm.  The downside I see to this plan is if we eventually decide to have
the purge operation not clear all the BOs, then we would probably rather
have userspace freeing objects that had been purged until it finds one
in the cache that hadn't been purged, rather than forcing reallocation
of this BO now (and possibly then purging something from elsewhere in
the cache).


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

On Wed, 27 Sep 2017 15:24:16 +0100
Chris Wilson  wrote:

> Quoting Boris Brezillon (2017-09-27 15:06:53)
> > On Wed, 27 Sep 2017 14:50:10 +0100
> > Chris Wilson  wrote:
> >   
> > > Quoting Boris Brezillon (2017-09-27 14:45:17)  
> > > >  static struct vc4_bo *
> > > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const char 
> > > > *name)
> > > >  {
> > > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, 
> > > > uint32_t size, const char *name)
> > > >  return NULL;
> > > >  }
> > > >  
> > > > +if (vc4_bo_purgeable(bo, false)) {
> > > > +mtx_unlock(>lock);
> > > > +return NULL;
> > > 
> > > So this would just mean that the bo was purged in the meantime. Why not
> > > just try to use the next one in the cache or allocate afresh?  
> > 
> > No, this means the BO was purged and the kernel failed to allocate the
> > memory back. We don't care about the retained status here, because we
> > don't need to restore BO's content, that's why we're not checking
> > arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
> > fail with the same ENOMEM error because both path use the CMA mem.  
> 
> Hmm, you don't treat purging as permanent. But you do track the lose of
> contents, so retained is false?

vc4_bo_purgeable() is not reporting the retained status, it just
reports whether the BO can be used or not. I can change
vc4_bo_purgeable() semantic to return 1 if the BO content was retained,
0 if it was purged and -1 if you the ioctl returned an error (ENOMEM)
if you prefer, but in the end, all I'll check here is
'vc4_bo_purgeable() >= 0' because I don't don't care about the retained
status in this specific use case, all I care about is whether the BO can
be re-used or not (IOW, is there a valid CMA region attached to the BO).

> 
> I took a harder line, and said that userspace should recreate the object
> from scratch after it was purged. I thought that would be easier
> overall. But maybe not.:)

Well, maybe I'm wrong in how I implemented this
DRM_IOCTL_VC4_GEM_MADVISE ioctl, but right now, when the BO has been
purged and someone marks it back as unpurgeable I'm trying to
re-allocate BO's buffer in the ioctl path, and if the CMA allocation
fails I return -ENOMEM. I could move the allocation in the fault
handler, but this would result in pretty much the same behavior except
it would require an extra page-fault to realize the memory is not
available or force us to check the retained status and decide to
release the BO object from the BO cache.

Note that I tried to stay as close as possible to the existing
CMA-based-BO logic where everything is allocated at creation time and
not based on an on-demand allocation, hence the decision to allocate
the CMA region in the ioctl path and not in the page-fault handler.

Regards,

Boris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Chris Wilson]

Quoting Boris Brezillon (2017-09-27 15:06:53)
> On Wed, 27 Sep 2017 14:50:10 +0100
> Chris Wilson  wrote:
> 
> > Quoting Boris Brezillon (2017-09-27 14:45:17)
> > >  static struct vc4_bo *
> > >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const char 
> > > *name)
> > >  {
> > > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, 
> > > uint32_t size, const char *name)
> > >  return NULL;
> > >  }
> > >  
> > > +if (vc4_bo_purgeable(bo, false)) {
> > > +mtx_unlock(>lock);
> > > +return NULL;  
> > 
> > So this would just mean that the bo was purged in the meantime. Why not
> > just try to use the next one in the cache or allocate afresh?
> 
> No, this means the BO was purged and the kernel failed to allocate the
> memory back. We don't care about the retained status here, because we
> don't need to restore BO's content, that's why we're not checking
> arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
> fail with the same ENOMEM error because both path use the CMA mem.

Hmm, you don't treat purging as permanent. But you do track the lose of
contents, so retained is false?

I took a harder line, and said that userspace should recreate the object
from scratch after it was purged. I thought that would be easier
overall. But maybe not.:)
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

On Wed, 27 Sep 2017 14:50:10 +0100
Chris Wilson  wrote:

> Quoting Boris Brezillon (2017-09-27 14:45:17)
> >  static struct vc4_bo *
> >  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const char 
> > *name)
> >  {
> > @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, uint32_t 
> > size, const char *name)
> >  return NULL;
> >  }
> >  
> > +if (vc4_bo_purgeable(bo, false)) {
> > +mtx_unlock(>lock);
> > +return NULL;  
> 
> So this would just mean that the bo was purged in the meantime. Why not
> just try to use the next one in the cache or allocate afresh?

No, this means the BO was purged and the kernel failed to allocate the
memory back. We don't care about the retained status here, because we
don't need to restore BO's content, that's why we're not checking
arg.retained in vc4_bo_purgeable(). Allocating a fresh BO is likely to
fail with the same ENOMEM error because both path use the CMA mem.

> Not sure
> how way allocation failures are handled up the stack, but anyway this is
> not necessarily -ENOMEM.

Right, we should try with all elements in the list to see if one
of them is still around or if the kernel manages to get some
memory back in the meantime.


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Chris Wilson]

Quoting Boris Brezillon (2017-09-27 14:45:17)
>  static struct vc4_bo *
>  vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const char *name)
>  {
> @@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, uint32_t 
> size, const char *name)
>  return NULL;
>  }
>  
> +if (vc4_bo_purgeable(bo, false)) {
> +mtx_unlock(>lock);
> +return NULL;

So this would just mean that the bo was purged in the meantime. Why not
just try to use the next one in the cache or allocate afresh? Not sure
how way allocation failures are handled up the stack, but anyway this is
not necessarily -ENOMEM.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] vc4: Mark BOs as purgeable when they enter the BO cache

[Boris Brezillon]

This patch makes use of the DRM_IOCTL_VC4_GEM_MADVISE ioctl to mark all
BOs placed in the mesa BO cache as purgeable so that the system can
reclaim this memory under memory pressure.

Signed-off-by: Boris Brezillon 
---
Hello,

Note that this series depends on kernel code that has not been accepted
yet and is just provided to show reviewers how the ioctl can be used
and what to expect from it.

Please do not consider this for inclusion in MESA until the kernel part
has been accepted.

I also lack a check to make sure the DRM_IOCTL_VC4_GEM_MADVISE ioctl is
available before using it.

Thanks,

Boris
---
 src/gallium/drivers/vc4/vc4_bufmgr.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/gallium/drivers/vc4/vc4_bufmgr.c 
b/src/gallium/drivers/vc4/vc4_bufmgr.c
index 0653f8823232..8ee37ac7010d 100644
--- a/src/gallium/drivers/vc4/vc4_bufmgr.c
+++ b/src/gallium/drivers/vc4/vc4_bufmgr.c
@@ -87,6 +87,16 @@ vc4_bo_remove_from_cache(struct vc4_bo_cache *cache, struct 
vc4_bo *bo)
 cache->bo_size -= bo->size;
 }
 
+static int vc4_bo_purgeable(struct vc4_bo *bo, bool purgeable)
+{
+struct drm_vc4_gem_madvise arg = {
+.handle = bo->handle,
+.madv = purgeable ? VC4_MADV_DONTNEED : VC4_MADV_WILLNEED,
+};
+
+return vc4_ioctl(bo->screen->fd, DRM_IOCTL_VC4_GEM_MADVISE, );
+}
+
 static struct vc4_bo *
 vc4_bo_from_cache(struct vc4_screen *screen, uint32_t size, const char *name)
 {
@@ -111,6 +121,11 @@ vc4_bo_from_cache(struct vc4_screen *screen, uint32_t 
size, const char *name)
 return NULL;
 }
 
+if (vc4_bo_purgeable(bo, false)) {
+mtx_unlock(>lock);
+return NULL;
+}
+
 pipe_reference_init(>reference, 1);
 vc4_bo_remove_from_cache(cache, bo);
 
@@ -296,6 +311,7 @@ vc4_bo_last_unreference_locked_timed(struct vc4_bo *bo, 
time_t time)
 cache->size_list_size = page_index + 1;
 }
 
+vc4_bo_purgeable(bo, true);
 bo->free_time = time;
 list_addtail(>size_list, >size_list[page_index]);
 list_addtail(>time_list, >time_list);
-- 
2.11.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev