From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id C0D9710E54F for ; Mon, 23 Jan 2023 19:53:10 +0000 (UTC) Message-ID: Date: Mon, 23 Jan 2023 20:53:06 +0100 MIME-Version: 1.0 Content-Language: en-US To: "Das, Nirmoy" , igt-dev@lists.freedesktop.org References: <20230120100750.24122-1-nirmoy.das@intel.com> <629d1ce2-a644-1b26-9264-b048b90e68b1@intel.com> <257cb532-adef-26f1-72f4-2c836ce1eea6@intel.com> From: Andrzej Hajda In-Reply-To: <257cb532-adef-26f1-72f4-2c836ce1eea6@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [igt-dev] [PATCH i-g-t 1/2] lib/i915/intel_memory_region: Add gem_has_smallbar List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: matthew.auld@intel.com Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 >>> --- >>>   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. Reviewed-by: Andrzej Hajda 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, >>