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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/07/16 17:30, Goel, Akash wrote:
> On 7/15/2016 5:01 PM, Tvrtko Ursulin wrote:
>> On 10/07/16 14:41, 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
>>>
>>> v2: Remove the redundant braces around pin count check and fix the
>>> marker
>>>      in documentation (Chris)
>>>
>>> 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/i915_guc_submission.c |  2 +-
>>>   drivers/gpu/drm/i915/intel_lrc.c           |  8 ++---
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c    |  2 +-
>>>   6 files changed, 54 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 6e2ddfa..84afa17 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3248,6 +3248,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
>>>    *
>>>    * Calls i915_gem_object_pin_pages() to prevent reaping of the
>>> object's
>>>    * pages and then returns a contiguous mapping of the backing
>>> storage into
>>> @@ -3259,7 +3260,8 @@ static inline void
>>> i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>>>    * Returns the pointer through which to access the mapped object,
>>> or an
>>>    * ERR_PTR() on error.
>>>    */
>>> -void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object
>>> *obj);
>>> +void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object
>>> *obj,
>>> +                     bool use_wc);
>>
>> Could you make it an enum instead of a bool? Commit message suggests
>> more modes will potentially be added and if so, and we start with an
>> enum straight away, it will make for less churn in the future.
>>
>> func(something, true) is always also quite unreadabe in the code because
>> one has to remember or remind himself what it really means.
>>
>> Something like func(something, MAP_WC) would be simply self-documenting.
>>
> Thanks nice suggestion, will do that.
> enum only or macros also will do ?
> #define MAP_CACHED    0x1
> #define MAP_WC        0x2

I think enum is better since you then have type checking on the arguments.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx