On Wed, Sep 09, 2015 at 04:07:10PM +0200, Michał Winiarski wrote:
Softpin allows userspace to take greater control of GPU virtual address space and eliminates the need of relocations. It can also be used to mirror addresses between GPU and CPU (shared virtual memory). Calls to drm_intel_bo_emit_reloc are still required to build the list of drm_i915_gem_exec_objects at exec time, but no entries in relocs are created. Self-relocs don't make any sense for softpinned objects and can indicate a programming errors, thus are forbidden. Softpinned objects are marked by asterisk in debug dumps.
This patch gets the kernel interface wrong.
static int +drm_intel_gem_bo_add_softpin_target(drm_intel_bo *bo, drm_intel_bo *target_bo) +{
- drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
- drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
- drm_intel_bo_gem *target_bo_gem = (drm_intel_bo_gem *) target_bo;
- if (bo_gem->has_error)
return -ENOMEM;- if (target_bo_gem->has_error) {
bo_gem->has_error = true;return -ENOMEM;- }
- if (!target_bo_gem->is_softpin)
return -EINVAL;- if (target_bo_gem == bo_gem)
return -EINVAL;- if (bo_gem->softpin_target_count == bo_gem->softpin_target_size) {
int new_size = bo_gem->softpin_target_size * 2;if (new_size == 0)new_size = bufmgr_gem->max_relocs;bo_gem->softpin_target = realloc(bo_gem->softpin_target, new_size *sizeof(drm_intel_bo *));if (!bo_gem->softpin_target)return -ENOMEM;bo_gem->softpin_target_size = new_size;- }
- bo_gem->softpin_target[bo_gem->softpin_target_count] = target_bo;
- drm_intel_gem_bo_reference(target_bo);
- bo_gem->softpin_target_count++;
- return 0;
+}
Short-circuiting the reloc handling like this is broken without instructing the kernel via the alternative mechanisms about the special handling the buffer requires EXEC_OBJECT_WRITE, EXEC_OBJECT_NEEDS_GTT).
void drm_intel_gem_bo_clear_relocs(drm_intel_bo *bo, int start) @@ -1998,6 +2084,12 @@ drm_intel_gem_bo_clear_relocs(drm_intel_bo *bo, int start) } bo_gem->reloc_count = start;
- for (i = 0; i < bo_gem->softpin_target_count; i++) {
drm_intel_bo_gem *target_bo_gem = (drm_intel_bo_gem *) bo_gem->softpin_target[i];drm_intel_gem_bo_unreference_locked_timed(&target_bo_gem->bo, time.tv_sec);- }
- bo_gem->softpin_target_count = 0;
This would have better pursued using a batch manager. -Chris