Re: [PATCH 3/6] drm/i915: Compact memcmp in i915_vma_compare()

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

On 12/01/2017 07:08, Tvrtko Ursulin wrote:
>
> On 11/01/2017 21:51, Chris Wilson wrote:
>> In preparation for the next patch to convert to using an anonymous union
>> and leaving the excess bytes in the union uninitialised, we first need
>> to make sure we do not compare using those uninitialised bytes. We also
>> want to preserve the compactness of the code, avoiding a second call to
>> memcmp or introducing a switch, so we take advantage of using the type
>> as an encoded size (as well as a unique identifier for each type of
>> view).
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +++++++-------
>>  drivers/gpu/drm/i915/i915_vma.h     | 15 +++++++++------
>>  2 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index 3187a260e6e1..36d85599ffc9 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -145,12 +145,6 @@ typedef uint64_t gen8_ppgtt_pml4e_t;
>>
>>  struct sg_table;
>>
>> -enum i915_ggtt_view_type {
>> -    I915_GGTT_VIEW_NORMAL = 0,
>> -    I915_GGTT_VIEW_ROTATED,
>> -    I915_GGTT_VIEW_PARTIAL,
>> -};
>> -
>>  struct intel_rotation_info {
>>      struct intel_rotation_plane_info {
>>          /* tiles */
>> @@ -184,10 +178,16 @@ static inline u64
>> intel_partial_get_page_offset(const struct intel_partial_info
>>      return pi->offset_size >> INTEL_PARTIAL_SIZE_BITS;
>>  }
>>
>> +enum i915_ggtt_view_type {
>> +    I915_GGTT_VIEW_NORMAL = 0,
>> +    I915_GGTT_VIEW_ROTATED = sizeof(struct intel_rotation_info),
>> +    I915_GGTT_VIEW_PARTIAL = sizeof(struct intel_partial_info),
>> +};
>
> I just realized this will uglify the debugfs output. In the light of
> that I think we should leave the enum human readable and instead you
> could have a static map in i915_vma_compare. Hopefully code generation
> wouldn't be affected by it.

Another alternative, perhaps better, a map of view type to printable 
names in i915_debugfs.c.

Regards,

Tvrtko

>> +
>>  struct i915_ggtt_view {
>>      enum i915_ggtt_view_type type;
>> -
>>      union {
>> +        /* Members need to contain no holes/padding */
>>          struct intel_partial_info partial;
>>          struct intel_rotation_info rotated;
>>      } params;
>> diff --git a/drivers/gpu/drm/i915/i915_vma.h
>> b/drivers/gpu/drm/i915/i915_vma.h
>> index a969bbb65871..19f049cef9e3 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.h
>> +++ b/drivers/gpu/drm/i915/i915_vma.h
>> @@ -199,15 +199,18 @@ i915_vma_compare(struct i915_vma *vma,
>>      if (cmp)
>>          return cmp;
>>
>> +    BUILD_BUG_ON(I915_GGTT_VIEW_NORMAL != 0);
>> +    cmp = vma->ggtt_view.type;
>>      if (!view)
>> -        return vma->ggtt_view.type;
>> +        return cmp;
>> +
>> +    cmp -= view->type;
>> +    if (cmp)
>> +        return cmp;
>>
>> -    if (vma->ggtt_view.type != view->type)
>> -        return vma->ggtt_view.type - view->type;
>> +    BUILD_BUG_ON(I915_GGTT_VIEW_PARTIAL == I915_GGTT_VIEW_ROTATED);
>>
>> -    return memcmp(&vma->ggtt_view.params,
>> -              &view->params,
>> -              sizeof(view->params));
>> +    return memcmp(&vma->ggtt_view.params, &view->params, view->type);
>>  }
>>
>>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level
>> cache_level,
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx