Re: [PATCH 10/11] drm/i915: Support to create write combined type vmaps

Chris Wilson
2016-06-28T09:52:48Z

On Mon, Jun 27, 2016 at 05:46:57PM +0530, akash.goel@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> vmaps has a provision for controlling the page protection bits, with which
> we can use to control the mapping type, e.g. WB, WC, UC or even WT.
> To allow the caller to choose their mapping type, we add a parameter to
> i915_gem_object_pin_map - but we still only allow one vmap to be cached
> per object. If the object is currently not pinned, then we recreate the
> previous vmap with the new access type, but if it was pinned we report an
> error. This effectively limits the access via i915_gem_object_pin_map to a
> single mapping type for the lifetime of the object. Not usually a problem,
> but something to be aware of when setting up the object's vmap.
>
> We will want to vary the access type to enable WC mappings of ringbuffer
> and context objects on !llc platforms, as well as other objects where we
> need coherent access to the GPU's pages without going through the GTT
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 ++-
> drivers/gpu/drm/i915/i915_gem.c | 57 +++++++++++++++++++++++++--------
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 8 ++---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
> 5 files changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20c701c..3ef1ee5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3197,6 +3197,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
> /**
> * i915_gem_object_pin_map - return a contiguous mapping of the entire object
> * @obj - the object to map into kernel address space
> + * &use_wc - whether the mapping should be using WC or WB pgprot_t

s/&/@/ I think

> /* get, pin, and map the pages of the object into kernel space */
> -void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
> +void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, bool use_wc)
> {
> + void *ptr;
> + bool has_wc;
> + bool pinned;
> int ret;
>
> lockdep_assert_held(&obj->base.dev->struct_mutex);
> + GEM_BUG_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0);
>
> ret = i915_gem_object_get_pages(obj);
> if (ret)
> return ERR_PTR(ret);
>
> + GEM_BUG_ON(obj->pages == NULL);
> i915_gem_object_pin_pages(obj);
>
> - if (!obj->mapping) {
> - obj->mapping = i915_gem_object_map(obj);
> - if (!obj->mapping) {
> - i915_gem_object_unpin_pages(obj);
> - return ERR_PTR(-ENOMEM);
> + pinned = (obj->pages_pin_count > 1);

Too many ()

Hmm. It may look a bit dubious if I add my r-b here. But I didn't spot
any rebasing errors.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre