Re: [igt-dev] [PATCH v16 6/8] lib: igt_gt: make gem_engine_can_store_dword() check engine class

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

On 28/03/2019 19:22, Andi Shyti wrote:
> Engines referred by class and instance are getting more populare,

Italian typo. ;)

> gem_engine_can_store_dword() should handle the situation.
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
>   lib/igt_gt.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>   lib/igt_gt.h |  6 ++++--
>   2 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 5999524326d0..630aa421ffb3 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -41,6 +41,7 @@
>   #include "intel_reg.h"
>   #include "intel_chipset.h"
>   #include "igt_dummyload.h"
> +#include "i915/gem_engine_topology.h"
>   
>   /**
>    * SECTION:igt_gt
> @@ -556,27 +557,58 @@ const struct intel_execution_engine intel_execution_engines[] = {
>   	{ NULL, 0, 0 }
>   };
>   
> -bool gem_can_store_dword(int fd, unsigned int engine)
> +static bool __gem_can_store_dword(const int gen, const struct intel_device_info *info)
>   {
> -	uint16_t devid = intel_get_drm_devid(fd);
> -	const struct intel_device_info *info = intel_get_device_info(devid);
> -	const int gen = ffs(info->gen);
> -
>   	if (gen <= 2) /* requires physical addresses */
>   		return false;
>   
>   	if (gen == 3 && (info->is_grantsdale || info->is_alviso))
>   		return false; /* only supports physical addresses */
>   
> +	if (info->is_broadwater)
> +		return false; /* Not sure yet... */
> +
> +	return true;
> +}
> +
> +bool gem_can_store_dword(int fd, uint64_t engine)

Yeah eb->flags is u64, although we don't need it all here. Okay, I don't mind.

> +{
> +	uint16_t devid = intel_get_drm_devid(fd);
> +	const struct intel_device_info *info = intel_get_device_info(devid);
> +	const int gen = ffs(info->gen);
> +
> +	if (!__gem_can_store_dword(gen, info))
> +		return false;
> +
>   	if (gen == 6 && ((engine & 0x3f) == I915_EXEC_BSD))
>   		return false; /* kills the machine! */
>   
> -	if (info->is_broadwater)
> -		return false; /* Not sure yet... */
> +	return true;
> +}
> +
> +bool gem_class_can_store_dword(int fd, uint64_t class)

But for class we definitely do not need 64 bits. u16 or unsigned int.

> +{
> +	uint16_t devid = intel_get_drm_devid(fd);
> +	const struct intel_device_info *info = intel_get_device_info(devid);
> +	const int gen = ffs(info->gen);
> +
> +	if (!__gem_can_store_dword(gen, info))
> +		return false;
> +
> +	if (gen == 6 && class == I915_ENGINE_CLASS_VIDEO)
> +		return false;
>   
>   	return true;
>   }
>   
> +bool gem_engine_can_store_dword(int fd, const struct intel_execution_engine2 *e)
> +{
> +	if (!gem_has_engine_topology(fd))
> +		return gem_can_store_dword(fd, e->flags);
> +
> +	return gem_class_can_store_dword(fd, e->class);
> +}

Couldn't you always use class?

It could be refactored with a eb_flags_to_class helper if you would like this solution. Then it wouldn't need to call gem_has_engine_topology I think.

bool gem_class_can_store_dword(int fd, uint64_t class)
{
	uint16_t devid = intel_get_drm_devid(fd);
	const struct intel_device_info *info = intel_get_device_info(devid);
	const int gen = ffs(info->gen);

	if (!__gem_can_store_dword(gen, info))
		return false;

	if (gen == 6 && class == I915_ENGINE_CLASS_VIDEO)
		return false;
 
 	return true;
 }

bool gem_can_store_dword(int fd, uint64_t engine)
{
	u16 class = eb_engine_to_class(engine);

	return gem_class_can_store_dword(class);
}

bool gem_engine_can_store_dword(int fd, const struct intel_execution_engine2 *e)
{
	return gem_class_can_store_dword(fd, e->class);
}

Unless I missed something fundamental..

Let me see the next patches first..

Regards,

Tvrtko

> +
>   const struct intel_execution_engine2 intel_execution_engines2[] = {
>   	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>   	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 52b2f1ea95a5..f3f07e895733 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -89,8 +89,6 @@ extern const struct intel_execution_engine {
>   bool gem_ring_is_physical_engine(int fd, unsigned int ring);
>   bool gem_ring_has_physical_engine(int fd, unsigned int ring);
>   
> -bool gem_can_store_dword(int fd, unsigned int engine);
> -
>   extern const struct intel_execution_engine2 {
>   	const char *name;
>   	int class;
> @@ -99,6 +97,10 @@ extern const struct intel_execution_engine2 {
>   	bool is_virtual;
>   } intel_execution_engines2[];
>   
> +bool gem_can_store_dword(int fd, uint64_t);
> +bool gem_class_can_store_dword(int fd, uint64_t class);
> +bool gem_engine_can_store_dword(int fd, const struct intel_execution_engine2 *e);
> +
>   unsigned int
>   gem_class_instance_to_eb_flags(int gem_fd,
>   			       enum drm_i915_gem_engine_class class,
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev