| Message ID | 1461151059-16361-13-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote: > From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com> > > This patch extends the GET_APERTURE ioctl to add support > for getting total size and available size of the stolen region > as well as single largest block available in the stolen region. > Also adds debugfs support to retieve the size information of the > stolen area. > > v2: respinned over Rodrigo's patch which extends the GET_APERTURE > too. Used drm_mm to get the size information of the stolen region > (Chris) > Added debugfs support for testing (Ankit) > > v3: Rebased to the latest drm-intel-nightly (Ankit) > > v4: Addressed comments, moved non-limits to debugfs.c (Chris/Tvrtko) > > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++- > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 41 ++++++++++++++++++++++++++++++++++ > include/uapi/drm/i915_drm.h | 6 +++++ > 5 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index d8d7994..55d110e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -580,7 +580,7 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data) > struct list_head map_list; > const uint64_t map_limit = ggtt->mappable_end; > uint64_t map_space, map_largest, fence_space, fence_largest; > - uint64_t last, size; > + uint64_t last, size, stolen_free, stolen_largest; > int ret; > > INIT_LIST_HEAD(&map_list); > @@ -593,6 +593,10 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data) > return ret; > > mutex_lock(&dev->struct_mutex); > + > + i915_gem_stolen_size_info(dev_priv, &stolen_free, > + &stolen_largest); > + > list_for_each_entry(vma, &ggtt->base.active_list, vm_link) > if (vma->pin_count && > (vma->node.start + vma->node.size) <= map_limit) > @@ -652,6 +656,12 @@ skip_first: > fence_space); > seq_printf(m, "Single largest fence available: %llu bytes\n", > fence_largest); > + seq_printf(m, "Total size of the stolen region: %llu bytes\n", > + arg.stolen_total_size); > + seq_printf(m, "Available size of the stolen region: %llu bytes\n", > + stolen_free); > + seq_printf(m, "Single largest area in the stolen region: %llu bytes\n", > + stolen_largest); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 37f9de8..d5c6b23 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3424,6 +3424,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > u32 gtt_offset, > u32 size); > int __must_check i915_gem_stolen_freeze(struct drm_i915_private *i915); > +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv, > + uint64_t *stolen_free, > + uint64_t *stolen_largest); > > /* i915_gem_shrinker.c */ > unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index a1be35f..e0f2259 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -166,6 +166,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, > args->aper_size = ggtt->base.total; > args->aper_available_size = args->aper_size - pinned; > args->map_total_size = ggtt->mappable_end;; > + args->stolen_total_size = > + dev_priv->mm.volatile_stolen ? 0 : ggtt->stolen_usable_size; > > return 0; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index ef28af6..f2621c5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -912,3 +912,44 @@ int i915_gem_stolen_freeze(struct drm_i915_private *i915) > > return ret; > } > + > + > +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv, > + uint64_t *stolen_free, > + uint64_t *stolen_largest) > +{ > + struct drm_mm *mm = &dev_priv->mm.stolen; > + struct drm_mm_node *head_node = &mm->head_node; > + struct drm_mm_node *entry; > + uint64_t hole_size, hole_start, hole_end, largest_hole = 0; > + uint64_t total_free = 0; > + > + if (dev_priv->mm.volatile_stolen) { > + *stolen_free = 0; > + *stolen_largest = 0; > + return; > + } > + > + if (head_node->hole_follows) { > + hole_start = drm_mm_hole_node_start(head_node); > + hole_end = drm_mm_hole_node_end(head_node); > + hole_size = hole_end - hole_start; > + total_free += hole_size; > + if (largest_hole < hole_size) > + largest_hole = hole_size; > + } > + Why does this block need to be separately handled and the loop below would not cover it? On first iteration entry will be the head node below as well, no? > + drm_mm_for_each_node(entry, mm) { > + if (entry->hole_follows) { > + hole_start = drm_mm_hole_node_start(entry); > + hole_end = drm_mm_hole_node_end(entry); > + hole_size = hole_end - hole_start; > + total_free += hole_size; > + if (largest_hole < hole_size) > + largest_hole = hole_size; > + } > + } > + > + *stolen_free = total_free; > + *stolen_largest = largest_hole; > +} > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 8f38407..424e57e 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture { > * Total space in the mappable region of the aperture, in bytes > */ > __u64 map_total_size; > + > + /** > + * Total space in the stolen region, in bytes > + */ > + __u64 stolen_total_size; > + How will the userspace detect existence of the new ioctl fields? Is it intended that they try to call it with a buffer of certain size and act on the failure when that fails? Is that good enough or we need something better like get_param or something? Regards, Tvrtko > }; > > struct drm_i915_get_pipe_from_crtc_id { >
On Thu, Apr 21, 2016 at 03:17:06PM +0100, Tvrtko Ursulin wrote: > >+void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv, > >+ uint64_t *stolen_free, > >+ uint64_t *stolen_largest) > >+{ > >+ struct drm_mm *mm = &dev_priv->mm.stolen; > >+ struct drm_mm_node *head_node = &mm->head_node; > >+ struct drm_mm_node *entry; > >+ uint64_t hole_size, hole_start, hole_end, largest_hole = 0; > >+ uint64_t total_free = 0; > >+ > >+ if (dev_priv->mm.volatile_stolen) { > >+ *stolen_free = 0; > >+ *stolen_largest = 0; > >+ return; > >+ } > >+ > >+ if (head_node->hole_follows) { > >+ hole_start = drm_mm_hole_node_start(head_node); > >+ hole_end = drm_mm_hole_node_end(head_node); > >+ hole_size = hole_end - hole_start; > >+ total_free += hole_size; > >+ if (largest_hole < hole_size) > >+ largest_hole = hole_size; > >+ } > >+ > > Why does this block need to be separately handled and the loop below > would not cover it? On first iteration entry will be the head node > below as well, no? Hmm, didn't I/somebody add drm_mm_for_each_hole() ? > >+ *stolen_free = total_free; > >+ *stolen_largest = largest_hole; > >+} > >diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > >index 8f38407..424e57e 100644 > >--- a/include/uapi/drm/i915_drm.h > >+++ b/include/uapi/drm/i915_drm.h > >@@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture { > > * Total space in the mappable region of the aperture, in bytes > > */ > > __u64 map_total_size; > >+ > >+ /** > >+ * Total space in the stolen region, in bytes > >+ */ > >+ __u64 stolen_total_size; > >+ > > How will the userspace detect existence of the new ioctl fields? Is > it intended that they try to call it with a buffer of certain size > and act on the failure when that fails? Is that good enough or we > need something better like get_param or something? As we are extending the structure: 1. Old userspace, old kernel: unaffacted 2. Old userspace, new kernel: Kernel computes the new fields, but the struct is truncated in the copy back to userspace. userspace only sees the fields it used to, no change. 3. New userspace, old kernel: Userspace passes in a larger struct, kernel only copies back in the fields it knows and zero fills the tail of the user's struct. userspace sees a stolen_total_size of 0 and knows to avoid the new interface. 4. New userspace, new kernel: Everything explodes^W just works. In this case, the struct is only an out, so we don't need to do invalid pad or flag rejection. We just have the ABI rule that anything the kernel doesn't know about is ignored and zero-filled were applicable. -Chris
On 21/04/16 15:41, Chris Wilson wrote: > On Thu, Apr 21, 2016 at 03:17:06PM +0100, Tvrtko Ursulin wrote: >>> +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv, >>> + uint64_t *stolen_free, >>> + uint64_t *stolen_largest) >>> +{ >>> + struct drm_mm *mm = &dev_priv->mm.stolen; >>> + struct drm_mm_node *head_node = &mm->head_node; >>> + struct drm_mm_node *entry; >>> + uint64_t hole_size, hole_start, hole_end, largest_hole = 0; >>> + uint64_t total_free = 0; >>> + >>> + if (dev_priv->mm.volatile_stolen) { >>> + *stolen_free = 0; >>> + *stolen_largest = 0; >>> + return; >>> + } >>> + >>> + if (head_node->hole_follows) { >>> + hole_start = drm_mm_hole_node_start(head_node); >>> + hole_end = drm_mm_hole_node_end(head_node); >>> + hole_size = hole_end - hole_start; >>> + total_free += hole_size; >>> + if (largest_hole < hole_size) >>> + largest_hole = hole_size; >>> + } >>> + >> >> Why does this block need to be separately handled and the loop below >> would not cover it? On first iteration entry will be the head node >> below as well, no? > > Hmm, didn't I/somebody add drm_mm_for_each_hole() ? I see it in the header, yes. >>> + *stolen_free = total_free; >>> + *stolen_largest = largest_hole; >>> +} >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>> index 8f38407..424e57e 100644 >>> --- a/include/uapi/drm/i915_drm.h >>> +++ b/include/uapi/drm/i915_drm.h >>> @@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture { >>> * Total space in the mappable region of the aperture, in bytes >>> */ >>> __u64 map_total_size; >>> + >>> + /** >>> + * Total space in the stolen region, in bytes >>> + */ >>> + __u64 stolen_total_size; >>> + >> >> How will the userspace detect existence of the new ioctl fields? Is >> it intended that they try to call it with a buffer of certain size >> and act on the failure when that fails? Is that good enough or we >> need something better like get_param or something? > > As we are extending the structure: > > 1. Old userspace, old kernel: unaffacted > > 2. Old userspace, new kernel: > Kernel computes the new fields, but the struct is truncated in the copy > back to userspace. userspace only sees the fields it used to, no change. > > 3. New userspace, old kernel: > Userspace passes in a larger struct, kernel only copies back in the > fields it knows and zero fills the tail of the user's struct. userspace > sees a stolen_total_size of 0 and knows to avoid the new interface. I suppose it doesn't make any practical difference to the driver between "there is no stolen memory" and "driver does not support stolen memory query / create". For mappable region it is a bit weirder because it wouldn't be able to tell if there is no mappable or no query support so it would potentially incorrectly avoid mmap_gtt etc. > 4. New userspace, new kernel: > Everything explodes^W just works. > > In this case, the struct is only an out, so we don't need to do invalid > pad or flag rejection. We just have the ABI rule that anything the > kernel doesn't know about is ignored and zero-filled were applicable. > -Chris > Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index d8d7994..55d110e 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -580,7 +580,7 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data) struct list_head map_list; const uint64_t map_limit = ggtt->mappable_end; uint64_t map_space, map_largest, fence_space, fence_largest; - uint64_t last, size; + uint64_t last, size, stolen_free, stolen_largest; int ret; INIT_LIST_HEAD(&map_list); @@ -593,6 +593,10 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data) return ret; mutex_lock(&dev->struct_mutex); + + i915_gem_stolen_size_info(dev_priv, &stolen_free, + &stolen_largest); + list_for_each_entry(vma, &ggtt->base.active_list, vm_link) if (vma->pin_count && (vma->node.start + vma->node.size) <= map_limit) @@ -652,6 +656,12 @@ skip_first: fence_space); seq_printf(m, "Single largest fence available: %llu bytes\n", fence_largest); + seq_printf(m, "Total size of the stolen region: %llu bytes\n", + arg.stolen_total_size); + seq_printf(m, "Available size of the stolen region: %llu bytes\n", + stolen_free); + seq_printf(m, "Single largest area in the stolen region: %llu bytes\n", + stolen_largest); return 0; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 37f9de8..d5c6b23 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3424,6 +3424,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, u32 gtt_offset, u32 size); int __must_check i915_gem_stolen_freeze(struct drm_i915_private *i915); +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv, + uint64_t *stolen_free, + uint64_t *stolen_largest); /* i915_gem_shrinker.c */ unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a1be35f..e0f2259 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -166,6 +166,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, args->aper_size = ggtt->base.total; args->aper_available_size = args->aper_size - pinned; args->map_total_size = ggtt->mappable_end;; + args->stolen_total_size = + dev_priv->mm.volatile_stolen ? 0 : ggtt->stolen_usable_size; return 0; } diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index ef28af6..f2621c5 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -912,3 +912,44 @@ int i915_gem_stolen_freeze(struct drm_i915_private *i915) return ret; } + + +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv, + uint64_t *stolen_free, + uint64_t *stolen_largest) +{ + struct drm_mm *mm = &dev_priv->mm.stolen; + struct drm_mm_node *head_node = &mm->head_node; + struct drm_mm_node *entry; + uint64_t hole_size, hole_start, hole_end, largest_hole = 0; + uint64_t total_free = 0; + + if (dev_priv->mm.volatile_stolen) { + *stolen_free = 0; + *stolen_largest = 0; + return; + } + + if (head_node->hole_follows) { + hole_start = drm_mm_hole_node_start(head_node); + hole_end = drm_mm_hole_node_end(head_node); + hole_size = hole_end - hole_start; + total_free += hole_size; + if (largest_hole < hole_size) + largest_hole = hole_size; + } + + drm_mm_for_each_node(entry, mm) { + if (entry->hole_follows) { + hole_start = drm_mm_hole_node_start(entry); + hole_end = drm_mm_hole_node_end(entry); + hole_size = hole_end - hole_start; + total_free += hole_size; + if (largest_hole < hole_size) + largest_hole = hole_size; + } + } + + *stolen_free = total_free; + *stolen_largest = largest_hole; +} diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 8f38407..424e57e 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture { * Total space in the mappable region of the aperture, in bytes */ __u64 map_total_size; + + /** + * Total space in the stolen region, in bytes + */ + __u64 stolen_total_size; + }; struct drm_i915_get_pipe_from_crtc_id {