Skip to contentSkip to conversation

drm/i915: Don't try to handle GuC when GuC is not supported.

PATCH In review Rodrigo Vivi 5 participants 9 messages Sep 21, 2016 – Oct 6, 2016

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Rodrigo Vivi

author maintainer
#0 lore raw

PATCH 1/1 drm/i915: Don't try to handle GuC when GuC is not supported.

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Besides being useless handling, our driver needs
to be smarter than the user trying to use an invalid paramenter.

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +7 -0
index 6fd39ef..da0f5ed 100644
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}
+
 	/* A negative value means "use platform default" */
 	if (i915.enable_guc_loading < 0)
 		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#1 lore raw

✓ Fi.CI.BAT: success for drm/i915: Don't try to handle GuC when GuC is not supported.

== Series Details ==

== Series Details ==

Series: drm/i915: Don't try to handle GuC when GuC is not supported.
URL : https://patchwork.freedesktop.org/series/12753/
State : success

== Summary ==

Series 12753v1 drm/i915: Don't try to handle GuC when GuC is not supported.
https://patchwork.freedesktop.org/api/1.0/series/12753/revisions/1/mbox/

Test kms_pipe_crc_basic:

        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-skl-6770hq)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq)
fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
fi-byt-n2820     total:244  pass:208  dwarn:0   dfail:0   fail:1   skip:35 
fi-hsw-4770k     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6700k     total:244  pass:219  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:244  pass:228  dwarn:1   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:244  pass:208  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37

Results at /archive/results/CI_IGT_test/Patchwork_2565/

463d07a32d87742a73e1ed352a6d6daa3f29d0c2 drm-intel-nightly: 2016y-09m-21d-16h-35m-54s UTC integration manifest 00e8796 drm/i915: Don't try to handle GuC when GuC is not supported.

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#2 lore raw
In reply to Rodrigo Vivi

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 5 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Besides being useless handling, our driver needs
to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user passes i915.enable_guc_something=1, right?

Quoted 19 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>

---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++

 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device *dev)
  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
  	const char *fw_path;

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device *dev) · line 728

Quoted 6 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
}

Or we could even go with our current "design pattern" and create intel_sanitize_guc_options().

This way we'll be able to avoid adding a second failure code path, since we already have one for platforms with guc but options disabled.

Quoted 4 lines

+
  /* A negative value means "use platform default" */ if (i915.enable_guc_loading < 0) i915.enable_guc_loading = HAS_GUC_UCODE(dev);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author maintainer
#3 lore raw
Quoted 9 lines · Paulo Zanoni

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 5 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Besides being useless handling, our driver needs
to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user passes i915.enable_guc_something=1, right?

1 is not a problem actually since it means "use if available". There is not firmware and execution continues.

2 is the problem because it means "use guc or fail if not available". But platforms that don't have guc can't fail. driver needs to be smarter than that.

Quoted 29 lines
Quoted 17 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device *dev) · line 728

Quoted 9 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

Quoted 10 lines
if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
}

yeap, this works as well. I just went for the simplest option that minimized at most any interactions for platforms where GuC simply doesn't exist.

Quoted 3 lines

Or we could even go with our current "design pattern" and create intel_sanitize_guc_options().

This is indeed a very good idea.

Quoted 9 lines

This way we'll be able to avoid adding a second failure code path, since we already have one for platforms with guc but options disabled.

Quoted 4 lines
drivers/gpu/drm/i915/intel_guc_loader.c +1 -0
+
 	/* A negative value means "use platform default" */
 	if (i915.enable_guc_loading < 0)
 		i915.enable_guc_loading = HAS_GUC_UCODE(dev);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#4 lore raw
Quoted 11 lines · Rodrigo
Quoted 9 lines · Paulo Zanoni

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 5 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Besides being useless handling, our driver needs
to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user passes i915.enable_guc_something=1, right?

Quoted 6 lines

1 is not a problem actually since it means "use if available". There is not firmware and execution continues.

2 is the problem because it means "use guc or fail if not available". But platforms that don't have guc can't fail. driver needs to be smarter than that.

Not sure it needs to be smarter than that really, since all these debug options auto-taint the kernel if you touch them. As in: You get to keep all the pieces.

We can still do some auto-cleanup of modoptions ofc if there's a good need for them.
-Daniel

Quoted 65 lines
Quoted 30 lines
Quoted 18 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device *dev) · line 728

Quoted 9 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

Quoted 10 lines
if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
}

yeap, this works as well. I just went for the simplest option that minimized at most any interactions for platforms where GuC simply doesn't exist.

Quoted 3 lines

Or we could even go with our current "design pattern" and create intel_sanitize_guc_options().

This is indeed a very good idea.

Quoted 9 lines

This way we'll be able to avoid adding a second failure code path, since we already have one for platforms with guc but options disabled.

Quoted 4 lines
drivers/gpu/drm/i915/intel_guc_loader.c +1 -0
+
 	/* A negative value means "use platform default" */
 	if (i915.enable_guc_loading < 0)
 		i915.enable_guc_loading = HAS_GUC_UCODE(dev);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author maintainer
#5 lore raw

Hi Daniel,

So, can we close https://bugs.freedesktop.org/show_bug.cgi?id=97573 with wontfix or notabug?

I don't have a strong side on that actually, but Jani was against it it seems.

Thanks,
Rodrigo.

Quoted 19 lines · Daniel Vetter
Quoted 11 lines · Rodrigo
Quoted 9 lines · Paulo Zanoni

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 5 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Besides being useless handling, our driver needs
to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user passes i915.enable_guc_something=1, right?

Quoted 6 lines

1 is not a problem actually since it means "use if available". There is not firmware and execution continues.

2 is the problem because it means "use guc or fail if not available". But platforms that don't have guc can't fail. driver needs to be smarter than that.

Quoted 74 lines

Not sure it needs to be smarter than that really, since all these debug options auto-taint the kernel if you touch them. As in: You get to keep all the pieces.

We can still do some auto-cleanup of modoptions ofc if there's a good need for them.
-Daniel

Quoted 65 lines
Quoted 30 lines
Quoted 18 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device *dev) · line 728

Quoted 9 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

Quoted 10 lines
if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
}

yeap, this works as well. I just went for the simplest option that minimized at most any interactions for platforms where GuC simply doesn't exist.

Quoted 3 lines

Or we could even go with our current "design pattern" and create intel_sanitize_guc_options().

This is indeed a very good idea.

Quoted 9 lines

This way we'll be able to avoid adding a second failure code path, since we already have one for platforms with guc but options disabled.

Quoted 4 lines
drivers/gpu/drm/i915/intel_guc_loader.c +1 -0
+
 	/* A negative value means "use platform default" */
 	if (i915.enable_guc_loading < 0)
 		i915.enable_guc_loading = HAS_GUC_UCODE(dev);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#6 lore raw

Em Qua, 2016-10-05 às 23:37 +0000, Vivi, Rodrigo escreveu:

Quoted 9 lines

Hi Daniel,

So, can we close https://bugs.freedesktop.org/show_bug.cgi?id=97573
with
wontfix or notabug?

I don't have a strong side on that actually, but Jani was against it it
seems.

Just my opinion:

Considering that we already identified the problem and the fix is simple, I really think it's better to just fix it. In fact I thought you were going to submit V2 and I was planning to do the review.

Fixing this may help reducing future bug triaging time, because if we keep the problem unfixed we may get the same bug report again and again and again. It's easy to say "users get to keep all the pieces of the broken kernel", but we usually have to triage the problems regardless, discuss what to do, etc.

Quoted 125 lines

Thanks,
Rodrigo.

Quoted 119 lines · Daniel Vetter
Quoted 20 lines · Rodrigo
Quoted 6 lines · Paulo Zanoni

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 4 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Quoted 7 lines
Quoted 3 lines

Besides being useless handling, our driver needs to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user
passes i915.enable_guc_something=1, right?

1 is not a problem actually since it means "use if available". There is
not firmware and execution continues.

Quoted 5 lines

2 is the problem because it means "use guc or fail if not available".
But platforms that don't have guc can't fail. driver needs to be smarter
than that.

Not sure it needs to be smarter than that really, since all these debug
options auto-taint the kernel if you touch them. As in: You get to keep
all the pieces.

We can still do some auto-cleanup of modoptions ofc if there's a good need
for them.
-Daniel

Quoted 79 lines
Quoted 44 lines
Quoted 25 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>

---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++

 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device
 *dev)
  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
  	const char *fw_path;

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device *dev) · line 728

Quoted 6 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading =
HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission =
Quoted 2 lines

HAS_GUC_SCHED(dev_priv);
}

yeap, this works as well. I just went for the simplest option that
minimized at most any interactions for platforms where GuC simply doesn't exist.

Quoted 5 lines

Or we could even go with our current "design pattern" and create
intel_sanitize_guc_options().

This is indeed a very good idea.

Quoted 14 lines

This way we'll be able to avoid adding a second failure code path,
since we already have one for platforms with guc but options disabled.

Quoted 6 lines

+
  /* A negative value means "use platform default" */ if (i915.enable_guc_loading < 0) i915.enable_guc_loading = HAS_GUC_UCODE(dev);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#7 lore raw
Quoted 17 lines · Paulo Zanoni

Em Qua, 2016-10-05 às 23:37 +0000, Vivi, Rodrigo escreveu:

Quoted 9 lines

Hi Daniel,

So, can we close https://bugs.freedesktop.org/show_bug.cgi?id=97573
with
wontfix or notabug?

I don't have a strong side on that actually, but Jani was against it it
seems.

Just my opinion:

Considering that we already identified the problem and the fix is simple, I really think it's better to just fix it. In fact I thought you were going to submit V2 and I was planning to do the review.

Quoted 5 lines

Fixing this may help reducing future bug triaging time, because if we keep the problem unfixed we may get the same bug report again and again and again. It's easy to say "users get to keep all the pieces of the broken kernel", but we usually have to triage the problems regardless, discuss what to do, etc.

All, please stop the discussion and just do one or the other. I'll ack either approach.

http://dilbert.com/strip/1998-11-10

BR,
Jani.

Quoted 126 lines
Quoted 126 lines

Thanks,
Rodrigo.

Quoted 119 lines · Daniel Vetter
Quoted 20 lines · Rodrigo
Quoted 6 lines · Paulo Zanoni

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 4 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Quoted 7 lines
Quoted 3 lines

Besides being useless handling, our driver needs to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user
passes i915.enable_guc_something=1, right?

1 is not a problem actually since it means "use if available". There is
not firmware and execution continues.

Quoted 5 lines

2 is the problem because it means "use guc or fail if not available".
But platforms that don't have guc can't fail. driver needs to be smarter
than that.

Not sure it needs to be smarter than that really, since all these debug
options auto-taint the kernel if you touch them. As in: You get to keep
all the pieces.

We can still do some auto-cleanup of modoptions ofc if there's a good need
for them.
-Daniel

Quoted 79 lines
Quoted 44 lines
Quoted 25 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>

---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++

 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device
 *dev)
  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
  	const char *fw_path;

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device *dev) · line 728

Quoted 6 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading =
HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission =
Quoted 2 lines

HAS_GUC_SCHED(dev_priv);
}

yeap, this works as well. I just went for the simplest option that
minimized at most any interactions for platforms where GuC simply doesn't exist.

Quoted 5 lines

Or we could even go with our current "design pattern" and create
intel_sanitize_guc_options().

This is indeed a very good idea.

Quoted 14 lines

This way we'll be able to avoid adding a second failure code path,
since we already have one for platforms with guc but options disabled.

Quoted 6 lines

+
  /* A negative value means "use platform default" */ if (i915.enable_guc_loading < 0) i915.enable_guc_loading = HAS_GUC_UCODE(dev);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author maintainer
#8 lore raw
In reply to Paulo Zanoni
Quoted 17 lines · Paulo Zanoni

Em Qua, 2016-10-05 às 23:37 +0000, Vivi, Rodrigo escreveu:

Quoted 9 lines

Hi Daniel,

So, can we close https://bugs.freedesktop.org/show_bug.cgi?id=97573
with
wontfix or notabug?

I don't have a strong side on that actually, but Jani was against it it
seems.

Just my opinion:

Considering that we already identified the problem and the fix is simple, I really think it's better to just fix it. In fact I thought you were going to submit V2 and I was planning to do the review.

Quoted 5 lines

Fixing this may help reducing future bug triaging time, because if we keep the problem unfixed we may get the same bug report again and again and again. It's easy to say "users get to keep all the pieces of the broken kernel", but we usually have to triage the problems regardless, discuss what to do, etc.

Here it is:
https://patchwork.freedesktop.org/patch/113922/

Quoted 120 lines
Quoted 120 lines

Thanks,
Rodrigo.

Quoted 114 lines · Daniel Vetter
Quoted 20 lines · Rodrigo
Quoted 6 lines · Paulo Zanoni

Em Qua, 2016-09-21 às 11:22 -0700, Rodrigo Vivi escreveu:

Quoted 4 lines

Avoid any kind of GuC handling if GuC is not supported on a giving platform.

Quoted 7 lines
Quoted 3 lines

Besides being useless handling, our driver needs to be smarter than the user trying to use an invalid paramenter.

So the problem is when a platform doesn't support guc and the user
passes i915.enable_guc_something=1, right?

1 is not a problem actually since it means "use if available". There is
not firmware and execution continues.

Quoted 5 lines

2 is the problem because it means "use guc or fail if not available".
But platforms that don't have guc can't fail. driver needs to be smarter
than that.

Not sure it needs to be smarter than that really, since all these debug
options auto-taint the kernel if you touch them. As in: You get to keep
all the pieces.

We can still do some auto-cleanup of modoptions ofc if there's a good need
for them.
-Daniel

Quoted 75 lines
Quoted 44 lines
Quoted 21 lines

Cc: Jani Nikula <[email protected]>
Cc: Anusha Srivatsa <[email protected]>
Cc: Christophe Prigent <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97573

Signed-off-by Rodrigo Vivi <[email protected]>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/gpu/drm/i915/intel_guc_loader.c +0 -0
index 6fd39ef..da0f5ed 100644

drivers/gpu/drm/i915/intel_guc_loader.c void intel_guc_init(struct drm_device · line 729

Quoted 10 lines
drivers/gpu/drm/i915/intel_guc_loader.c +6 -0
@@ -720,6 +720,13 @@ void intel_guc_init(struct drm_device
 *dev)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
+	if (!HAS_GUC(dev)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+		fw_path = NULL;
+		return;
+	}

Instead of this, how about we just patch the code below with:

if (!HAS_GUC(dev_priv)) {
	i915.enable_guc_loading = 0;
	i915.enable_guc_submission = 0;
} else {
	/* A negative value means "use platform default" */
	if (i915.enable_guc_loading < 0)
		i915.enable_guc_loading =
HAS_GUC_UCODE(dev_priv);
	if (i915.enable_guc_submission < 0)
		i915.enable_guc_submission =
Quoted 2 lines

HAS_GUC_SCHED(dev_priv);
}

yeap, this works as well. I just went for the simplest option that
minimized at most any interactions for platforms where GuC simply doesn't exist.

Quoted 5 lines

Or we could even go with our current "design pattern" and create
intel_sanitize_guc_options().

This is indeed a very good idea.

Quoted 14 lines

This way we'll be able to avoid adding a second failure code path,
since we already have one for platforms with guc but options disabled.

Quoted 6 lines
drivers/gpu/drm/i915/intel_guc_loader.c +1 -0
+
 	/* A negative value means "use platform default" */
 	if (i915.enable_guc_loading < 0)

i915.enable_guc_loading =
HAS_GUC_UCODE(dev);

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx