Re: [Intel-gfx] [PATCH 15/31] drm/i915/slpc: Sanitize GuC version

2017-09-28 Thread Sagar Arun Kamble 



On 9/21/2017 6:22 PM, Michal Wajdeczko wrote:
On Tue, 19 Sep 2017 19:41:51 +0200, Sagar Arun Kamble 
 wrote:



From: Tom O'Rourke 

The SLPC interface is dependent on GuC version.
Only GuC versions known to be compatible are supported here.

SLPC with GuC firmware v9 is supported with this series.

v1: Updated with modified sanitize_slpc_option in earlier patch.

v2-v3: Rebase.

v4: Updated support for GuC firmware v9.

v5: Commit subject updated.

v6: Commit subject and message update. Add support condition as >=v9.

v7: Sanitizing GuC version in intel_uc_init_fw for SLPC compatibility.
    Added info. print for needed version and pointer to 01.org.

v8: s/FIRMWARE_URL/I915_FIRMWARE_URL, Macro added for SLPC required GuC
    Major version and rearrangement for sanitization. (MichalW, Joonas)

v9: Checking major_ver_found to sanitize SLPC option enable_slpc post
    fetching the firmware as with Custom firmware loaded through
    guc_firmware_path parameter, major_ver_wanted are cleared. (Lukasz)

v10: Moved the I915_FIRMWARE_URL macro to intel_uc_common.h.

Signed-off-by: Tom O'Rourke 
Signed-off-by: Sagar Arun Kamble 
---
 drivers/gpu/drm/i915/intel_csr.c    | 15 ++-
 drivers/gpu/drm/i915/intel_guc.h    |  1 +
 drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++
 drivers/gpu/drm/i915/intel_uc.c |  1 +
 drivers/gpu/drm/i915/intel_uc_common.h  |  2 ++
 5 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c 
b/drivers/gpu/drm/i915/intel_csr.c

index 965988f..56c56f5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -52,11 +52,6 @@
 MODULE_FIRMWARE(I915_CSR_BXT);
 #define BXT_CSR_VERSION_REQUIRED    CSR_VERSION(1, 7)
-#define FIRMWARE_URL "https://01.org/linuxgraphics/downloads/firmware;
-
-
-
-
 #define CSR_MAX_FW_SIZE    0x2FFF
 #define CSR_DEFAULT_FW_OFFSET    0x
@@ -309,11 +304,12 @@ static uint32_t *parse_csr_fw(struct 
drm_i915_private *dev_priv,

if (csr->version != required_version) {
 DRM_INFO("Refusing to load DMC firmware v%u.%u,"
- " please use v%u.%u [" FIRMWARE_URL "].\n",
+ " please use v%u.%u [%s].\n",
  CSR_VERSION_MAJOR(csr->version),
  CSR_VERSION_MINOR(csr->version),
  CSR_VERSION_MAJOR(required_version),
- CSR_VERSION_MINOR(required_version));
+ CSR_VERSION_MINOR(required_version),
+ I915_FIRMWARE_URL);


Hmm, I'm not sure that including URL here is useful.
URL will be repeated in csr_load_work_fn() where we can explain
its purpose ;)

Sure. Will remove from here.




 return NULL;
 }
@@ -420,8 +416,9 @@ static void csr_load_work_fn(struct work_struct 
*work)

 } else {
 dev_notice(dev_priv->drm.dev,
    "Failed to load DMC firmware"
-   " [" FIRMWARE_URL "],"
-   " disabling runtime power management.\n");
+   " [%s],"
+   " disabling runtime power management.\n",
+   I915_FIRMWARE_URL);


Maybe more user friendly message should looks like:

"Failed to load DMC firmware, disabling runtime power management."
"DMC firmware can be downloaded from 
https://01.org/linuxgraphics/downloads/firmware;

Yes. Will update like this.



 }
release_firmware(fw);
diff --git a/drivers/gpu/drm/i915/intel_guc.h 
b/drivers/gpu/drm/i915/intel_guc.h

index a894991..3821bf2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -159,6 +159,7 @@ static inline void intel_guc_init_early(struct 
intel_guc *guc)

 int intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
 u32 intel_guc_wopcm_size(struct intel_guc *guc);
+void intel_guc_fetch_sanitize_options(struct intel_guc *guc);
/* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
b/drivers/gpu/drm/i915/intel_guc_loader.c

index 6ee7c16..4550620 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -64,6 +64,8 @@
 #define GLK_FW_MAJOR 10
 #define GLK_FW_MINOR 56
+#define I915_SLPC_REQUIRED_GUC_MAJOR 9
+
 #define GUC_FW_PATH(platform, major, minor) \
    "i915/" __stringify(platform) "_guc_ver" __stringify(major) 
"_" __stringify(minor) ".bin"

@@ -418,3 +420,16 @@ int intel_guc_select_fw(struct intel_guc *guc)
return 0;
 }
+
+void intel_guc_fetch_sanitize_options(struct intel_guc *guc)
+{
+    if (guc->fw.major_ver_found <
+    I915_SLPC_REQUIRED_GUC_MAJOR) {


Generally we do not allow Guc fw major "found" to be different than 
"wanted"

so this check could be also done against "wanted".
With Custom firmware loaded through  guc_firmware_path parameter, 
major_ver_wanted is cleared. And

Re: [Intel-gfx] [PATCH 15/31] drm/i915/slpc: Sanitize GuC version

2017-09-21 Thread Michal Wajdeczko 
On Tue, 19 Sep 2017 19:41:51 +0200, Sagar Arun Kamble  
 wrote:



From: Tom O'Rourke 

The SLPC interface is dependent on GuC version.
Only GuC versions known to be compatible are supported here.

SLPC with GuC firmware v9 is supported with this series.

v1: Updated with modified sanitize_slpc_option in earlier patch.

v2-v3: Rebase.

v4: Updated support for GuC firmware v9.

v5: Commit subject updated.

v6: Commit subject and message update. Add support condition as >=v9.

v7: Sanitizing GuC version in intel_uc_init_fw for SLPC compatibility.
Added info. print for needed version and pointer to 01.org.

v8: s/FIRMWARE_URL/I915_FIRMWARE_URL, Macro added for SLPC required GuC
Major version and rearrangement for sanitization. (MichalW, Joonas)

v9: Checking major_ver_found to sanitize SLPC option enable_slpc post
fetching the firmware as with Custom firmware loaded through
guc_firmware_path parameter, major_ver_wanted are cleared. (Lukasz)

v10: Moved the I915_FIRMWARE_URL macro to intel_uc_common.h.

Signed-off-by: Tom O'Rourke 
Signed-off-by: Sagar Arun Kamble 
---
 drivers/gpu/drm/i915/intel_csr.c| 15 ++-
 drivers/gpu/drm/i915/intel_guc.h|  1 +
 drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++
 drivers/gpu/drm/i915/intel_uc.c |  1 +
 drivers/gpu/drm/i915/intel_uc_common.h  |  2 ++
 5 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c  
b/drivers/gpu/drm/i915/intel_csr.c

index 965988f..56c56f5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -52,11 +52,6 @@
 MODULE_FIRMWARE(I915_CSR_BXT);
 #define BXT_CSR_VERSION_REQUIRED   CSR_VERSION(1, 7)
-#define FIRMWARE_URL  "https://01.org/linuxgraphics/downloads/firmware;
-
-
-
-
 #define CSR_MAX_FW_SIZE0x2FFF
 #define CSR_DEFAULT_FW_OFFSET  0x
@@ -309,11 +304,12 @@ static uint32_t *parse_csr_fw(struct  
drm_i915_private *dev_priv,

if (csr->version != required_version) {
DRM_INFO("Refusing to load DMC firmware v%u.%u,"
-" please use v%u.%u [" FIRMWARE_URL "].\n",
+" please use v%u.%u [%s].\n",
 CSR_VERSION_MAJOR(csr->version),
 CSR_VERSION_MINOR(csr->version),
 CSR_VERSION_MAJOR(required_version),
-CSR_VERSION_MINOR(required_version));
+CSR_VERSION_MINOR(required_version),
+I915_FIRMWARE_URL);


Hmm, I'm not sure that including URL here is useful.
URL will be repeated in csr_load_work_fn() where we can explain
its purpose ;)



return NULL;
}
@@ -420,8 +416,9 @@ static void csr_load_work_fn(struct work_struct  
*work)

} else {
dev_notice(dev_priv->drm.dev,
   "Failed to load DMC firmware"
-  " [" FIRMWARE_URL "],"
-  " disabling runtime power management.\n");
+  " [%s],"
+  " disabling runtime power management.\n",
+  I915_FIRMWARE_URL);


Maybe more user friendly message should looks like:

"Failed to load DMC firmware, disabling runtime power management."
"DMC firmware can be downloaded from  
https://01.org/linuxgraphics/downloads/firmware;



}
release_firmware(fw);
diff --git a/drivers/gpu/drm/i915/intel_guc.h  
b/drivers/gpu/drm/i915/intel_guc.h

index a894991..3821bf2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -159,6 +159,7 @@ static inline void intel_guc_init_early(struct  
intel_guc *guc)

 int intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
 u32 intel_guc_wopcm_size(struct intel_guc *guc);
+void intel_guc_fetch_sanitize_options(struct intel_guc *guc);
/* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c  
b/drivers/gpu/drm/i915/intel_guc_loader.c

index 6ee7c16..4550620 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -64,6 +64,8 @@
 #define GLK_FW_MAJOR 10
 #define GLK_FW_MINOR 56
+#define I915_SLPC_REQUIRED_GUC_MAJOR 9
+
 #define GUC_FW_PATH(platform, major, minor) \
"i915/" __stringify(platform) "_guc_ver" __stringify(major) "_"  
__stringify(minor) ".bin"

@@ -418,3 +420,16 @@ int intel_guc_select_fw(struct intel_guc *guc)
return 0;
 }
+
+void intel_guc_fetch_sanitize_options(struct intel_guc *guc)
+{
+   if (guc->fw.major_ver_found <
+   I915_SLPC_REQUIRED_GUC_MAJOR) {


Generally we do not allow Guc fw major "found" to be different than  
"wanted"

so this check could be

[Intel-gfx] [PATCH 15/31] drm/i915/slpc: Sanitize GuC version

2017-09-19 Thread Sagar Arun Kamble 
From: Tom O'Rourke 

The SLPC interface is dependent on GuC version.
Only GuC versions known to be compatible are supported here.

SLPC with GuC firmware v9 is supported with this series.

v1: Updated with modified sanitize_slpc_option in earlier patch.

v2-v3: Rebase.

v4: Updated support for GuC firmware v9.

v5: Commit subject updated.

v6: Commit subject and message update. Add support condition as >=v9.

v7: Sanitizing GuC version in intel_uc_init_fw for SLPC compatibility.
Added info. print for needed version and pointer to 01.org.

v8: s/FIRMWARE_URL/I915_FIRMWARE_URL, Macro added for SLPC required GuC
Major version and rearrangement for sanitization. (MichalW, Joonas)

v9: Checking major_ver_found to sanitize SLPC option enable_slpc post
fetching the firmware as with Custom firmware loaded through
guc_firmware_path parameter, major_ver_wanted are cleared. (Lukasz)

v10: Moved the I915_FIRMWARE_URL macro to intel_uc_common.h.

Signed-off-by: Tom O'Rourke 
Signed-off-by: Sagar Arun Kamble 
---
 drivers/gpu/drm/i915/intel_csr.c| 15 ++-
 drivers/gpu/drm/i915/intel_guc.h|  1 +
 drivers/gpu/drm/i915/intel_guc_loader.c | 15 +++
 drivers/gpu/drm/i915/intel_uc.c |  1 +
 drivers/gpu/drm/i915/intel_uc_common.h  |  2 ++
 5 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 965988f..56c56f5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -52,11 +52,6 @@
 MODULE_FIRMWARE(I915_CSR_BXT);
 #define BXT_CSR_VERSION_REQUIRED   CSR_VERSION(1, 7)
 
-#define FIRMWARE_URL  "https://01.org/linuxgraphics/downloads/firmware;
-
-
-
-
 #define CSR_MAX_FW_SIZE0x2FFF
 #define CSR_DEFAULT_FW_OFFSET  0x
 
@@ -309,11 +304,12 @@ static uint32_t *parse_csr_fw(struct drm_i915_private 
*dev_priv,
 
if (csr->version != required_version) {
DRM_INFO("Refusing to load DMC firmware v%u.%u,"
-" please use v%u.%u [" FIRMWARE_URL "].\n",
+" please use v%u.%u [%s].\n",
 CSR_VERSION_MAJOR(csr->version),
 CSR_VERSION_MINOR(csr->version),
 CSR_VERSION_MAJOR(required_version),
-CSR_VERSION_MINOR(required_version));
+CSR_VERSION_MINOR(required_version),
+I915_FIRMWARE_URL);
return NULL;
}
 
@@ -420,8 +416,9 @@ static void csr_load_work_fn(struct work_struct *work)
} else {
dev_notice(dev_priv->drm.dev,
   "Failed to load DMC firmware"
-  " [" FIRMWARE_URL "],"
-  " disabling runtime power management.\n");
+  " [%s],"
+  " disabling runtime power management.\n",
+  I915_FIRMWARE_URL);
}
 
release_firmware(fw);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index a894991..3821bf2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -159,6 +159,7 @@ static inline void intel_guc_init_early(struct intel_guc 
*guc)
 int intel_guc_select_fw(struct intel_guc *guc);
 int intel_guc_init_hw(struct intel_guc *guc);
 u32 intel_guc_wopcm_size(struct intel_guc *guc);
+void intel_guc_fetch_sanitize_options(struct intel_guc *guc);
 
 /* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
b/drivers/gpu/drm/i915/intel_guc_loader.c
index 6ee7c16..4550620 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -64,6 +64,8 @@
 #define GLK_FW_MAJOR 10
 #define GLK_FW_MINOR 56
 
+#define I915_SLPC_REQUIRED_GUC_MAJOR 9
+
 #define GUC_FW_PATH(platform, major, minor) \
"i915/" __stringify(platform) "_guc_ver" __stringify(major) "_" 
__stringify(minor) ".bin"
 
@@ -418,3 +420,16 @@ int intel_guc_select_fw(struct intel_guc *guc)
 
return 0;
 }
+
+void intel_guc_fetch_sanitize_options(struct intel_guc *guc)
+{
+   if (guc->fw.major_ver_found <
+   I915_SLPC_REQUIRED_GUC_MAJOR) {
+   DRM_INFO("SLPC not supported with GuC firmware"
+" v%u, please use v%u+ [%s].\n",
+guc->fw.major_ver_found,
+I915_SLPC_REQUIRED_GUC_MAJOR,
+I915_FIRMWARE_URL);
+   i915.enable_slpc = 0;
+   }
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index eeec986..350027f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -194,6 +194,7 @@ static void