Re: [PATCH v8] drm/i915: Avoid writing relocs with addresses in non-canonical form

[Chris Wilson <chris@chris-wilson.co.uk>]

On Tue, Dec 29, 2015 at 06:14:19PM +0100, Michał Winiarski wrote:
> According to PRM, some parts of HW require the addresses to be in
> a canonical form, where bits [63:48] == [47]. Let's convert addresses to
> canonical form prior to relocating and return converted offsets to
> userspace. We also need to make sure that userspace is using addresses
> in canonical form in case of softpin.
> 
> v2: Whitespace fixup, gen8_canonical_addr description (Chris, Ville)
> v3: Rebase on top of softpin, fix a hole in relocate_entry,
>     s/expect/require (Chris)
> v4: Handle softpin in validate_exec_list (Chris)
> v5: Convert back to canonical form at copy_to_user time (Chris)
> v6: Don't use struct exec_object2 in place of exec_object
> v7: Use sign_extend64 for converting to canonical form (Joonas),
>     reject non-canonical and non-page-aligned offset for softpin (Chris)
> v8: Convert back to non-canonical form in a function,
>     split the test for EXEC_OBJECT_PINNED (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

I double checked that all the roundtrips between kernel<->userspace
should be idempotent, so

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Out of curiousity, you should not see any difference in runtime of
gem_exec_lut_handle.

There should also be a Testcase: and perhaps a
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92699

> +		/* Offset can be used as input (EXEC_OBJECT_PINNED), reject
> +		 * any non-page-aligned or non-canonial addresses.
                                           ^non-canonical
> +		 */
> +		if (exec[i].flags & EXEC_OBJECT_PINNED) {
> +			if (exec[i].offset !=
> +			    gen8_canonical_addr(exec[i].offset & PAGE_MASK))
> +				return -EINVAL;
> +
> +			/* From drm_mm perspective address space is continuous,
> +			 * so from this point we're always using non-canonical
> +			 * form internally.
> +			 */
> +			exec[i].offset = gen8_noncanonical_addr(exec[i].offset);
> +		}
> +
> +
  ^ extra newline
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx