| Message ID | 20220204174436.830121-5-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Refactor ADS access to use iosys_map | expand |
Hi Am 04.02.22 um 18:44 schrieb Lucas De Marchi: > Add a variant of shmem_read() that takes a iosys_map pointer rather > than a plain pointer as argument. It's mostly a copy __shmem_rw() but > adapting the api and removing the write support since there's currently > only need to use iosys_map as destination. > > Reworking __shmem_rw() to share the implementation was tempting, but > finding a good balance between reuse and clarity pushed towards a little > code duplication. Since the function is small, just add the similar > function with a copy/paste/adapt approach. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/gt/shmem_utils.c | 33 +++++++++++++++++++++++++++ > drivers/gpu/drm/i915/gt/shmem_utils.h | 3 +++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c b/drivers/gpu/drm/i915/gt/shmem_utils.c > index 0683b27a3890..764adefdb4be 100644 > --- a/drivers/gpu/drm/i915/gt/shmem_utils.c > +++ b/drivers/gpu/drm/i915/gt/shmem_utils.c > @@ -3,6 +3,7 @@ > * Copyright © 2020 Intel Corporation > */ > > +#include <linux/iosys-map.h> > #include <linux/mm.h> > #include <linux/pagemap.h> > #include <linux/shmem_fs.h> > @@ -123,6 +124,38 @@ static int __shmem_rw(struct file *file, loff_t off, > return 0; > } > Here's a good example of how to avoid iosys_map_incr() and use the memcpy offset: > +int shmem_read_to_iosys_map(struct file *file, loff_t off, > + struct iosys_map *map, size_t len) > +{ > + struct iosys_map map_iter = *map; Rather replace map_iter with something like unsigned long map_off = 0; > + unsigned long pfn; > + > + for (pfn = off >> PAGE_SHIFT; len; pfn++) { > + unsigned int this = > + min_t(size_t, PAGE_SIZE - offset_in_page(off), len); > + struct page *page; > + void *vaddr; > + > + page = shmem_read_mapping_page_gfp(file->f_mapping, pfn, > + GFP_KERNEL); > + if (IS_ERR(page)) > + return PTR_ERR(page); > + > + vaddr = kmap(page); > + iosys_map_memcpy_to(&map_iter, 0, vaddr + offset_in_page(off), > + this); Use map_off to index into map directly. > + mark_page_accessed(page); > + kunmap(page); > + put_page(page); > + > + len -= this; > + iosys_map_incr(&map_iter, this); Raplace iosys_map_incr() with map_off += this; > + off = 0; Maybe off += this ? I think this pattern should be applied to all similar code. As you already noted, iosys_map_incr() is problematic. Best regards Thomas > + } > + > + return 0; > +} > + > int shmem_read(struct file *file, loff_t off, void *dst, size_t len) > { > return __shmem_rw(file, off, dst, len, false); > diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.h b/drivers/gpu/drm/i915/gt/shmem_utils.h > index c1669170c351..e1784999faee 100644 > --- a/drivers/gpu/drm/i915/gt/shmem_utils.h > +++ b/drivers/gpu/drm/i915/gt/shmem_utils.h > @@ -8,6 +8,7 @@ > > #include <linux/types.h> > > +struct iosys_map; > struct drm_i915_gem_object; > struct file; > > @@ -17,6 +18,8 @@ struct file *shmem_create_from_object(struct drm_i915_gem_object *obj); > void *shmem_pin_map(struct file *file); > void shmem_unpin_map(struct file *file, void *ptr); > > +int shmem_read_to_iosys_map(struct file *file, loff_t off, > + struct iosys_map *map, size_t len); > int shmem_read(struct file *file, loff_t off, void *dst, size_t len); > int shmem_write(struct file *file, loff_t off, void *src, size_t len); >
On Fri, Feb 04, 2022 at 08:15:12PM +0100, Thomas Zimmermann wrote: >Hi > >Am 04.02.22 um 18:44 schrieb Lucas De Marchi: >>Add a variant of shmem_read() that takes a iosys_map pointer rather >>than a plain pointer as argument. It's mostly a copy __shmem_rw() but >>adapting the api and removing the write support since there's currently >>only need to use iosys_map as destination. >> >>Reworking __shmem_rw() to share the implementation was tempting, but >>finding a good balance between reuse and clarity pushed towards a little >>code duplication. Since the function is small, just add the similar >>function with a copy/paste/adapt approach. >> >>Cc: Matt Roper <matthew.d.roper@intel.com> >>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>Cc: David Airlie <airlied@linux.ie> >>Cc: Daniel Vetter <daniel@ffwll.ch> >>Cc: Matthew Auld <matthew.auld@intel.com> >>Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >>--- >> drivers/gpu/drm/i915/gt/shmem_utils.c | 33 +++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/gt/shmem_utils.h | 3 +++ >> 2 files changed, 36 insertions(+) >> >>diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c b/drivers/gpu/drm/i915/gt/shmem_utils.c >>index 0683b27a3890..764adefdb4be 100644 >>--- a/drivers/gpu/drm/i915/gt/shmem_utils.c >>+++ b/drivers/gpu/drm/i915/gt/shmem_utils.c >>@@ -3,6 +3,7 @@ >> * Copyright © 2020 Intel Corporation >> */ >>+#include <linux/iosys-map.h> >> #include <linux/mm.h> >> #include <linux/pagemap.h> >> #include <linux/shmem_fs.h> >>@@ -123,6 +124,38 @@ static int __shmem_rw(struct file *file, loff_t off, >> return 0; >> } > >Here's a good example of how to avoid iosys_map_incr() and use the >memcpy offset: > >>+int shmem_read_to_iosys_map(struct file *file, loff_t off, >>+ struct iosys_map *map, size_t len) >>+{ >>+ struct iosys_map map_iter = *map; > >Rather replace map_iter with something like > > unsigned long map_off = 0; > >>+ unsigned long pfn; >>+ >>+ for (pfn = off >> PAGE_SHIFT; len; pfn++) { >>+ unsigned int this = >>+ min_t(size_t, PAGE_SIZE - offset_in_page(off), len); >>+ struct page *page; >>+ void *vaddr; >>+ >>+ page = shmem_read_mapping_page_gfp(file->f_mapping, pfn, >>+ GFP_KERNEL); >>+ if (IS_ERR(page)) >>+ return PTR_ERR(page); >>+ >>+ vaddr = kmap(page); >>+ iosys_map_memcpy_to(&map_iter, 0, vaddr + offset_in_page(off), >>+ this); > >Use map_off to index into map directly. > >>+ mark_page_accessed(page); >>+ kunmap(page); >>+ put_page(page); >>+ >>+ len -= this; >>+ iosys_map_incr(&map_iter, this); > >Raplace iosys_map_incr() with map_off += this; > >>+ off = 0; > >Maybe off += this ? Wouldn't that be wrong? AFAICS `off` is the offset of the file (source) and we zero it here so just the first iteration considers it as a page offset - next iterations are expected to copy whole pages or the remaining of the buffer. Encapsulating the dest offset calculation inside iosys_map with an iter was a way to avoid this kind of bugs in places that need to keep track of both source and dest. Anyway, I disagree and commit here. I will change to map_off and think about the other cases. There are also some more complex cases in which copying the map and work with it avoids other bugs that I will have to think about: - patch 9 ("drm/i915/guc: Convert engine record to iosys_map"): as you already noticed we delegate its update to other compilation unit - patch 11 ("drm/i915/guc: Convert golden context prep to iosys_map"): info_map, which can point either to the real buffer (in system or IO memory) or to a temporary buffer (system memory) - patch 17: it needs to write to 2 parts of struct: passing different offsets depending on the call is IMO more error prone than making sure we are working with the right variable. thanks Lucas De Marchi > >I think this pattern should be applied to all similar code. As you >already noted, iosys_map_incr() is problematic. > >Best regards >Thomas > >>+ } >>+ >>+ return 0; >>+} >>+ >> int shmem_read(struct file *file, loff_t off, void *dst, size_t len) >> { >> return __shmem_rw(file, off, dst, len, false); >>diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.h b/drivers/gpu/drm/i915/gt/shmem_utils.h >>index c1669170c351..e1784999faee 100644 >>--- a/drivers/gpu/drm/i915/gt/shmem_utils.h >>+++ b/drivers/gpu/drm/i915/gt/shmem_utils.h >>@@ -8,6 +8,7 @@ >> #include <linux/types.h> >>+struct iosys_map; >> struct drm_i915_gem_object; >> struct file; >>@@ -17,6 +18,8 @@ struct file *shmem_create_from_object(struct drm_i915_gem_object *obj); >> void *shmem_pin_map(struct file *file); >> void shmem_unpin_map(struct file *file, void *ptr); >>+int shmem_read_to_iosys_map(struct file *file, loff_t off, >>+ struct iosys_map *map, size_t len); >> int shmem_read(struct file *file, loff_t off, void *dst, size_t len); >> int shmem_write(struct file *file, loff_t off, void *src, size_t len); > >-- >Thomas Zimmermann >Graphics Driver Developer >SUSE Software Solutions Germany GmbH >Maxfeldstr. 5, 90409 Nürnberg, Germany >(HRB 36809, AG Nürnberg) >Geschäftsführer: Ivo Totev
diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.c b/drivers/gpu/drm/i915/gt/shmem_utils.c index 0683b27a3890..764adefdb4be 100644 --- a/drivers/gpu/drm/i915/gt/shmem_utils.c +++ b/drivers/gpu/drm/i915/gt/shmem_utils.c @@ -3,6 +3,7 @@ * Copyright © 2020 Intel Corporation */ +#include <linux/iosys-map.h> #include <linux/mm.h> #include <linux/pagemap.h> #include <linux/shmem_fs.h> @@ -123,6 +124,38 @@ static int __shmem_rw(struct file *file, loff_t off, return 0; } +int shmem_read_to_iosys_map(struct file *file, loff_t off, + struct iosys_map *map, size_t len) +{ + struct iosys_map map_iter = *map; + unsigned long pfn; + + for (pfn = off >> PAGE_SHIFT; len; pfn++) { + unsigned int this = + min_t(size_t, PAGE_SIZE - offset_in_page(off), len); + struct page *page; + void *vaddr; + + page = shmem_read_mapping_page_gfp(file->f_mapping, pfn, + GFP_KERNEL); + if (IS_ERR(page)) + return PTR_ERR(page); + + vaddr = kmap(page); + iosys_map_memcpy_to(&map_iter, 0, vaddr + offset_in_page(off), + this); + mark_page_accessed(page); + kunmap(page); + put_page(page); + + len -= this; + iosys_map_incr(&map_iter, this); + off = 0; + } + + return 0; +} + int shmem_read(struct file *file, loff_t off, void *dst, size_t len) { return __shmem_rw(file, off, dst, len, false); diff --git a/drivers/gpu/drm/i915/gt/shmem_utils.h b/drivers/gpu/drm/i915/gt/shmem_utils.h index c1669170c351..e1784999faee 100644 --- a/drivers/gpu/drm/i915/gt/shmem_utils.h +++ b/drivers/gpu/drm/i915/gt/shmem_utils.h @@ -8,6 +8,7 @@ #include <linux/types.h> +struct iosys_map; struct drm_i915_gem_object; struct file; @@ -17,6 +18,8 @@ struct file *shmem_create_from_object(struct drm_i915_gem_object *obj); void *shmem_pin_map(struct file *file); void shmem_unpin_map(struct file *file, void *ptr); +int shmem_read_to_iosys_map(struct file *file, loff_t off, + struct iosys_map *map, size_t len); int shmem_read(struct file *file, loff_t off, void *dst, size_t len); int shmem_write(struct file *file, loff_t off, void *src, size_t len);
Add a variant of shmem_read() that takes a iosys_map pointer rather than a plain pointer as argument. It's mostly a copy __shmem_rw() but adapting the api and removing the write support since there's currently only need to use iosys_map as destination. Reworking __shmem_rw() to share the implementation was tempting, but finding a good balance between reuse and clarity pushed towards a little code duplication. Since the function is small, just add the similar function with a copy/paste/adapt approach. Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/gt/shmem_utils.c | 33 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/gt/shmem_utils.h | 3 +++ 2 files changed, 36 insertions(+)