diff mbox series

intel: Do not assert on unknown chips in drm_intel_decode_context_alloc

Message ID 20201118163601.958254-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series intel: Do not assert on unknown chips in drm_intel_decode_context_alloc | expand

Commit Message

Tvrtko Ursulin Nov. 18, 2020, 4:36 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

There is this long standing nit of igt/tools/intel_error_decode asserting
when you feed it an error state from a GPU the local libdrm does not know
of.

To fix this I need a tweak in drm_intel_decode_context_alloc to make it
not assert but just return NULL (which seems an already possible return
value).

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 intel/intel_decode.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

Comments

Chris Wilson Nov. 18, 2020, 5:04 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> There is this long standing nit of igt/tools/intel_error_decode asserting
> when you feed it an error state from a GPU the local libdrm does not know
> of.
> 
> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
> not assert but just return NULL (which seems an already possible return
> value).
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Good riddance,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Tvrtko Ursulin Nov. 19, 2020, 1:42 p.m. UTC | #2 
On 18/11/2020 17:04, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> There is this long standing nit of igt/tools/intel_error_decode asserting
>> when you feed it an error state from a GPU the local libdrm does not know
>> of.
>>
>> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
>> not assert but just return NULL (which seems an already possible return
>> value).
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Good riddance,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks, now how can push to drm and is there some testing to be 
triggered before, or after?

Regards,

Tvrtko
Chris Wilson Nov. 19, 2020, 1:52 p.m. UTC | #3 
Quoting Tvrtko Ursulin (2020-11-19 13:42:07)
> 
> On 18/11/2020 17:04, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> There is this long standing nit of igt/tools/intel_error_decode asserting
> >> when you feed it an error state from a GPU the local libdrm does not know
> >> of.
> >>
> >> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
> >> not assert but just return NULL (which seems an already possible return
> >> value).
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Good riddance,
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Thanks, now how can push to drm and is there some testing to be 
> triggered before, or after?

cd intel; for i in tests/gen*.sh; do $i; done

But clearly I haven't built libdrm since automake was dropped.
-Chris
Tvrtko Ursulin Nov. 19, 2020, 1:58 p.m. UTC | #4 
On 19/11/2020 13:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-19 13:42:07)
>>
>> On 18/11/2020 17:04, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> There is this long standing nit of igt/tools/intel_error_decode asserting
>>>> when you feed it an error state from a GPU the local libdrm does not know
>>>> of.
>>>>
>>>> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
>>>> not assert but just return NULL (which seems an already possible return
>>>> value).
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Good riddance,
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Thanks, now how can push to drm and is there some testing to be
>> triggered before, or after?
> 
> cd intel; for i in tests/gen*.sh; do $i; done
> 
> But clearly I haven't built libdrm since automake was dropped.

Thanks, all good:

$ for t in ../../intel/tests/gen*.sh; do bash -x $t; done
++ echo ../../intel/tests/gen4-3d.batch.sh
++ sed 's|\.sh$||'
+ TEST_FILENAME=../../intel/tests/gen4-3d.batch
+ ./test_decode ../../intel/tests/gen4-3d.batch
+ ret=0
+ test 0 = 1
+ exit 0
++ echo ../../intel/tests/gen5-3d.batch.sh
++ sed 's|\.sh$||'
+ TEST_FILENAME=../../intel/tests/gen5-3d.batch
+ ./test_decode ../../intel/tests/gen5-3d.batch
+ ret=0
+ test 0 = 1
+ exit 0
++ echo ../../intel/tests/gen6-3d.batch.sh
++ sed 's|\.sh$||'
+ TEST_FILENAME=../../intel/tests/gen6-3d.batch
+ ./test_decode ../../intel/tests/gen6-3d.batch
+ ret=0
+ test 0 = 1
+ exit 0
++ echo ../../intel/tests/gen7-2d-copy.batch.sh
++ sed 's|\.sh$||'
+ TEST_FILENAME=../../intel/tests/gen7-2d-copy.batch
+ ./test_decode ../../intel/tests/gen7-2d-copy.batch
+ ret=0
+ test 0 = 1
+ exit 0
++ echo ../../intel/tests/gen7-3d.batch.sh
++ sed 's|\.sh$||'
+ TEST_FILENAME=../../intel/tests/gen7-3d.batch
+ ./test_decode ../../intel/tests/gen7-3d.batch
+ ret=0
+ test 0 = 1
+ exit 0

Regards,

Tvrtko
Tvrtko Ursulin June 17, 2021, 9:20 a.m. UTC | #5 
+ a bunch of recent committers to libdrm

Guys, anyone okay to push this patch? I can resend if required.

Regards,

Tvrtko

On 19/11/2020 13:58, Tvrtko Ursulin wrote:
> 
> On 19/11/2020 13:52, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2020-11-19 13:42:07)
>>>
>>> On 18/11/2020 17:04, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> There is this long standing nit of igt/tools/intel_error_decode 
>>>>> asserting
>>>>> when you feed it an error state from a GPU the local libdrm does 
>>>>> not know
>>>>> of.
>>>>>
>>>>> To fix this I need a tweak in drm_intel_decode_context_alloc to 
>>>>> make it
>>>>> not assert but just return NULL (which seems an already possible 
>>>>> return
>>>>> value).
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Good riddance,
>>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> Thanks, now how can push to drm and is there some testing to be
>>> triggered before, or after?
>>
>> cd intel; for i in tests/gen*.sh; do $i; done
>>
>> But clearly I haven't built libdrm since automake was dropped.
> 
> Thanks, all good:
> 
> $ for t in ../../intel/tests/gen*.sh; do bash -x $t; done
> ++ echo ../../intel/tests/gen4-3d.batch.sh
> ++ sed 's|\.sh$||'
> + TEST_FILENAME=../../intel/tests/gen4-3d.batch
> + ./test_decode ../../intel/tests/gen4-3d.batch
> + ret=0
> + test 0 = 1
> + exit 0
> ++ echo ../../intel/tests/gen5-3d.batch.sh
> ++ sed 's|\.sh$||'
> + TEST_FILENAME=../../intel/tests/gen5-3d.batch
> + ./test_decode ../../intel/tests/gen5-3d.batch
> + ret=0
> + test 0 = 1
> + exit 0
> ++ echo ../../intel/tests/gen6-3d.batch.sh
> ++ sed 's|\.sh$||'
> + TEST_FILENAME=../../intel/tests/gen6-3d.batch
> + ./test_decode ../../intel/tests/gen6-3d.batch
> + ret=0
> + test 0 = 1
> + exit 0
> ++ echo ../../intel/tests/gen7-2d-copy.batch.sh
> ++ sed 's|\.sh$||'
> + TEST_FILENAME=../../intel/tests/gen7-2d-copy.batch
> + ./test_decode ../../intel/tests/gen7-2d-copy.batch
> + ret=0
> + test 0 = 1
> + exit 0
> ++ echo ../../intel/tests/gen7-3d.batch.sh
> ++ sed 's|\.sh$||'
> + TEST_FILENAME=../../intel/tests/gen7-3d.batch
> + ./test_decode ../../intel/tests/gen7-3d.batch
> + ret=0
> + test 0 = 1
> + exit 0
> 
> Regards,
> 
> Tvrtko
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/intel/intel_decode.c b/intel/intel_decode.c
index e0a516644314..be6f77984d65 100644
--- a/intel/intel_decode.c
+++ b/intel/intel_decode.c
@@ -3815,32 +3815,35 @@  drm_public struct drm_intel_decode *
 drm_intel_decode_context_alloc(uint32_t devid)
 {
 	struct drm_intel_decode *ctx;
+	int gen = 0;
 
-	ctx = calloc(1, sizeof(struct drm_intel_decode));
-	if (!ctx)
-		return NULL;
-
-	ctx->devid = devid;
-	ctx->out = stdout;
-
-	if (intel_get_genx(devid, &ctx->gen))
+	if (intel_get_genx(devid, &gen))
 		;
 	else if (IS_GEN8(devid))
-		ctx->gen = 8;
+		gen = 8;
 	else if (IS_GEN7(devid))
-		ctx->gen = 7;
+		gen = 7;
 	else if (IS_GEN6(devid))
-		ctx->gen = 6;
+		gen = 6;
 	else if (IS_GEN5(devid))
-		ctx->gen = 5;
+		gen = 5;
 	else if (IS_GEN4(devid))
-		ctx->gen = 4;
+		gen = 4;
 	else if (IS_9XX(devid))
-		ctx->gen = 3;
-	else {
-		assert(IS_GEN2(devid));
-		ctx->gen = 2;
-	}
+		gen = 3;
+	else if (IS_GEN2(devid))
+		gen = 2;
+
+	if (!gen)
+		return NULL;
+
+	ctx = calloc(1, sizeof(struct drm_intel_decode));
+	if (!ctx)
+		return NULL;
+
+	ctx->devid = devid;
+	ctx->gen = gen;
+	ctx->out = stdout;
 
 	return ctx;
 }