Re: [igt-dev] [PATCH i-g-t 1/2] lib/i915/intel_memory_region: Add gem_has_smallbar

["Das, Nirmoy" <nirmoy.das@linux.intel.com>]

On 1/23/2023 8:53 PM, Andrzej Hajda wrote:
>
>
> On 23.01.2023 16:45, Das, Nirmoy wrote:
>>
>> On 1/23/2023 4:16 PM, Andrzej Hajda wrote:
>>>
>>>
>>> On 20.01.2023 11:07, Nirmoy Das wrote:
>>>> Add gem_has_smallbar() to detect a device with smallbar.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>   lib/i915/intel_memory_region.c | 32 ++++++++++++++++++++++++++++++++
>>>>   lib/i915/intel_memory_region.h |  1 +
>>>>   2 files changed, 33 insertions(+)
>>>>
>>>> diff --git a/lib/i915/intel_memory_region.c 
>>>> b/lib/i915/intel_memory_region.c
>>>> index 84e1bceb3..655720819 100644
>>>> --- a/lib/i915/intel_memory_region.c
>>>> +++ b/lib/i915/intel_memory_region.c
>>>> @@ -183,6 +183,38 @@ bool gem_has_lmem(int fd)
>>>>       return gem_get_lmem_region_count(fd) > 0;
>>>>   }
>>>>   +/**
>>>> + * gem_has_smallbar:
>>>> + * @fd: open i915 drm file descriptor
>>>> + *
>>>> + * Helper function to check if the device comes with small-bar.
>>>> + *
>>>> + * Returns: True if at least one lmem region was found.
>>>> + */
>>>> +bool gem_has_smallbar(int fd)
>>>> +{
>>>> +    struct drm_i915_query_memory_regions *info;
>>>> +    struct drm_i915_memory_region_info *rf;
>>>> +    bool ret = false;
>>>> +
>>>> +    info = gem_get_query_memory_regions(fd);
>>>> +    igt_assert(info);
>>>> +
>>>> +    for (int i = 0; i < info->num_regions; i++) {
>>>> +        rf = &info->regions[i];
>>>> +        if (rf->region.memory_class == I915_MEMORY_CLASS_DEVICE) {
>>>> +            if (rf->probed_size > rf->probed_cpu_visible_size) {
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +    free(info);
>>>
>>> Why not for_each_memory_region ?
>>
>>
>> No particular reason, it is inspired by existing code from the same 
>> file.  for_each_memory_region() though calls into 
>> gem_get_query_memory_regions() eventually.
>
> I though it could be shorter, sth like:
> bool gem_has_smallbar(int fd)
> {
>     bool ret;
>
>     for_each_memory_region(r, fd)
>         if (r->ci.memory_class == I915_MEMORY_CLASS_DEVICE && r->size 
> > r->cpu_size)
>             ret = true;
>     return ret;
> }
>
> Apparently 'break' leaks memory, so it is not so perfect :)
> Up to you.
>

Looks good but because you r-b this version, lazy me will take it 
without re-spinning it :)

> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>


Thanks, Andrzej.


Nirmoy

>
>
> Regards
> Andrzej
>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>> Regards
>>> Andrzej
>>>
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +
>>>>   /* A version of gem_create_in_memory_region_list which can be 
>>>> allowed to
>>>>      fail so that the object creation can be retried */
>>>>   int __gem_create_in_memory_region_list(int fd, uint32_t *handle, 
>>>> uint64_t *size, uint32_t flags,
>>>> diff --git a/lib/i915/intel_memory_region.h 
>>>> b/lib/i915/intel_memory_region.h
>>>> index 425bda0ec..9e24bd8fb 100644
>>>> --- a/lib/i915/intel_memory_region.h
>>>> +++ b/lib/i915/intel_memory_region.h
>>>> @@ -62,6 +62,7 @@ struct drm_i915_query_memory_regions 
>>>> *gem_get_query_memory_regions(int fd);
>>>>   unsigned int gem_get_lmem_region_count(int fd);
>>>>     bool gem_has_lmem(int fd);
>>>> +bool gem_has_smallbar(int fd);
>>>>     int __gem_create_in_memory_region_list(int fd, uint32_t 
>>>> *handle, uint64_t *size, uint32_t flags,
>>>>                          const struct 
>>>> drm_i915_gem_memory_class_instance *mem_regions,
>>>
>