[PATCH 1/3] drm/i915/opregion: fix version check

[PATCH 1/3] drm/i915/opregion: fix version check

[Jani Nikula]

The u32 version field encodes major version in the high word. We've been
checking for version >= 0.2.

Add opregion version logging while at it.

Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 30ae96c5c97c..7e4152d97c45 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -46,6 +46,9 @@
 #define OPREGION_ASLE_EXT_OFFSET	0x1C00
 
 #define OPREGION_SIGNATURE "IntelGraphicsMem"
+
+#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
+
 #define MBOX_ACPI      (1<<0)
 #define MBOX_SWSCI     (1<<1)
 #define MBOX_ASLE      (1<<2)
@@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	opregion->header = base;
 	opregion->lid_state = base + ACPI_CLID;
 
+	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
+			 opregion->header->opregion_ver >> 16,
+			 opregion->header->opregion_ver & 0xffff);
+
 	mboxes = opregion->header->mboxes;
 	if (mboxes & MBOX_ACPI) {
 		DRM_DEBUG_DRIVER("Public ACPI methods supported\n");
@@ -952,8 +959,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	if (dmi_check_system(intel_no_opregion_vbt))
 		goto out;
 
-	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
-	    opregion->asle->rvda && opregion->asle->rvds) {
+	if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 0) &&
+	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
 		opregion->rvda = memremap(opregion->asle->rvda,
 					  opregion->asle->rvds,
 					  MEMREMAP_WB);
-- 
2.20.1

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

[PATCH 2/3] drm/i915/opregion: rvda is relative from opregion base in opregion 2.1+

[Jani Nikula]

Starting from opregion version 2.1 (roughly corresponding to ICL+) the
RVDA field is relative from the beginning of opregion, not absolute
address.

Fix the error path while at it.

v2: Make relative vs. absolute conditional on the opregion version,
    bumped for the purpose. Turned out there are machines relying on
    absolute RVDA in the wild.

Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 7e4152d97c45..c7d523da6631 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -121,7 +121,8 @@ struct opregion_asle {
 	u64 fdss;
 	u32 fdsp;
 	u32 stat;
-	u64 rvda;	/* Physical address of raw vbt data */
+	u64 rvda;	/* Physical (2.0) or relative from opregion (2.1+)
+			 * address of raw VBT data. */
 	u32 rvds;	/* Size of raw vbt data */
 	u8 rsvd[58];
 } __packed;
@@ -961,9 +962,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 
 	if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 0) &&
 	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
-		opregion->rvda = memremap(opregion->asle->rvda,
-					  opregion->asle->rvds,
+
+		resource_size_t rvda = opregion->asle->rvda;
+
+		/*
+		 * opregion 2.0: rvda is the physical VBT address.
+		 *
+		 * opregion 2.1+: rvda is unsigned, relative offset from
+		 * opregion base, and should never point within opregion.
+		 */
+		if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 1)) {
+			WARN_ON(rvda < OPREGION_SIZE);
+
+			rvda += asls;
+		}
+
+		opregion->rvda = memremap(rvda, opregion->asle->rvds,
 					  MEMREMAP_WB);
+
 		vbt = opregion->rvda;
 		vbt_size = opregion->asle->rvds;
 		if (intel_bios_is_valid_vbt(vbt, vbt_size)) {
@@ -973,6 +989,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 			goto out;
 		} else {
 			DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n");
+			memunmap(opregion->rvda);
+			opregion->rvda = NULL;
 		}
 	}
 
-- 
2.20.1

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

[PATCH 3/3] HACK: drm/i915/opregion: ICL should have opregion 2.1+ and relative rvda

[Jani Nikula]

Temporary CI workaround before proper opregion version bump propagates
to the machines.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index c7d523da6631..e90be277ac21 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -971,7 +971,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 		 * opregion 2.1+: rvda is unsigned, relative offset from
 		 * opregion base, and should never point within opregion.
 		 */
-		if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 1)) {
+		if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 1) ||
+		    IS_ICELAKE(dev_priv)) {
 			WARN_ON(rvda < OPREGION_SIZE);
 
 			rvda += asls;
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/opregion: fix version check

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/opregion: fix version check
URL   : https://patchwork.freedesktop.org/series/56395/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d6761937ae7b drm/i915/opregion: fix version check
7b8f0501093b drm/i915/opregion: rvda is relative from opregion base in opregion 2.1+
-:35: WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#35: FILE: drivers/gpu/drm/i915/intel_opregion.c:125:
+			 * address of raw VBT data. */

-:45: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
#45: FILE: drivers/gpu/drm/i915/intel_opregion.c:965:
 	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
+

total: 0 errors, 1 warnings, 1 checks, 43 lines checked
b611b8f9a275 HACK: drm/i915/opregion: ICL should have opregion 2.1+ and relative rvda

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

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/opregion: fix version check

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/opregion: fix version check
URL   : https://patchwork.freedesktop.org/series/56395/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5567 -> Patchwork_12175
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12175 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12175, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56395/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12175:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-apl-guc:         PASS -> DMESG-WARN

  
Known issues
------------

  Here are the changes found in Patchwork_12175 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_busy@basic-flip-a:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182] +1

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       PASS -> WARN [fdo#109380]

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         PASS -> FAIL [fdo#104008]

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109380]: https://bugs.freedesktop.org/show_bug.cgi?id=109380


Participating hosts (49 -> 44)
------------------------------

  Missing    (5): fi-kbl-soraka fi-hsw-4770r fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


Build changes
-------------

    * Linux: CI_DRM_5567 -> Patchwork_12175

  CI_DRM_5567: 5725867972123d46f4a4d382ed5fa2eb0348b00c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4815: 947301563259726b65ce47d3a3fe37931ed42efe @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12175: b611b8f9a27553340cc1e7d736be3ba5fc9a19d1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b611b8f9a275 HACK: drm/i915/opregion: ICL should have opregion 2.1+ and relative rvda
7b8f0501093b drm/i915/opregion: rvda is relative from opregion base in opregion 2.1+
d6761937ae7b drm/i915/opregion: fix version check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12175/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Jani Nikula]

On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> The u32 version field encodes major version in the high word. We've been
> checking for version >= 0.2.
>
> Add opregion version logging while at it.
>
> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 30ae96c5c97c..7e4152d97c45 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -46,6 +46,9 @@
>  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
>  
>  #define OPREGION_SIGNATURE "IntelGraphicsMem"
> +
> +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
> +
>  #define MBOX_ACPI      (1<<0)
>  #define MBOX_SWSCI     (1<<1)
>  #define MBOX_ASLE      (1<<2)
> @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	opregion->header = base;
>  	opregion->lid_state = base + ACPI_CLID;
>  
> +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
> +			 opregion->header->opregion_ver >> 16,
> +			 opregion->header->opregion_ver & 0xffff);
> +

This is ridiculous and maddening. On our CI APL this prints [1]:

<7>[    7.029368] [drm:intel_opregion_setup [i915]] ACPI OpRegion version 512.0

Yes, version 512.0.

Digging into it, I found one random version of the opregion spec that
has:

	Bits [31:16] - Major Version Number
	Bits [23:0] - Minor Version Number

The minor is supposed to be [15:0]. But this APL (maybe others, need to
investigate) has the major version encoded to bits 31:24.

This obviously screws up the opregion version check in patch 2 as well,
leading to relative RVDA being used.

Absolutely disgusting.


BR,
Jani.


[1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12175/fi-apl-guc/boot0.log


>  	mboxes = opregion->header->mboxes;
>  	if (mboxes & MBOX_ACPI) {
>  		DRM_DEBUG_DRIVER("Public ACPI methods supported\n");
> @@ -952,8 +959,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	if (dmi_check_system(intel_no_opregion_vbt))
>  		goto out;
>  
> -	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
> -	    opregion->asle->rvda && opregion->asle->rvds) {
> +	if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 0) &&
> +	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
>  		opregion->rvda = memremap(opregion->asle->rvda,
>  					  opregion->asle->rvds,
>  					  MEMREMAP_WB);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Imre Deak]

On Fri, Feb 08, 2019 at 05:09:51PM +0200, Jani Nikula wrote:
> On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> > The u32 version field encodes major version in the high word. We've been
> > checking for version >= 0.2.
> >
> > Add opregion version logging while at it.
> >
> > Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > index 30ae96c5c97c..7e4152d97c45 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > @@ -46,6 +46,9 @@
> >  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
> >  
> >  #define OPREGION_SIGNATURE "IntelGraphicsMem"
> > +
> > +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
> > +
> >  #define MBOX_ACPI      (1<<0)
> >  #define MBOX_SWSCI     (1<<1)
> >  #define MBOX_ASLE      (1<<2)
> > @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  	opregion->header = base;
> >  	opregion->lid_state = base + ACPI_CLID;
> >  
> > +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
> > +			 opregion->header->opregion_ver >> 16,
> > +			 opregion->header->opregion_ver & 0xffff);
> > +
> 
> This is ridiculous and maddening. On our CI APL this prints [1]:
> 
> <7>[    7.029368] [drm:intel_opregion_setup [i915]] ACPI OpRegion version 512.0
> 
> Yes, version 512.0.
> 
> Digging into it, I found one random version of the opregion spec that
> has:
> 
> 	Bits [31:16] - Major Version Number
> 	Bits [23:0] - Minor Version Number
> 
> The minor is supposed to be [15:0]. But this APL (maybe others, need to
> investigate) has the major version encoded to bits 31:24.
> 
> This obviously screws up the opregion version check in patch 2 as well,
> leading to relative RVDA being used.

Maybe you should use another version field to decode this version field.
Just kidding:)

> 
> Absolutely disgusting.
> 
> 
> BR,
> Jani.
> 
> 
> [1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12175/fi-apl-guc/boot0.log
> 
> 
> >  	mboxes = opregion->header->mboxes;
> >  	if (mboxes & MBOX_ACPI) {
> >  		DRM_DEBUG_DRIVER("Public ACPI methods supported\n");
> > @@ -952,8 +959,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  	if (dmi_check_system(intel_no_opregion_vbt))
> >  		goto out;
> >  
> > -	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
> > -	    opregion->asle->rvda && opregion->asle->rvds) {
> > +	if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 0) &&
> > +	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
> >  		opregion->rvda = memremap(opregion->asle->rvda,
> >  					  opregion->asle->rvds,
> >  					  MEMREMAP_WB);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Ville Syrjälä]

On Fri, Feb 08, 2019 at 05:09:51PM +0200, Jani Nikula wrote:
> On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> > The u32 version field encodes major version in the high word. We've been
> > checking for version >= 0.2.
> >
> > Add opregion version logging while at it.
> >
> > Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > index 30ae96c5c97c..7e4152d97c45 100644
> > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > @@ -46,6 +46,9 @@
> >  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
> >  
> >  #define OPREGION_SIGNATURE "IntelGraphicsMem"
> > +
> > +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
> > +
> >  #define MBOX_ACPI      (1<<0)
> >  #define MBOX_SWSCI     (1<<1)
> >  #define MBOX_ASLE      (1<<2)
> > @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> >  	opregion->header = base;
> >  	opregion->lid_state = base + ACPI_CLID;
> >  
> > +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
> > +			 opregion->header->opregion_ver >> 16,
> > +			 opregion->header->opregion_ver & 0xffff);
> > +
> 
> This is ridiculous and maddening. On our CI APL this prints [1]:
> 
> <7>[    7.029368] [drm:intel_opregion_setup [i915]] ACPI OpRegion version 512.0
> 
> Yes, version 512.0.
> 
> Digging into it, I found one random version of the opregion spec that
> has:
> 
> 	Bits [31:16] - Major Version Number
> 	Bits [23:0] - Minor Version Number
> 
> The minor is supposed to be [15:0]. But this APL (maybe others, need to
> investigate) has the major version encoded to bits 31:24.

Most copies of the spec I have seem to have the 24 + 16 bits mess.

They also claim that HSW+ should generally have version 3.0. So
not sure this 2.0 vs. 2.1 business even makes sense.

We should probably grab the opregion from all the machines we have
around and see what they actully look like.

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

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Ville Syrjälä]

On Fri, Feb 08, 2019 at 05:57:53PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 08, 2019 at 05:09:51PM +0200, Jani Nikula wrote:
> > On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> > > The u32 version field encodes major version in the high word. We've been
> > > checking for version >= 0.2.
> > >
> > > Add opregion version logging while at it.
> > >
> > > Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> > > index 30ae96c5c97c..7e4152d97c45 100644
> > > --- a/drivers/gpu/drm/i915/intel_opregion.c
> > > +++ b/drivers/gpu/drm/i915/intel_opregion.c
> > > @@ -46,6 +46,9 @@
> > >  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
> > >  
> > >  #define OPREGION_SIGNATURE "IntelGraphicsMem"
> > > +
> > > +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
> > > +
> > >  #define MBOX_ACPI      (1<<0)
> > >  #define MBOX_SWSCI     (1<<1)
> > >  #define MBOX_ASLE      (1<<2)
> > > @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> > >  	opregion->header = base;
> > >  	opregion->lid_state = base + ACPI_CLID;
> > >  
> > > +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
> > > +			 opregion->header->opregion_ver >> 16,
> > > +			 opregion->header->opregion_ver & 0xffff);
> > > +
> > 
> > This is ridiculous and maddening. On our CI APL this prints [1]:
> > 
> > <7>[    7.029368] [drm:intel_opregion_setup [i915]] ACPI OpRegion version 512.0
> > 
> > Yes, version 512.0.
> > 
> > Digging into it, I found one random version of the opregion spec that
> > has:
> > 
> > 	Bits [31:16] - Major Version Number
> > 	Bits [23:0] - Minor Version Number
> > 
> > The minor is supposed to be [15:0]. But this APL (maybe others, need to
> > investigate) has the major version encoded to bits 31:24.
> 
> Most copies of the spec I have seem to have the 24 + 16 bits mess.
> 
> They also claim that HSW+ should generally have version 3.0. So
> not sure this 2.0 vs. 2.1 business even makes sense.
> 
> We should probably grab the opregion from all the machines we have
> around and see what they actully look like.

Here's the output from my random opregion collection:
bsw_rvp_BRAS.opregion
	over:	0x02000000
bw_ASUS_P5E-VM_HDMI.opregion
	over:	0x01010000
cl_hp_compaq_6910p.opregion
	over:	0x01010000
ctg_dell_latitude_e5400.opregion
	over:	0x02000000
dell_xps_13_9350.opregion
	over:	0x02000000
elk.opregion
	over:	0x02000000
hsw_brix_pro_GIGABYTE_M4HM87P.opregion
	over:	0x02000000
hsw_Gigabyte_Z97x-UD5H.opregion
	over:	0x02000000
hsw_shark_bay_HSWLPTU1.opregion
	over:	0x02000000
ilk_dell_latiture_e5410.opregion
	over:	0x02000000
ivb_BHZ7710H.opregion
	over:	0x02000000
ivb_Lenovo_ThinkPad_X1_Carbon.opregion
	over:	0x02000000
skl_MSI_MS-7971.opregion
	over:	0x02000000
snb_Dell_XPS_8300.opregion
	over:	0x02000000
vlv_ffrd8_BLAKFF81.ppregion

So looks like all of them have the major in [31:24].

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

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Jani Nikula]

On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 08, 2019 at 05:57:53PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 08, 2019 at 05:09:51PM +0200, Jani Nikula wrote:
>> > On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>> > > The u32 version field encodes major version in the high word. We've been
>> > > checking for version >= 0.2.
>> > >
>> > > Add opregion version logging while at it.
>> > >
>> > > Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
>> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > Cc: Imre Deak <imre.deak@intel.com>
>> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
>> > >  1 file changed, 9 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> > > index 30ae96c5c97c..7e4152d97c45 100644
>> > > --- a/drivers/gpu/drm/i915/intel_opregion.c
>> > > +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> > > @@ -46,6 +46,9 @@
>> > >  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
>> > >  
>> > >  #define OPREGION_SIGNATURE "IntelGraphicsMem"
>> > > +
>> > > +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
>> > > +
>> > >  #define MBOX_ACPI      (1<<0)
>> > >  #define MBOX_SWSCI     (1<<1)
>> > >  #define MBOX_ASLE      (1<<2)
>> > > @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>> > >  	opregion->header = base;
>> > >  	opregion->lid_state = base + ACPI_CLID;
>> > >  
>> > > +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
>> > > +			 opregion->header->opregion_ver >> 16,
>> > > +			 opregion->header->opregion_ver & 0xffff);
>> > > +
>> > 
>> > This is ridiculous and maddening. On our CI APL this prints [1]:
>> > 
>> > <7>[    7.029368] [drm:intel_opregion_setup [i915]] ACPI OpRegion version 512.0
>> > 
>> > Yes, version 512.0.
>> > 
>> > Digging into it, I found one random version of the opregion spec that
>> > has:
>> > 
>> > 	Bits [31:16] - Major Version Number
>> > 	Bits [23:0] - Minor Version Number
>> > 
>> > The minor is supposed to be [15:0]. But this APL (maybe others, need to
>> > investigate) has the major version encoded to bits 31:24.
>> 
>> Most copies of the spec I have seem to have the 24 + 16 bits mess.
>> 
>> They also claim that HSW+ should generally have version 3.0. So
>> not sure this 2.0 vs. 2.1 business even makes sense.
>> 
>> We should probably grab the opregion from all the machines we have
>> around and see what they actully look like.
>
> Here's the output from my random opregion collection:
> bsw_rvp_BRAS.opregion
> 	over:	0x02000000
> bw_ASUS_P5E-VM_HDMI.opregion
> 	over:	0x01010000
> cl_hp_compaq_6910p.opregion
> 	over:	0x01010000

So judging by the multitude of specs and interpretations, these two
could be a lot of things.

v101.0		16+16 BCD
v257.0		16+16
v1.10000	8+24 BCD
v1.65536	8+24

However it feels more likely all of these are really 8 bits major + 8
bits minor (BCD or not) + 16 bits reserved/zero, i.e.

v1.1

But that interpretation isn't supported by *any* of the specs I have.

BR,
Jani.


> ctg_dell_latitude_e5400.opregion
> 	over:	0x02000000
> dell_xps_13_9350.opregion
> 	over:	0x02000000
> elk.opregion
> 	over:	0x02000000
> hsw_brix_pro_GIGABYTE_M4HM87P.opregion
> 	over:	0x02000000
> hsw_Gigabyte_Z97x-UD5H.opregion
> 	over:	0x02000000
> hsw_shark_bay_HSWLPTU1.opregion
> 	over:	0x02000000
> ilk_dell_latiture_e5410.opregion
> 	over:	0x02000000
> ivb_BHZ7710H.opregion
> 	over:	0x02000000
> ivb_Lenovo_ThinkPad_X1_Carbon.opregion
> 	over:	0x02000000
> skl_MSI_MS-7971.opregion
> 	over:	0x02000000
> snb_Dell_XPS_8300.opregion
> 	over:	0x02000000
> vlv_ffrd8_BLAKFF81.ppregion
>
> So looks like all of them have the major in [31:24].

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Jani Nikula]

On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Feb 08, 2019 at 05:57:53PM +0200, Ville Syrjälä wrote:
>>> On Fri, Feb 08, 2019 at 05:09:51PM +0200, Jani Nikula wrote:
>>> > On Fri, 08 Feb 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>>> > > The u32 version field encodes major version in the high word. We've been
>>> > > checking for version >= 0.2.
>>> > >
>>> > > Add opregion version logging while at it.
>>> > >
>>> > > Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
>>> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> > > Cc: Imre Deak <imre.deak@intel.com>
>>> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> > > ---
>>> > >  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
>>> > >  1 file changed, 9 insertions(+), 2 deletions(-)
>>> > >
>>> > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>>> > > index 30ae96c5c97c..7e4152d97c45 100644
>>> > > --- a/drivers/gpu/drm/i915/intel_opregion.c
>>> > > +++ b/drivers/gpu/drm/i915/intel_opregion.c
>>> > > @@ -46,6 +46,9 @@
>>> > >  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
>>> > >  
>>> > >  #define OPREGION_SIGNATURE "IntelGraphicsMem"
>>> > > +
>>> > > +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
>>> > > +
>>> > >  #define MBOX_ACPI      (1<<0)
>>> > >  #define MBOX_SWSCI     (1<<1)
>>> > >  #define MBOX_ASLE      (1<<2)
>>> > > @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>>> > >  	opregion->header = base;
>>> > >  	opregion->lid_state = base + ACPI_CLID;
>>> > >  
>>> > > +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
>>> > > +			 opregion->header->opregion_ver >> 16,
>>> > > +			 opregion->header->opregion_ver & 0xffff);
>>> > > +
>>> > 
>>> > This is ridiculous and maddening. On our CI APL this prints [1]:
>>> > 
>>> > <7>[    7.029368] [drm:intel_opregion_setup [i915]] ACPI OpRegion version 512.0
>>> > 
>>> > Yes, version 512.0.
>>> > 
>>> > Digging into it, I found one random version of the opregion spec that
>>> > has:
>>> > 
>>> > 	Bits [31:16] - Major Version Number
>>> > 	Bits [23:0] - Minor Version Number
>>> > 
>>> > The minor is supposed to be [15:0]. But this APL (maybe others, need to
>>> > investigate) has the major version encoded to bits 31:24.
>>> 
>>> Most copies of the spec I have seem to have the 24 + 16 bits mess.
>>> 
>>> They also claim that HSW+ should generally have version 3.0. So
>>> not sure this 2.0 vs. 2.1 business even makes sense.
>>> 
>>> We should probably grab the opregion from all the machines we have
>>> around and see what they actully look like.
>>
>> Here's the output from my random opregion collection:
>> bsw_rvp_BRAS.opregion
>> 	over:	0x02000000
>> bw_ASUS_P5E-VM_HDMI.opregion
>> 	over:	0x01010000
>> cl_hp_compaq_6910p.opregion
>> 	over:	0x01010000
>
> So judging by the multitude of specs and interpretations, these two
> could be a lot of things.
>
> v101.0		16+16 BCD
> v257.0		16+16
> v1.10000	8+24 BCD
> v1.65536	8+24
>
> However it feels more likely all of these are really 8 bits major + 8
> bits minor (BCD or not) + 16 bits reserved/zero, i.e.
>
> v1.1
>
> But that interpretation isn't supported by *any* of the specs I have.

Turns out that's what all the other software thinks, undocumented.

31:24 major
23:16 minor
15:8  revision
7:0   reserved

No idea about BCD...

BR,
Jani.


>
> BR,
> Jani.
>
>
>> ctg_dell_latitude_e5400.opregion
>> 	over:	0x02000000
>> dell_xps_13_9350.opregion
>> 	over:	0x02000000
>> elk.opregion
>> 	over:	0x02000000
>> hsw_brix_pro_GIGABYTE_M4HM87P.opregion
>> 	over:	0x02000000
>> hsw_Gigabyte_Z97x-UD5H.opregion
>> 	over:	0x02000000
>> hsw_shark_bay_HSWLPTU1.opregion
>> 	over:	0x02000000
>> ilk_dell_latiture_e5410.opregion
>> 	over:	0x02000000
>> ivb_BHZ7710H.opregion
>> 	over:	0x02000000
>> ivb_Lenovo_ThinkPad_X1_Carbon.opregion
>> 	over:	0x02000000
>> skl_MSI_MS-7971.opregion
>> 	over:	0x02000000
>> snb_Dell_XPS_8300.opregion
>> 	over:	0x02000000
>> vlv_ffrd8_BLAKFF81.ppregion
>>
>> So looks like all of them have the major in [31:24].

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Ville Syrjälä]

On Fri, Feb 08, 2019 at 02:43:30PM +0200, Jani Nikula wrote:
> The u32 version field encodes major version in the high word. We've been
> checking for version >= 0.2.
> 
> Add opregion version logging while at it.
> 
> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 30ae96c5c97c..7e4152d97c45 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -46,6 +46,9 @@
>  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
>  
>  #define OPREGION_SIGNATURE "IntelGraphicsMem"
> +
> +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
> +
>  #define MBOX_ACPI      (1<<0)
>  #define MBOX_SWSCI     (1<<1)
>  #define MBOX_ASLE      (1<<2)
> @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	opregion->header = base;
>  	opregion->lid_state = base + ACPI_CLID;
>  
> +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
> +			 opregion->header->opregion_ver >> 16,
> +			 opregion->header->opregion_ver & 0xffff);

BTW the spec says this is 4bit bcd. So this printk isn't quite correct.

> +
>  	mboxes = opregion->header->mboxes;
>  	if (mboxes & MBOX_ACPI) {
>  		DRM_DEBUG_DRIVER("Public ACPI methods supported\n");
> @@ -952,8 +959,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  	if (dmi_check_system(intel_no_opregion_vbt))
>  		goto out;
>  
> -	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
> -	    opregion->asle->rvda && opregion->asle->rvds) {
> +	if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 0) &&
> +	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
>  		opregion->rvda = memremap(opregion->asle->rvda,
>  					  opregion->asle->rvds,
>  					  MEMREMAP_WB);
> -- 
> 2.20.1

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

Re: [PATCH 1/3] drm/i915/opregion: fix version check

[Jani Nikula]

On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 08, 2019 at 02:43:30PM +0200, Jani Nikula wrote:
>> The u32 version field encodes major version in the high word. We've been
>> checking for version >= 0.2.
>> 
>> Add opregion version logging while at it.
>> 
>> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB")
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_opregion.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 30ae96c5c97c..7e4152d97c45 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -46,6 +46,9 @@
>>  #define OPREGION_ASLE_EXT_OFFSET	0x1C00
>>  
>>  #define OPREGION_SIGNATURE "IntelGraphicsMem"
>> +
>> +#define OPREGION_VERSION(major, minor) (((major) << 16) | (minor))
>> +
>>  #define MBOX_ACPI      (1<<0)
>>  #define MBOX_SWSCI     (1<<1)
>>  #define MBOX_ASLE      (1<<2)
>> @@ -924,6 +927,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>>  	opregion->header = base;
>>  	opregion->lid_state = base + ACPI_CLID;
>>  
>> +	DRM_DEBUG_DRIVER("ACPI OpRegion version %u.%u\n",
>> +			 opregion->header->opregion_ver >> 16,
>> +			 opregion->header->opregion_ver & 0xffff);
>
> BTW the spec says this is 4bit bcd. So this printk isn't quite correct.

Uh, another version of the spec even gives an example:

[31:16] [15:0] version
   A       F   10.15

Doesn't look like BCD to me. It would have to be 0x10 and 0x15 in the
example then. :(

You can't make this stuff up.

BR,
Jani.


>
>> +
>>  	mboxes = opregion->header->mboxes;
>>  	if (mboxes & MBOX_ACPI) {
>>  		DRM_DEBUG_DRIVER("Public ACPI methods supported\n");
>> @@ -952,8 +959,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>>  	if (dmi_check_system(intel_no_opregion_vbt))
>>  		goto out;
>>  
>> -	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
>> -	    opregion->asle->rvda && opregion->asle->rvds) {
>> +	if (opregion->header->opregion_ver >= OPREGION_VERSION(2, 0) &&
>> +	    opregion->asle && opregion->asle->rvda && opregion->asle->rvds) {
>>  		opregion->rvda = memremap(opregion->asle->rvda,
>>  					  opregion->asle->rvds,
>>  					  MEMREMAP_WB);
>> -- 
>> 2.20.1

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx