[igt-dev] [PATCH i-g-t 1/3] i915/gem_engine_topology: Only use the main copy engines for XY_BLOCK_COPY

[Zbigniew Kempczyński]

On Fri, Feb 25, 2022 at 03:50:41PM +0100, Kamil Konieczny wrote:
> Hi Zbigniew,
> 
> can you change patch subject ? The patch itself adds two general
> helper functions to lib, and one helper for checking copy
> capability. It do not uses it by itself. Maybe change it to
> something like : Add capability helpers.

Ok, I will change it.

> 
> Dnia 2022-02-25 at 12:06:24 +0100, Zbigniew Kempczyński napisał(a):
> > From: Chris Wilson <chris.p.wilson at intel.com>
> > 
> 
> Below is description about why change is needed, it's ok.
> 
> > XY_BLOCK_COPY blt command is used to transfer the ccs data.
> 
> Imho it would be helpfull here to put full description of ccs,
> "ccs compressed data". There is also compute engine and it has
> the same shorthand ccs.

Ccs data is ok for me. In this context it implicates compressed data,
not compute command streamer. XY_BLOCK_COPY_BLT is blitter command
so it is unrelated to CCS.

> 
> > So, we can only run the tests on those engines which have
> 
> s/So, We/We/

Ok.
> 
> > support for the "block_copy" capability.
> 
> What is missing here is description about what was changed in lib.

Ok, I will change it.

> 
> > 
> > Signed-off-by: Chris Wilson <chris.p.wilson at intel.com>
> > Signed-off-by: Apoorva Singh <apoorva1.singh at intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Melkaveri, Arjun <arjun.melkaveri at intel.com>
> > ---
> >  lib/i915/gem_engine_topology.c | 38 ++++++++++++++++++++++++++++++++++
> >  lib/i915/gem_engine_topology.h |  5 +++++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> > index bd12d0bc9..18fc00ab0 100644
> > --- a/lib/i915/gem_engine_topology.c
> > +++ b/lib/i915/gem_engine_topology.c
> > @@ -534,6 +534,44 @@ void gem_engine_properties_restore(int fd, const struct gem_engine_properties *s
> >  	}
> >  }
> >  
> > +static bool
> > +__gem_engine_has_capability(int i915, const char *engine,
> > +			    const char *attr, const char *cap)
> > +{
> > +	char buf[4096] = {};
> > +	FILE *file;
> > +
> > +	file = __open_attr(igt_sysfs_open(i915), "r",
> > +			   "engine", engine, attr, NULL);
> > +	if (file) {
> > +		fread(buf, 1, sizeof(buf) - 1, file);
> > +		fclose(file);
> > +	}
> 
> What happens when file==NULL ? Should'nt we return NULL immediatly ?
> Maybe re-arrange it like:
> 	if(!file)
> 		return NULL;

Ok, I will modify it.

> 
> What about buffer end ? Or when number of readed bytes will be zero ?
> I think we should add null string terminator at end of readed buf.

What for? char buf[4096] = {} initializes it to zero so if you'll read
zero bytes strstr() will return NULL.

> 
> > +
> > +	return strstr(buf, cap);
> > +}
> 
> One problem with this function is it do not differentiate between
> when it was zero read (so it do not helped us) and when i915
> driver fills it and we can actually check capabilities here.

We are interested in positive scenario where kernel reports caps. Negative
case - kernel reports caps but doesn't support or caps file doesn't
exits is treated as negative. 

> 
> > +
> > +bool gem_engine_has_capability(int i915, const char *engine, const char *cap)
> > +{
> > +	return __gem_engine_has_capability(i915, engine, "capabilities", cap);
> > +}
> > +
> > +bool gem_engine_has_known_capability(int i915, const char *engine, const char *cap)
> > +{
> > +	return __gem_engine_has_capability(i915, engine, "known_capabilities", cap);
> > +}
> > +
> > +bool gem_engine_can_block_copy(int i915, const struct intel_execution_engine2 *engine)
> > +{
> > +	if (engine->class != I915_ENGINE_CLASS_COPY)
> > +		return false;
> > +
> > +	if (!gem_engine_has_known_capability(i915, engine->name, "block_copy"))
> > +		return intel_gen(intel_get_drm_devid(i915)) >= 12;
> 
> This looks like guessing but maybe it is ok.

If kernel doesn't support reporting caps we assume gen 12 is enough to claim
we can execute block-copy.

--
Zbigniew

> 
> > +
> > +	return gem_engine_has_capability(i915, engine->name, "block_copy");
> > +}
> > +
> >  uint32_t gem_engine_mmio_base(int i915, const char *engine)
> >  {
> >  	unsigned int mmio = 0;
> > diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> > index b413aa8ab..987f2bf94 100644
> > --- a/lib/i915/gem_engine_topology.h
> > +++ b/lib/i915/gem_engine_topology.h
> > @@ -133,6 +133,11 @@ int gem_engine_property_printf(int i915, const char *engine, const char *attr,
> >  
> >  uint32_t gem_engine_mmio_base(int i915, const char *engine);
> >  
> > +bool gem_engine_has_capability(int i915, const char *engine, const char *cap);
> > +bool gem_engine_has_known_capability(int i915, const char *engine, const char *cap);
> > +
> > +bool gem_engine_can_block_copy(int i915, const struct intel_execution_engine2 *engine);
> > +
> >  void dyn_sysfs_engines(int i915, int engines, const char *file,
> >  		       void (*test)(int i915, int engine));
> >  
> > -- 
> > 2.32.0
> > 
> Regards,
> Kamil Konieczny