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

Submitted by Jani Nikula on Feb. 8, 2019, 12:43 p.m.

Details

Message ID 20190208124332.886-1-jani.nikula@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Jani Nikula Feb. 8, 2019, 12:43 p.m.
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(-)

Patch hide | download patch | download mbox

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);

Comments

Jani Nikula Feb. 8, 2019, 3:09 p.m.
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);
Imre Deak Feb. 8, 2019, 3:11 p.m.
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
Ville Syrjälä Feb. 8, 2019, 3:57 p.m.
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ä Feb. 8, 2019, 4:12 p.m.
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ä Feb. 8, 2019, 4:23 p.m.
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
Jani Nikula Feb. 8, 2019, 5:54 p.m.
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 Feb. 8, 2019, 6:02 p.m.
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 Feb. 8, 2019, 6:18 p.m.
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].