Re: [PATCH igt] tests/gem_workarounds: Skip write only registers

[Oscar Mateo <oscar.mateo@intel.com>]

On 09/28/2017 08:00 AM, Mika Kuoppala wrote:
> We have no means to check write only registers as
> this would need access through context image. For now we
> know that cnl has a one such register, 0xe5f0 which is used
> to set WaForceContextSaveRestoreNonCoherent:cnl. By inspecting
> the context image without and with workaround applied:
>
> 0x0000a840: 0x0000e5f0 0xffff0800
> 0x0000a840: 0x0000e5f0 0xffff0820
>
> we can conclude that the workaround setup is working right
> in this particular case.
>
> Add a write only list and add register 0xe5f0 into this list.
> This is a temporary solution until a more capable context image
> checker emerges.
>
> v2: add code comment about adhocness (Petri)
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102943
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Acked-by: Oscar Mateo <oscar.mateo@intel.com>

>   tests/gem_workarounds.c | 38 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c
> index d6641bd5..5e30a7b8 100644
> --- a/tests/gem_workarounds.c
> +++ b/tests/gem_workarounds.c
> @@ -29,6 +29,8 @@
>   
>   #include <fcntl.h>
>   
> +static int gen;
> +
>   enum operation {
>   	GPU_RESET,
>   	SUSPEND_RESUME,
> @@ -41,6 +43,21 @@ struct intel_wa_reg {
>   	uint32_t mask;
>   };
>   
> +static struct write_only_list {
> +	unsigned int gen;
> +	uint32_t addr;
> +} wo_list[] = {
> +	{ 10, 0xE5F0 } /* WaForceContextSaveRestoreNonCoherent:cnl */
> +
> +	/*
> +	 * FIXME: If you are contemplating adding stuff here
> +	 * consider this as a temporary solution. You need to
> +	 * manually check from context image that your workaround
> +	 * is having an effect. Consider creating a context image
> +	 * validator to act as a superior solution.
> +	 */
> +};
> +
>   static struct intel_wa_reg *wa_regs;
>   static int num_wa_regs;
>   
> @@ -64,6 +81,21 @@ static void test_suspend_resume(void)
>   	igt_system_suspend_autoresume(SUSPEND_STATE_MEM, SUSPEND_TEST_NONE);
>   }
>   
> +static bool write_only(const uint32_t addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(wo_list); i++) {
> +		if (gen == wo_list[i].gen &&
> +		    addr == wo_list[i].addr) {
> +			igt_info("Skipping check for 0x%x due to write only\n", addr);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>   static int workaround_fail_count(void)
>   {
>   	int i, fail_count = 0;
> @@ -85,6 +117,9 @@ static int workaround_fail_count(void)
>   			  wa_regs[i].addr, wa_regs[i].value, wa_regs[i].mask,
>   			  val, ok ? "OK" : "FAIL");
>   
> +		if (write_only(wa_regs[i].addr))
> +			continue;
> +
>   		if (!ok) {
>   			igt_warn("0x%05X\t0x%08X\t0x%08X\t0x%08X\t%s\n",
>   				 wa_regs[i].addr, wa_regs[i].value,
> @@ -124,7 +159,6 @@ igt_main
>   {
>   	igt_fixture {
>   		int device = drm_open_driver_master(DRIVER_INTEL);
> -		const int gen = intel_gen(intel_get_drm_devid(device));
>   		struct pci_device *pci_dev;
>   		FILE *file;
>   		char *line = NULL;
> @@ -133,6 +167,8 @@ igt_main
>   
>   		igt_require_gem(device);
>   
> +		gen = intel_gen(intel_get_drm_devid(device));
> +
>   		pci_dev = intel_get_pci_device();
>   		igt_require(pci_dev);
>   

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx