| Message ID | 20180926080353.20867-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Trim partial view sg lists | expand |
Quoting Tvrtko Ursulin (2018-09-26 09:03:53) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Partial views are small but there can be many of them, and since the sg > list space for them is allocated pessimistically, we can save some slab by > trimming the unused tail entries. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > On the other hand with the diminishing importance of mmap ggtt do we > expect to see partial views in use these days? We have over a decade of hw that depends on them, and mesa is still not entirely rid of them (and there's always i915_dri.so). So partials are very much sticking around for a long time to come. > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++ > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8624b4bdc242..0fe22263aa9b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2323,6 +2323,8 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg) > (((__iter).curr += PAGE_SIZE) >= (__iter).max) ? \ > (__iter) = __sgt_iter(__sg_next((__iter).sgp), false), 0 : 0) > > +bool i915_sg_trim(struct sg_table *orig_st); > + > static inline unsigned int i915_sg_page_sizes(struct scatterlist *sg) > { > unsigned int page_sizes; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index db9688d14912..7156ddef178b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2491,7 +2491,7 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, > mutex_unlock(&obj->mm.lock); > } > > -static bool i915_sg_trim(struct sg_table *orig_st) > +bool i915_sg_trim(struct sg_table *orig_st) > { > struct sg_table new_st; > struct scatterlist *sg, *new_sg; You can spare me having to move a few lines later if you move this to i915_scatterlist_utils.c? > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index f6c7ab413081..1ec721c20581 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3835,6 +3835,8 @@ intel_partial_pages(const struct i915_ggtt_view *view, > count -= len >> PAGE_SHIFT; > if (count == 0) { > sg_mark_end(sg); > + i915_sg_trim(st); /* Drop any unused tail entries. */ > + Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 26/09/2018 09:15, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-26 09:03:53) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Partial views are small but there can be many of them, and since the sg >> list space for them is allocated pessimistically, we can save some slab by >> trimming the unused tail entries. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> On the other hand with the diminishing importance of mmap ggtt do we >> expect to see partial views in use these days? > > We have over a decade of hw that depends on them, and mesa is still not > entirely rid of them (and there's always i915_dri.so). So partials are > very much sticking around for a long time to come. > >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/i915_gem.c | 2 +- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++ >> 3 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 8624b4bdc242..0fe22263aa9b 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2323,6 +2323,8 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg) >> (((__iter).curr += PAGE_SIZE) >= (__iter).max) ? \ >> (__iter) = __sgt_iter(__sg_next((__iter).sgp), false), 0 : 0) >> >> +bool i915_sg_trim(struct sg_table *orig_st); >> + >> static inline unsigned int i915_sg_page_sizes(struct scatterlist *sg) >> { >> unsigned int page_sizes; >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index db9688d14912..7156ddef178b 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2491,7 +2491,7 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, >> mutex_unlock(&obj->mm.lock); >> } >> >> -static bool i915_sg_trim(struct sg_table *orig_st) >> +bool i915_sg_trim(struct sg_table *orig_st) >> { >> struct sg_table new_st; >> struct scatterlist *sg, *new_sg; > > You can spare me having to move a few lines later if you move this to > i915_scatterlist_utils.c? To be honest I am feeling optimistic today and said to myself I'd try to add a proper trim to lib/scatterlist.c. It may take a better part of the year to merge it :) but the dumb trim is starting to annoy me. >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index f6c7ab413081..1ec721c20581 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -3835,6 +3835,8 @@ intel_partial_pages(const struct i915_ggtt_view *view, >> count -= len >> PAGE_SHIFT; >> if (count == 0) { >> sg_mark_end(sg); >> + i915_sg_trim(st); /* Drop any unused tail entries. */ >> + > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> With the move to i915_scatterlist_utils.c and can keep it then? What do you need the new file for - the error state stuff? Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-09-26 09:30:41) > > On 26/09/2018 09:15, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-09-26 09:03:53) > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> Partial views are small but there can be many of them, and since the sg > >> list space for them is allocated pessimistically, we can save some slab by > >> trimming the unused tail entries. > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> --- > >> On the other hand with the diminishing importance of mmap ggtt do we > >> expect to see partial views in use these days? > > > > We have over a decade of hw that depends on them, and mesa is still not > > entirely rid of them (and there's always i915_dri.so). So partials are > > very much sticking around for a long time to come. > > > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ > >> drivers/gpu/drm/i915/i915_gem.c | 2 +- > >> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++ > >> 3 files changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 8624b4bdc242..0fe22263aa9b 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -2323,6 +2323,8 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg) > >> (((__iter).curr += PAGE_SIZE) >= (__iter).max) ? \ > >> (__iter) = __sgt_iter(__sg_next((__iter).sgp), false), 0 : 0) > >> > >> +bool i915_sg_trim(struct sg_table *orig_st); > >> + > >> static inline unsigned int i915_sg_page_sizes(struct scatterlist *sg) > >> { > >> unsigned int page_sizes; > >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> index db9688d14912..7156ddef178b 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> @@ -2491,7 +2491,7 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, > >> mutex_unlock(&obj->mm.lock); > >> } > >> > >> -static bool i915_sg_trim(struct sg_table *orig_st) > >> +bool i915_sg_trim(struct sg_table *orig_st) > >> { > >> struct sg_table new_st; > >> struct scatterlist *sg, *new_sg; > > > > You can spare me having to move a few lines later if you move this to > > i915_scatterlist_utils.c? > > To be honest I am feeling optimistic today and said to myself I'd try to > add a proper trim to lib/scatterlist.c. It may take a better part of the > year to merge it :) but the dumb trim is starting to annoy me. > >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > >> index f6c7ab413081..1ec721c20581 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >> @@ -3835,6 +3835,8 @@ intel_partial_pages(const struct i915_ggtt_view *view, > >> count -= len >> PAGE_SHIFT; > >> if (count == 0) { > >> sg_mark_end(sg); > >> + i915_sg_trim(st); /* Drop any unused tail entries. */ > >> + > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > With the move to i915_scatterlist_utils.c and can keep it then? > > What do you need the new file for - the error state stuff? I'm just splitting i915_gem_object.c (again) for async get-pages. Trying to get down to just the chipset level + ioctl interface in i915_gem.c. -Chris
On 26/09/2018 10:11, Patchwork wrote: > == Series Details == > > Series: drm/i915: Trim partial view sg lists > URL : https://patchwork.freedesktop.org/series/50177/ > State : success > > == Summary == > > = CI Bug Log - changes from CI_DRM_4878 -> Patchwork_10281 = > > == Summary - WARNING == > > Minor unknown changes coming with Patchwork_10281 need to be verified > manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_10281, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/50177/revisions/1/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_10281: > > === IGT changes === > > ==== Warnings ==== > > igt@pm_rpm@module-reload: > fi-hsw-4770r: SKIP -> PASS > > > == Known issues == > > Here are the changes found in Patchwork_10281 that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@debugfs_test@read_all_entries: > fi-icl-u: PASS -> DMESG-WARN (fdo#107724, fdo#108070) > > igt@kms_frontbuffer_tracking@basic: > fi-byt-clapper: PASS -> FAIL (fdo#103167) > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: > fi-bdw-samus: NOTRUN -> INCOMPLETE (fdo#107773) > > > ==== Possible fixes ==== > > igt@drv_module_reload@basic-reload-inject: > fi-hsw-4770r: DMESG-WARN (fdo#107924, fdo#107425) -> PASS > > igt@gem_exec_suspend@basic-s3: > fi-bdw-samus: INCOMPLETE (fdo#107773) -> PASS > > igt@kms_psr@primary_page_flip: > fi-whl-u: FAIL (fdo#107336) -> PASS > fi-cnl-u: FAIL (fdo#107336) -> PASS > > igt@prime_vgem@basic-fence-flip: > fi-cfl-8700k: FAIL (fdo#104008) -> PASS > > > fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 > fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008 > fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336 > fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425 > fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724 > fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773 > fdo#107924 https://bugs.freedesktop.org/show_bug.cgi?id=107924 > fdo#108070 https://bugs.freedesktop.org/show_bug.cgi?id=108070 > > > == Participating hosts (48 -> 41) == > > Missing (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-skl-6700hq > > > == Build changes == > > * Linux: CI_DRM_4878 -> Patchwork_10281 > > CI_DRM_4878: 49cf5deb3ecf1dcdf996518192a922645a41bf5f @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4650: a6e21812d100dce68450727e79fc09e0c0033683 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_10281: a96cc55eff1403a1e6dff1884b00f0511b16239c @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > a96cc55eff14 drm/i915: Trim partial view sg lists Pushed, thanks for the review! Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8624b4bdc242..0fe22263aa9b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2323,6 +2323,8 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg) (((__iter).curr += PAGE_SIZE) >= (__iter).max) ? \ (__iter) = __sgt_iter(__sg_next((__iter).sgp), false), 0 : 0) +bool i915_sg_trim(struct sg_table *orig_st); + static inline unsigned int i915_sg_page_sizes(struct scatterlist *sg) { unsigned int page_sizes; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index db9688d14912..7156ddef178b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2491,7 +2491,7 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, mutex_unlock(&obj->mm.lock); } -static bool i915_sg_trim(struct sg_table *orig_st) +bool i915_sg_trim(struct sg_table *orig_st) { struct sg_table new_st; struct scatterlist *sg, *new_sg; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f6c7ab413081..1ec721c20581 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3835,6 +3835,8 @@ intel_partial_pages(const struct i915_ggtt_view *view, count -= len >> PAGE_SHIFT; if (count == 0) { sg_mark_end(sg); + i915_sg_trim(st); /* Drop any unused tail entries. */ + return st; }