[PATCH i-g-t 1/2] lib/intel_common: Add placeholder for common i915 and xe functions

[Zbigniew Kempczyński]

On Tue, Mar 12, 2024 at 12:43:41PM +0000, Matthew Auld wrote:
> On 12/03/2024 12:26, Zbigniew Kempczyński wrote:
> > Some constructs like querying if device is discrete or getting memory
> > region origin are common so add new file to place those functions into.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > ---
> >   lib/intel_common.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++
> >   lib/intel_common.h | 25 +++++++++++++
> >   lib/meson.build    |  1 +
> >   3 files changed, 117 insertions(+)
> >   create mode 100644 lib/intel_common.c
> >   create mode 100644 lib/intel_common.h
> > 
> > diff --git a/lib/intel_common.c b/lib/intel_common.c
> > new file mode 100644
> > index 0000000000..3cc7b7f2be
> > --- /dev/null
> > +++ b/lib/intel_common.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#include "drmtest.h"
> > +#include "intel_chipset.h"
> > +#include "intel_common.h"
> > +#include "i915/intel_memory_region.h"
> > +#include "xe/xe_query.h"
> > +
> > +/**
> > + * is_intel_dgfx:
> > + * @fd: drm fd
> > + *
> > + * Check if Intel device opened at @fd is discrete regardless driver (i915/xe).
> > + *
> > + * Returns: True if device is Intel discrete, false otherwise
> > + */
> > +bool is_intel_dgfx(int fd)
> > +{
> > +	return get_intel_driver(fd) == INTEL_DRIVER_XE ? xe_has_vram(fd) :
> > +							 gem_has_lmem(fd);
> > +}
> > +
> > +/**
> > + * is_intel_system_region:
> > + * @fd: drm fd
> > + * @region: region id
> > + *
> > + * Check if @region is system region on @fd opened Intel device.
> > + *
> > + * Returns: True if @region is system memory, false otherwise
> > + */
> > +bool is_intel_system_region(int fd, uint64_t region)
> > +{
> > +	enum intel_driver driver = get_intel_driver(fd);
> > +	bool is_system;
> > +
> > +	if (driver == INTEL_DRIVER_I915)
> > +		is_system = IS_SYSTEM_MEMORY_REGION(region);
> > +	else
> > +		is_system = (region == system_memory(fd));
> > +
> > +	return is_system;
> > +}
> > +
> > +/**
> > + * is_intel_vram_region:
> > + * @fd: drm fd
> > + * @region: region id
> > + *
> > + * Check if @region is vram (device memory) region on @fd opened Intel device.
> > + *
> > + * Returns: True if @region is vram, false otherwise
> > + */
> > +bool is_intel_vram_region(int fd, uint64_t region)
> > +{
> 
> return !is_intel_system_region(fd, region)

I thought about it but what if user will pass 'region == 0'?
This is not system and not vram, so I assumed both functions
on xe will return false. I may assert on region == 0 on xe
to make sure user won't pass invalid region.

> 
> ?
> 
> > +	enum intel_driver driver = get_intel_driver(fd);
> > +	bool is_vram;
> > +
> > +	if (driver == INTEL_DRIVER_I915)
> > +		is_vram = IS_DEVICE_MEMORY_REGION(region);
> > +	else
> > +		is_vram = (region & (all_memory_regions(fd) & (~system_memory(fd))));
> > +
> > +	return is_vram;
> > +}
> > +
> > +/**
> > + * is_intel_region_compressible:
> > + * @fd: drm fd
> > + * @region: region id
> > + *
> > + * Check if @region is compressible on @fd opened Intel device.
> > + *
> > + * Returns: True if @region is compressible, false otherwise
> > + */
> > +bool is_intel_region_compressible(int fd, uint64_t region)
> > +{
> > +	bool is_dgfx = is_intel_dgfx(fd);
> > +	bool is_vram = is_intel_vram_region(fd, region);
> > +
> > +	if (is_dgfx && is_vram)
> > +		return true;
> > +
> > +	if (AT_LEAST_GEN(intel_get_drm_devid(fd), 20) && !is_dgfx)
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> Just thinking about DG1. It's dgpu but I don't think it has flat-ccs, so I
> assume it is using aux-ccs and therefore anything is compressible? But here
> we return true for vram only? We also also have older igpu platforms that
> use aux-ccs, but we return false here. Perhaps we mean
> is_intel_region_flat_ccs_compressible() ? Or we need to check if the
> platform has_flatccs() && is_dgfx ?

Agree, I silently bypassed DG1 and aux-ccs platforms. I think it makes
sense to have dedicated helpers for flatccs and aux-ccs grouping in
is_intel_region_compressible(). I'm going to slightly reshape and
respin.

Thanks for the review.

--
Zbigniew

> 
> Otherwise I think looks good.
> 
> > diff --git a/lib/intel_common.h b/lib/intel_common.h
> > new file mode 100644
> > index 0000000000..924802473b
> > --- /dev/null
> > +++ b/lib/intel_common.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#ifndef __INTEL_COMMON_H__
> > +#define __INTEL_COMMON_H__
> > +
> > +/**
> > + * SECTION:intel_common
> > + * @short_description: i915/xe common library code
> > + * @title: Intel library
> > + * @include: intel_common.h
> > + *
> > + */
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> > +bool is_intel_dgfx(int fd);
> > +bool is_intel_system_region(int fd, uint64_t region);
> > +bool is_intel_vram_region(int fd, uint64_t region);
> > +bool is_intel_region_compressible(int fd, uint64_t region);
> > +
> > +#endif
> > diff --git a/lib/meson.build b/lib/meson.build
> > index 6122861d8b..934bac5c67 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -59,6 +59,7 @@ lib_sources = [
> >   	'intel_bufops.c',
> >   	'intel_chipset.c',
> >   	'intel_cmds_info.c',
> > +	'intel_common.c',
> >   	'intel_compute.c',
> >   	'intel_compute_square_kernels.c',
> >   	'intel_ctx.c',