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

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

On 12/01/2017 10:05, Chris Wilson wrote:
> On Thu, Jan 12, 2017 at 07:14:55AM +0000, Tvrtko Ursulin wrote:
>> 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.
>
> Yeah, when in doubt remove guess work from the reader.

Remembered one downside of the enum->string map - the wasted space due 
sparseness. Not sure then, if it keeps code generation in check perhaps 
enum->size map would be better.

Regards,

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