| Message ID | 20190130191825.10672-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: permit zero valued dmap in for_each_sgt_dma | expand |
Quoting Matthew Auld (2019-01-30 19:18:25) > Break on NULL iter.sgp, rather than dmap == 0, on the off chance that we > have some hypothetical selftest or similar in the future that considers > dmap = 0 to be perfectly valid. 0 == DMA_MAPPING_ERROR It wouldn't be a dma iterator at that point. for_each_sgt_device_addr, _daddr? -Chris
On Wed, 30 Jan 2019 at 20:03, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Matthew Auld (2019-01-30 19:18:25) > > Break on NULL iter.sgp, rather than dmap == 0, on the off chance that we > > have some hypothetical selftest or similar in the future that considers > > dmap = 0 to be perfectly valid. > > 0 == DMA_MAPPING_ERROR Ah, I have DMA_MAPPING_ERROR (~(dma_addr_t)0), and I guess zero is also invalid... > > It wouldn't be a dma iterator at that point. > > for_each_sgt_device_addr, _daddr? Do you mean just rename it to say for_each_sgt_device_addr, or introduce a new helper with that name? So say in ggtt_insert_entries: if (something) for_each_sgt_device_addr() {} else for_each_sgt_dma() {} ?
Quoting Matthew Auld (2019-02-11 14:39:38) > On Wed, 30 Jan 2019 at 20:03, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Matthew Auld (2019-01-30 19:18:25) > > > Break on NULL iter.sgp, rather than dmap == 0, on the off chance that we > > > have some hypothetical selftest or similar in the future that considers > > > dmap = 0 to be perfectly valid. > > > > 0 == DMA_MAPPING_ERROR > > Ah, I have DMA_MAPPING_ERROR (~(dma_addr_t)0), and I guess zero is > also invalid... > > > > > It wouldn't be a dma iterator at that point. > > > > for_each_sgt_device_addr, _daddr? > > Do you mean just rename it to say for_each_sgt_device_addr, or > introduce a new helper with that name? > > So say in ggtt_insert_entries: > > if (something) > for_each_sgt_device_addr() {} > else > for_each_sgt_dma() {} If our vfuncs naturally split down into different semantics then keeping both around can prove useful. If not, let's just call it device_addr and move on. I hope it's the former, as we may need to review some contention over dma-mapping apis and so knowing what semantics apply will be useful. (Perhaps an alternative would be to start ring-fencing x86-only code so that we have some excuse to our abuses.) -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d072f3369ee1..04f102d5bbe9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2181,7 +2181,7 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg) */ #define for_each_sgt_dma(__dmap, __iter, __sgt) \ for ((__iter) = __sgt_iter((__sgt)->sgl, true); \ - ((__dmap) = (__iter).dma + (__iter).curr); \ + ((__dmap) = (__iter).dma + (__iter).curr), (__iter).sgp; \ (((__iter).curr += I915_GTT_PAGE_SIZE) >= (__iter).max) ? \ (__iter) = __sgt_iter(__sg_next((__iter).sgp), true), 0 : 0)
Break on NULL iter.sgp, rather than dmap == 0, on the off chance that we have some hypothetical selftest or similar in the future that considers dmap = 0 to be perfectly valid. Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)