[PATCH] drm/i915/opregion: rvda is relative from opregion base, not absolute

[PATCH] drm/i915/opregion: rvda is relative from opregion base, not absolute

[Jani Nikula]

We've supported the opregion RVDA/RVDS fields for VBT size >= 6 KB since
commit 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6
KB"). That's three years, almost to the date.

The implementation was based on spec only, in anticipation of systems
with big VBT. Now, the spec has been changed. The RVDA is supposed to be
relative from the beginning of opregion, not absolute address.

This is obviously a backward/forward incompatible change. I've been told
there are no systems out there using the field. Fingers crossed. This
will still be problematic for older kernels, and we can only try to
backport the fix.

Fix the error path 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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 30ae96c5c97c..30324b963e24 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -118,7 +118,7 @@ struct opregion_asle {
 	u64 fdss;
 	u32 fdsp;
 	u32 stat;
-	u64 rvda;	/* Physical address of raw vbt data */
+	u64 rvda;	/* Address of raw vbt data, relative from opregion */
 	u32 rvds;	/* Size of raw vbt data */
 	u8 rsvd[58];
 } __packed;
@@ -954,7 +954,13 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 
 	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
 	    opregion->asle->rvda && opregion->asle->rvds) {
-		opregion->rvda = memremap(opregion->asle->rvda,
+		/*
+		 * rvda is unsigned, relative from opregion base, and should
+		 * never point within opregion.
+		 */
+		WARN_ON(opregion->asle->rvda < OPREGION_SIZE);
+
+		opregion->rvda = memremap(asls + opregion->asle->rvda,
 					  opregion->asle->rvds,
 					  MEMREMAP_WB);
 		vbt = opregion->rvda;
@@ -966,6 +972,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

✗ Fi.CI.BAT: failure for drm/i915/opregion: rvda is relative from opregion base, not absolute (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/opregion: rvda is relative from opregion base, not absolute (rev2)
URL   : https://patchwork.freedesktop.org/series/53996/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5500 -> Patchwork_12069
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12069 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12069, 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/53996/revisions/2/mbox/

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

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

### 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_12069 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#103558] / [fdo#105602]

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#105602]

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

  
#### Possible fixes ####

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

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         FAIL [fdo#103182] -> 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#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108915]: https://bugs.freedesktop.org/show_bug.cgi?id=108915
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (46 -> 41)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


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

    * Linux: CI_DRM_5500 -> Patchwork_12069

  CI_DRM_5500: a76602bc7e9c1661ac1328103ef67dbc342a7655 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4798: 998e0a4aedf10fb5f7c271018cd80d874668bf55 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12069: 85213487ce8d4098058bc99a745c649bb965625b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

85213487ce8d drm/i915/opregion: rvda is relative from opregion base, not absolute

== Logs ==

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

Re: [PATCH] drm/i915/opregion: rvda is relative from opregion base, not absolute

[Ville Syrjälä]

On Tue, Jan 29, 2019 at 03:31:21PM +0200, Jani Nikula wrote:
> We've supported the opregion RVDA/RVDS fields for VBT size >= 6 KB since
> commit 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6
> KB"). That's three years, almost to the date.
> 
> The implementation was based on spec only, in anticipation of systems
> with big VBT. Now, the spec has been changed. The RVDA is supposed to be
> relative from the beginning of opregion, not absolute address.
> 
> This is obviously a backward/forward incompatible change. I've been told
> there are no systems out there using the field. Fingers crossed. This
> will still be problematic for older kernels, and we can only try to
> backport the fix.
> 
> Fix the error path 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>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_opregion.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 30ae96c5c97c..30324b963e24 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -118,7 +118,7 @@ struct opregion_asle {
>  	u64 fdss;
>  	u32 fdsp;
>  	u32 stat;
> -	u64 rvda;	/* Physical address of raw vbt data */
> +	u64 rvda;	/* Address of raw vbt data, relative from opregion */
>  	u32 rvds;	/* Size of raw vbt data */
>  	u8 rsvd[58];
>  } __packed;
> @@ -954,7 +954,13 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>  
>  	if (opregion->header->opregion_ver >= 2 && opregion->asle &&
>  	    opregion->asle->rvda && opregion->asle->rvds) {
> -		opregion->rvda = memremap(opregion->asle->rvda,
> +		/*
> +		 * rvda is unsigned, relative from opregion base, and should
> +		 * never point within opregion.
> +		 */
> +		WARN_ON(opregion->asle->rvda < OPREGION_SIZE);
> +
> +		opregion->rvda = memremap(asls + opregion->asle->rvda,
>  					  opregion->asle->rvds,
>  					  MEMREMAP_WB);
>  		vbt = opregion->rvda;
> @@ -966,6 +972,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

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

✗ Fi.CI.BAT: failure for drm/i915/opregion: rvda is relative from opregion base, not absolute (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/opregion: rvda is relative from opregion base, not absolute (rev3)
URL   : https://patchwork.freedesktop.org/series/53996/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5518 -> Patchwork_12104
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12104 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12104, 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/53996/revisions/3/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

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

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@runner@aborted}:
    - fi-bxt-dsi:         NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

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

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       INCOMPLETE [fdo#108602] / [fdo#108744] -> PASS

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       {SKIP} [fdo#109271] / [fdo#109278] -> PASS +2

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

  * igt@pm_rpm@module-reload:
    - fi-skl-6770hq:      FAIL [fdo#108511] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         DMESG-FAIL [fdo#103182] -> 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#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108915]: https://bugs.freedesktop.org/show_bug.cgi?id=108915
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (43 -> 45)
------------------------------

  Additional (6): fi-bxt-dsi fi-ivb-3770 fi-icl-y fi-bsw-kefka fi-skl-6600u fi-snb-2600 
  Missing    (4): fi-byt-j1900 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


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

    * Linux: CI_DRM_5518 -> Patchwork_12104

  CI_DRM_5518: 2369fd28d3a46b865f6d4f1d309a4c6b7b4e6d93 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4801: 6f6bacf12759fb319ade3ba37861ae711f8a5cd9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12104: 16324fdbea53e3a83778cc837d3c28fadde04d89 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

16324fdbea53 drm/i915/opregion: rvda is relative from opregion base, not absolute

== Logs ==

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

Re: [PATCH] drm/i915/opregion: rvda is relative from opregion base, not absolute

[Jani Nikula]

On Tue, 29 Jan 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> This is obviously a backward/forward incompatible change. I've been
> told there are no systems out there using the field.

There are systems like that, in our CI too. Back to the drawing board.

BR,
Jani.


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