| Message ID | 20210624193045.5087-3-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915, drm/ttm: Update the ttm_move_memcpy() interface | expand |
On 6/24/21 9:30 PM, Thomas Hellström wrote: > The buffer object argument to ttm_move_memcpy was only used to > determine whether the destination memory should be cleared only > or whether we should copy data. Replace it with a "clear" bool, and > update the callers. > > The intention here is to be able to use ttm_move_memcpy() async under > a dma-fence as a fallback if an accelerated blit fails in a security- > critical path where data might leak if the blit is not properly > performed. For that purpose the bo is an unsuitable argument since > its relevant members might already have changed at call time. > > Finally, update the ttm_move_memcpy kerneldoc that seems to have > ended up with a stale version. Hmm, Not sure where the Cc: Christian König ended up, but in any case Christian if you find this patch ok, Ack to merge through drm_intel_gt_next? /Thomas > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- > drivers/gpu/drm/ttm/ttm_bo_util.c | 20 ++++++++++---------- > include/drm/ttm/ttm_bo_driver.h | 2 +- > 3 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 4e529adcdfc7..f19847abe856 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -517,7 +517,7 @@ static void __i915_ttm_move(struct ttm_buffer_object *bo, bool clear, > obj->ttm.cached_io_st, > src_reg->region.start); > > - ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter); > + ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter); > } > } > > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c > index 2f57f824e6db..e3747f069674 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c > @@ -75,22 +75,21 @@ void ttm_mem_io_free(struct ttm_device *bdev, > > /** > * ttm_move_memcpy - Helper to perform a memcpy ttm move operation. > - * @bo: The struct ttm_buffer_object. > - * @new_mem: The struct ttm_resource we're moving to (copy destination). > - * @new_iter: A struct ttm_kmap_iter representing the destination resource. > + * @clear: Whether to clear rather than copy. > + * @num_pages: Number of pages of the operation. > + * @dst_iter: A struct ttm_kmap_iter representing the destination resource. > * @src_iter: A struct ttm_kmap_iter representing the source resource. > * > * This function is intended to be able to move out async under a > * dma-fence if desired. > */ > -void ttm_move_memcpy(struct ttm_buffer_object *bo, > +void ttm_move_memcpy(bool clear, > u32 num_pages, > struct ttm_kmap_iter *dst_iter, > struct ttm_kmap_iter *src_iter) > { > const struct ttm_kmap_iter_ops *dst_ops = dst_iter->ops; > const struct ttm_kmap_iter_ops *src_ops = src_iter->ops; > - struct ttm_tt *ttm = bo->ttm; > struct dma_buf_map src_map, dst_map; > pgoff_t i; > > @@ -99,10 +98,7 @@ void ttm_move_memcpy(struct ttm_buffer_object *bo, > return; > > /* Don't move nonexistent data. Clear destination instead. */ > - if (src_ops->maps_tt && (!ttm || !ttm_tt_is_populated(ttm))) { > - if (ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)) > - return; > - > + if (clear) { > for (i = 0; i < num_pages; ++i) { > dst_ops->map_local(dst_iter, &dst_map, i); > if (dst_map.is_iomem) > @@ -146,6 +142,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, > struct ttm_kmap_iter_linear_io io; > } _dst_iter, _src_iter; > struct ttm_kmap_iter *dst_iter, *src_iter; > + bool clear; > int ret = 0; > > if (ttm && ((ttm->page_flags & TTM_PAGE_FLAG_SWAPPED) || > @@ -169,7 +166,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, > goto out_src_iter; > } > > - ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter); > + clear = src_iter->ops->maps_tt && (!ttm || !ttm_tt_is_populated(ttm)); > + if (!(clear && ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC))) > + ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter); > + > src_copy = *src_mem; > ttm_bo_move_sync_cleanup(bo, dst_mem); > > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index 68d6069572aa..5f087575194b 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -322,7 +322,7 @@ int ttm_bo_tt_bind(struct ttm_buffer_object *bo, struct ttm_resource *mem); > */ > void ttm_bo_tt_destroy(struct ttm_buffer_object *bo); > > -void ttm_move_memcpy(struct ttm_buffer_object *bo, > +void ttm_move_memcpy(bool clear, > u32 num_pages, > struct ttm_kmap_iter *dst_iter, > struct ttm_kmap_iter *src_iter);
On Thu, 24 Jun 2021 at 20:31, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote: > > The buffer object argument to ttm_move_memcpy was only used to > determine whether the destination memory should be cleared only > or whether we should copy data. Replace it with a "clear" bool, and > update the callers. > > The intention here is to be able to use ttm_move_memcpy() async under > a dma-fence as a fallback if an accelerated blit fails in a security- > critical path where data might leak if the blit is not properly > performed. For that purpose the bo is an unsuitable argument since > its relevant members might already have changed at call time. > > Finally, update the ttm_move_memcpy kerneldoc that seems to have > ended up with a stale version. > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Am 28.06.21 um 13:21 schrieb Thomas Hellström: > > On 6/24/21 9:30 PM, Thomas Hellström wrote: >> The buffer object argument to ttm_move_memcpy was only used to >> determine whether the destination memory should be cleared only >> or whether we should copy data. Replace it with a "clear" bool, and >> update the callers. >> >> The intention here is to be able to use ttm_move_memcpy() async under >> a dma-fence as a fallback if an accelerated blit fails in a security- >> critical path where data might leak if the blit is not properly >> performed. For that purpose the bo is an unsuitable argument since >> its relevant members might already have changed at call time. >> >> Finally, update the ttm_move_memcpy kerneldoc that seems to have >> ended up with a stale version. > > Hmm, > > Not sure where the Cc: Christian König ended up, but in any case > Christian if you find this patch ok, Ack to merge through > drm_intel_gt_next? Please send out the patch once more. Only nit picks, but I would re-order the parameters for example. Regards, Christian. > > /Thomas > > > > >> >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- >> drivers/gpu/drm/ttm/ttm_bo_util.c | 20 ++++++++++---------- >> include/drm/ttm/ttm_bo_driver.h | 2 +- >> 3 files changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> index 4e529adcdfc7..f19847abe856 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> @@ -517,7 +517,7 @@ static void __i915_ttm_move(struct >> ttm_buffer_object *bo, bool clear, >> obj->ttm.cached_io_st, >> src_reg->region.start); >> - ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter); >> + ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter); >> } >> } >> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c >> b/drivers/gpu/drm/ttm/ttm_bo_util.c >> index 2f57f824e6db..e3747f069674 100644 >> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c >> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c >> @@ -75,22 +75,21 @@ void ttm_mem_io_free(struct ttm_device *bdev, >> /** >> * ttm_move_memcpy - Helper to perform a memcpy ttm move operation. >> - * @bo: The struct ttm_buffer_object. >> - * @new_mem: The struct ttm_resource we're moving to (copy >> destination). >> - * @new_iter: A struct ttm_kmap_iter representing the destination >> resource. >> + * @clear: Whether to clear rather than copy. >> + * @num_pages: Number of pages of the operation. >> + * @dst_iter: A struct ttm_kmap_iter representing the destination >> resource. >> * @src_iter: A struct ttm_kmap_iter representing the source resource. >> * >> * This function is intended to be able to move out async under a >> * dma-fence if desired. >> */ >> -void ttm_move_memcpy(struct ttm_buffer_object *bo, >> +void ttm_move_memcpy(bool clear, >> u32 num_pages, >> struct ttm_kmap_iter *dst_iter, >> struct ttm_kmap_iter *src_iter) >> { >> const struct ttm_kmap_iter_ops *dst_ops = dst_iter->ops; >> const struct ttm_kmap_iter_ops *src_ops = src_iter->ops; >> - struct ttm_tt *ttm = bo->ttm; >> struct dma_buf_map src_map, dst_map; >> pgoff_t i; >> @@ -99,10 +98,7 @@ void ttm_move_memcpy(struct ttm_buffer_object *bo, >> return; >> /* Don't move nonexistent data. Clear destination instead. */ >> - if (src_ops->maps_tt && (!ttm || !ttm_tt_is_populated(ttm))) { >> - if (ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)) >> - return; >> - >> + if (clear) { >> for (i = 0; i < num_pages; ++i) { >> dst_ops->map_local(dst_iter, &dst_map, i); >> if (dst_map.is_iomem) >> @@ -146,6 +142,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, >> struct ttm_kmap_iter_linear_io io; >> } _dst_iter, _src_iter; >> struct ttm_kmap_iter *dst_iter, *src_iter; >> + bool clear; >> int ret = 0; >> if (ttm && ((ttm->page_flags & TTM_PAGE_FLAG_SWAPPED) || >> @@ -169,7 +166,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object >> *bo, >> goto out_src_iter; >> } >> - ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter); >> + clear = src_iter->ops->maps_tt && (!ttm || >> !ttm_tt_is_populated(ttm)); >> + if (!(clear && ttm && !(ttm->page_flags & >> TTM_PAGE_FLAG_ZERO_ALLOC))) >> + ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter); >> + >> src_copy = *src_mem; >> ttm_bo_move_sync_cleanup(bo, dst_mem); >> diff --git a/include/drm/ttm/ttm_bo_driver.h >> b/include/drm/ttm/ttm_bo_driver.h >> index 68d6069572aa..5f087575194b 100644 >> --- a/include/drm/ttm/ttm_bo_driver.h >> +++ b/include/drm/ttm/ttm_bo_driver.h >> @@ -322,7 +322,7 @@ int ttm_bo_tt_bind(struct ttm_buffer_object *bo, >> struct ttm_resource *mem); >> */ >> void ttm_bo_tt_destroy(struct ttm_buffer_object *bo); >> -void ttm_move_memcpy(struct ttm_buffer_object *bo, >> +void ttm_move_memcpy(bool clear, >> u32 num_pages, >> struct ttm_kmap_iter *dst_iter, >> struct ttm_kmap_iter *src_iter);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 4e529adcdfc7..f19847abe856 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -517,7 +517,7 @@ static void __i915_ttm_move(struct ttm_buffer_object *bo, bool clear, obj->ttm.cached_io_st, src_reg->region.start); - ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter); + ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter); } } diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 2f57f824e6db..e3747f069674 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -75,22 +75,21 @@ void ttm_mem_io_free(struct ttm_device *bdev, /** * ttm_move_memcpy - Helper to perform a memcpy ttm move operation. - * @bo: The struct ttm_buffer_object. - * @new_mem: The struct ttm_resource we're moving to (copy destination). - * @new_iter: A struct ttm_kmap_iter representing the destination resource. + * @clear: Whether to clear rather than copy. + * @num_pages: Number of pages of the operation. + * @dst_iter: A struct ttm_kmap_iter representing the destination resource. * @src_iter: A struct ttm_kmap_iter representing the source resource. * * This function is intended to be able to move out async under a * dma-fence if desired. */ -void ttm_move_memcpy(struct ttm_buffer_object *bo, +void ttm_move_memcpy(bool clear, u32 num_pages, struct ttm_kmap_iter *dst_iter, struct ttm_kmap_iter *src_iter) { const struct ttm_kmap_iter_ops *dst_ops = dst_iter->ops; const struct ttm_kmap_iter_ops *src_ops = src_iter->ops; - struct ttm_tt *ttm = bo->ttm; struct dma_buf_map src_map, dst_map; pgoff_t i; @@ -99,10 +98,7 @@ void ttm_move_memcpy(struct ttm_buffer_object *bo, return; /* Don't move nonexistent data. Clear destination instead. */ - if (src_ops->maps_tt && (!ttm || !ttm_tt_is_populated(ttm))) { - if (ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)) - return; - + if (clear) { for (i = 0; i < num_pages; ++i) { dst_ops->map_local(dst_iter, &dst_map, i); if (dst_map.is_iomem) @@ -146,6 +142,7 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, struct ttm_kmap_iter_linear_io io; } _dst_iter, _src_iter; struct ttm_kmap_iter *dst_iter, *src_iter; + bool clear; int ret = 0; if (ttm && ((ttm->page_flags & TTM_PAGE_FLAG_SWAPPED) || @@ -169,7 +166,10 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, goto out_src_iter; } - ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter); + clear = src_iter->ops->maps_tt && (!ttm || !ttm_tt_is_populated(ttm)); + if (!(clear && ttm && !(ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC))) + ttm_move_memcpy(clear, dst_mem->num_pages, dst_iter, src_iter); + src_copy = *src_mem; ttm_bo_move_sync_cleanup(bo, dst_mem); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 68d6069572aa..5f087575194b 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -322,7 +322,7 @@ int ttm_bo_tt_bind(struct ttm_buffer_object *bo, struct ttm_resource *mem); */ void ttm_bo_tt_destroy(struct ttm_buffer_object *bo); -void ttm_move_memcpy(struct ttm_buffer_object *bo, +void ttm_move_memcpy(bool clear, u32 num_pages, struct ttm_kmap_iter *dst_iter, struct ttm_kmap_iter *src_iter);
The buffer object argument to ttm_move_memcpy was only used to determine whether the destination memory should be cleared only or whether we should copy data. Replace it with a "clear" bool, and update the callers. The intention here is to be able to use ttm_move_memcpy() async under a dma-fence as a fallback if an accelerated blit fails in a security- critical path where data might leak if the blit is not properly performed. For that purpose the bo is an unsuitable argument since its relevant members might already have changed at call time. Finally, update the ttm_move_memcpy kerneldoc that seems to have ended up with a stale version. Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- drivers/gpu/drm/ttm/ttm_bo_util.c | 20 ++++++++++---------- include/drm/ttm/ttm_bo_driver.h | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-)