[PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[Lucas De Marchi]

Convert the code to return-early style and fix missing calls
to release_firmware() if vbt is not valid.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index 969ade623691..9738511147b1 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
 		return ret;
 	}
 
-	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
-		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
-		if (opregion->vbt_firmware) {
-			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
-			opregion->vbt = opregion->vbt_firmware;
-			opregion->vbt_size = fw->size;
-			ret = 0;
-		} else {
-			ret = -ENOMEM;
-		}
-	} else {
+	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
 		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
 		ret = -EINVAL;
+		goto err_release_fw;
+	}
+
+	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
+	if (!opregion->vbt_firmware) {
+		ret = -ENOMEM;
+		goto err_release_fw;
 	}
 
+	opregion->vbt = opregion->vbt_firmware;
+	opregion->vbt_size = fw->size;
+
+	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
+
 	release_firmware(fw);
 
+	return 0;
+
+err_release_fw:
+	release_firmware(fw);
 	return ret;
 }
 
-- 
2.23.0

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

[Intel-gfx] [PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[Lucas De Marchi]

Convert the code to return-early style and fix missing calls
to release_firmware() if vbt is not valid.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index 969ade623691..9738511147b1 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
 		return ret;
 	}
 
-	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
-		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
-		if (opregion->vbt_firmware) {
-			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
-			opregion->vbt = opregion->vbt_firmware;
-			opregion->vbt_size = fw->size;
-			ret = 0;
-		} else {
-			ret = -ENOMEM;
-		}
-	} else {
+	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
 		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
 		ret = -EINVAL;
+		goto err_release_fw;
+	}
+
+	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
+	if (!opregion->vbt_firmware) {
+		ret = -ENOMEM;
+		goto err_release_fw;
 	}
 
+	opregion->vbt = opregion->vbt_firmware;
+	opregion->vbt_size = fw->size;
+
+	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
+
 	release_firmware(fw);
 
+	return 0;
+
+err_release_fw:
+	release_firmware(fw);
 	return ret;
 }
 
-- 
2.23.0

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

[PATCH 2/4] drm/i915/bios: rename bios to oprom when mapping pci rom

[Lucas De Marchi]

oprom is actually a better name to use when using
pci_map_rom(). "bios"  is way too generic and confusing.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index a03f56b7b4ef..1f83616cfc32 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1804,7 +1804,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
+static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
 {
 	size_t i;
 
@@ -1812,14 +1812,14 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 	for (i = 0; i + 4 < size; i++) {
 		void *vbt;
 
-		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
+		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
 			continue;
 
 		/*
 		 * This is the one place where we explicitly discard the address
 		 * space (__iomem) of the BIOS/VBT.
 		 */
-		vbt = (void __force *) bios + i;
+		vbt = (void __force *)oprom + i;
 		if (intel_bios_is_valid_vbt(vbt, size - i))
 			return vbt;
 
@@ -1842,7 +1842,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	const struct vbt_header *vbt = dev_priv->opregion.vbt;
 	const struct bdb_header *bdb;
-	u8 __iomem *bios = NULL;
+	u8 __iomem *oprom = NULL;
 
 	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv)) {
 		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
@@ -1855,11 +1855,11 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	if (!vbt) {
 		size_t size;
 
-		bios = pci_map_rom(pdev, &size);
-		if (!bios)
+		oprom = pci_map_rom(pdev, &size);
+		if (!oprom)
 			goto out;
 
-		vbt = find_vbt(bios, size);
+		vbt = find_vbt(oprom, size);
 		if (!vbt)
 			goto out;
 
@@ -1893,8 +1893,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 		init_vbt_missing_defaults(dev_priv);
 	}
 
-	if (bios)
-		pci_unmap_rom(pdev, bios);
+	if (oprom)
+		pci_unmap_rom(pdev, oprom);
 }
 
 /**
-- 
2.23.0

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

[Intel-gfx] [PATCH 2/4] drm/i915/bios: rename bios to oprom when mapping pci rom

[Lucas De Marchi]

oprom is actually a better name to use when using
pci_map_rom(). "bios"  is way too generic and confusing.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index a03f56b7b4ef..1f83616cfc32 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1804,7 +1804,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
+static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
 {
 	size_t i;
 
@@ -1812,14 +1812,14 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
 	for (i = 0; i + 4 < size; i++) {
 		void *vbt;
 
-		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
+		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
 			continue;
 
 		/*
 		 * This is the one place where we explicitly discard the address
 		 * space (__iomem) of the BIOS/VBT.
 		 */
-		vbt = (void __force *) bios + i;
+		vbt = (void __force *)oprom + i;
 		if (intel_bios_is_valid_vbt(vbt, size - i))
 			return vbt;
 
@@ -1842,7 +1842,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	const struct vbt_header *vbt = dev_priv->opregion.vbt;
 	const struct bdb_header *bdb;
-	u8 __iomem *bios = NULL;
+	u8 __iomem *oprom = NULL;
 
 	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv)) {
 		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
@@ -1855,11 +1855,11 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	if (!vbt) {
 		size_t size;
 
-		bios = pci_map_rom(pdev, &size);
-		if (!bios)
+		oprom = pci_map_rom(pdev, &size);
+		if (!oprom)
 			goto out;
 
-		vbt = find_vbt(bios, size);
+		vbt = find_vbt(oprom, size);
 		if (!vbt)
 			goto out;
 
@@ -1893,8 +1893,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 		init_vbt_missing_defaults(dev_priv);
 	}
 
-	if (bios)
-		pci_unmap_rom(pdev, bios);
+	if (oprom)
+		pci_unmap_rom(pdev, oprom);
 }
 
 /**
-- 
2.23.0

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

Re: [PATCH 2/4] drm/i915/bios: rename bios to oprom when mapping pci rom

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> oprom is actually a better name to use when using
> pci_map_rom(). "bios"  is way too generic and confusing.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index a03f56b7b4ef..1f83616cfc32 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1804,7 +1804,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	return vbt;
>  }
>  
> -static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
> +static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>  {
>  	size_t i;
>  
> @@ -1812,14 +1812,14 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
>  	for (i = 0; i + 4 < size; i++) {
>  		void *vbt;
>  
> -		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
> +		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>  			continue;
>  
>  		/*
>  		 * This is the one place where we explicitly discard the address
>  		 * space (__iomem) of the BIOS/VBT.
>  		 */
> -		vbt = (void __force *) bios + i;
> +		vbt = (void __force *)oprom + i;
>  		if (intel_bios_is_valid_vbt(vbt, size - i))
>  			return vbt;
>  
> @@ -1842,7 +1842,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	const struct vbt_header *vbt = dev_priv->opregion.vbt;
>  	const struct bdb_header *bdb;
> -	u8 __iomem *bios = NULL;
> +	u8 __iomem *oprom = NULL;
>  
>  	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv)) {
>  		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
> @@ -1855,11 +1855,11 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  	if (!vbt) {
>  		size_t size;
>  
> -		bios = pci_map_rom(pdev, &size);
> -		if (!bios)
> +		oprom = pci_map_rom(pdev, &size);
> +		if (!oprom)
>  			goto out;
>  
> -		vbt = find_vbt(bios, size);
> +		vbt = find_vbt(oprom, size);
>  		if (!vbt)
>  			goto out;
>  
> @@ -1893,8 +1893,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  		init_vbt_missing_defaults(dev_priv);
>  	}
>  
> -	if (bios)
> -		pci_unmap_rom(pdev, bios);
> +	if (oprom)
> +		pci_unmap_rom(pdev, oprom);
>  }
>  
>  /**

-- 
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: [Intel-gfx] [PATCH 2/4] drm/i915/bios: rename bios to oprom when mapping pci rom

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> oprom is actually a better name to use when using
> pci_map_rom(). "bios"  is way too generic and confusing.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index a03f56b7b4ef..1f83616cfc32 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1804,7 +1804,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	return vbt;
>  }
>  
> -static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
> +static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>  {
>  	size_t i;
>  
> @@ -1812,14 +1812,14 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
>  	for (i = 0; i + 4 < size; i++) {
>  		void *vbt;
>  
> -		if (ioread32(bios + i) != *((const u32 *) "$VBT"))
> +		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>  			continue;
>  
>  		/*
>  		 * This is the one place where we explicitly discard the address
>  		 * space (__iomem) of the BIOS/VBT.
>  		 */
> -		vbt = (void __force *) bios + i;
> +		vbt = (void __force *)oprom + i;
>  		if (intel_bios_is_valid_vbt(vbt, size - i))
>  			return vbt;
>  
> @@ -1842,7 +1842,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	const struct vbt_header *vbt = dev_priv->opregion.vbt;
>  	const struct bdb_header *bdb;
> -	u8 __iomem *bios = NULL;
> +	u8 __iomem *oprom = NULL;
>  
>  	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv)) {
>  		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
> @@ -1855,11 +1855,11 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  	if (!vbt) {
>  		size_t size;
>  
> -		bios = pci_map_rom(pdev, &size);
> -		if (!bios)
> +		oprom = pci_map_rom(pdev, &size);
> +		if (!oprom)
>  			goto out;
>  
> -		vbt = find_vbt(bios, size);
> +		vbt = find_vbt(oprom, size);
>  		if (!vbt)
>  			goto out;
>  
> @@ -1893,8 +1893,8 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  		init_vbt_missing_defaults(dev_priv);
>  	}
>  
> -	if (bios)
> -		pci_unmap_rom(pdev, bios);
> +	if (oprom)
> +		pci_unmap_rom(pdev, oprom);
>  }
>  
>  /**

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

[PATCH 3/4] drm/i915/bios: make sure to check vbt size

[Lucas De Marchi]

When we call intel_bios_is_valid_vbt(), size may not actually be the
size of the VBT, but rather the size of the blob the VBT is contained
in. For example, when mapping the PCI oprom, size will be the entire
oprom size. We don't want to read beyond what is reported to be the
VBT. So make sure we vbt->vbt_size makes sense and use that for
the latter checks.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 1f83616cfc32..671bbce6ba5b 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1777,11 +1777,13 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	if (!vbt)
 		return false;
 
-	if (sizeof(struct vbt_header) > size) {
+	if (sizeof(struct vbt_header) > size || vbt->vbt_size > size) {
 		DRM_DEBUG_DRIVER("VBT header incomplete\n");
 		return false;
 	}
 
+	size = vbt->vbt_size;
+
 	if (memcmp(vbt->signature, "$VBT", 4)) {
 		DRM_DEBUG_DRIVER("VBT invalid signature\n");
 		return false;
-- 
2.23.0

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

[Intel-gfx] [PATCH 3/4] drm/i915/bios: make sure to check vbt size

[Lucas De Marchi]

When we call intel_bios_is_valid_vbt(), size may not actually be the
size of the VBT, but rather the size of the blob the VBT is contained
in. For example, when mapping the PCI oprom, size will be the entire
oprom size. We don't want to read beyond what is reported to be the
VBT. So make sure we vbt->vbt_size makes sense and use that for
the latter checks.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 1f83616cfc32..671bbce6ba5b 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1777,11 +1777,13 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	if (!vbt)
 		return false;
 
-	if (sizeof(struct vbt_header) > size) {
+	if (sizeof(struct vbt_header) > size || vbt->vbt_size > size) {
 		DRM_DEBUG_DRIVER("VBT header incomplete\n");
 		return false;
 	}
 
+	size = vbt->vbt_size;
+
 	if (memcmp(vbt->signature, "$VBT", 4)) {
 		DRM_DEBUG_DRIVER("VBT invalid signature\n");
 		return false;
-- 
2.23.0

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

Re: [PATCH 3/4] drm/i915/bios: make sure to check vbt size

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> When we call intel_bios_is_valid_vbt(), size may not actually be the
> size of the VBT, but rather the size of the blob the VBT is contained
> in. For example, when mapping the PCI oprom, size will be the entire
> oprom size. We don't want to read beyond what is reported to be the
> VBT. So make sure we vbt->vbt_size makes sense and use that for
> the latter checks.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 1f83616cfc32..671bbce6ba5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1777,11 +1777,13 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	if (!vbt)
>  		return false;
>  
> -	if (sizeof(struct vbt_header) > size) {
> +	if (sizeof(struct vbt_header) > size || vbt->vbt_size > size) {
>  		DRM_DEBUG_DRIVER("VBT header incomplete\n");

Nitpick #1, semantically you should check the VBT signature before you
know ->vbt_size might make sense.

Nitpick #2, the debug message becomes increasingly non-informative. But
basically most messages in this function are less than stellar.

In any case, the goal is sane,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  		return false;
>  	}
>  
> +	size = vbt->vbt_size;
> +
>  	if (memcmp(vbt->signature, "$VBT", 4)) {
>  		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>  		return false;

-- 
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: [Intel-gfx] [PATCH 3/4] drm/i915/bios: make sure to check vbt size

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> When we call intel_bios_is_valid_vbt(), size may not actually be the
> size of the VBT, but rather the size of the blob the VBT is contained
> in. For example, when mapping the PCI oprom, size will be the entire
> oprom size. We don't want to read beyond what is reported to be the
> VBT. So make sure we vbt->vbt_size makes sense and use that for
> the latter checks.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 1f83616cfc32..671bbce6ba5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1777,11 +1777,13 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	if (!vbt)
>  		return false;
>  
> -	if (sizeof(struct vbt_header) > size) {
> +	if (sizeof(struct vbt_header) > size || vbt->vbt_size > size) {
>  		DRM_DEBUG_DRIVER("VBT header incomplete\n");

Nitpick #1, semantically you should check the VBT signature before you
know ->vbt_size might make sense.

Nitpick #2, the debug message becomes increasingly non-informative. But
basically most messages in this function are less than stellar.

In any case, the goal is sane,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  		return false;
>  	}
>  
> +	size = vbt->vbt_size;
> +
>  	if (memcmp(vbt->signature, "$VBT", 4)) {
>  		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>  		return false;

-- 
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 3/4] drm/i915/bios: make sure to check vbt size

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 12:08:52PM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> When we call intel_bios_is_valid_vbt(), size may not actually be the
>> size of the VBT, but rather the size of the blob the VBT is contained
>> in. For example, when mapping the PCI oprom, size will be the entire
>> oprom size. We don't want to read beyond what is reported to be the
>> VBT. So make sure we vbt->vbt_size makes sense and use that for
>> the latter checks.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 1f83616cfc32..671bbce6ba5b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1777,11 +1777,13 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>>  	if (!vbt)
>>  		return false;
>>
>> -	if (sizeof(struct vbt_header) > size) {
>> +	if (sizeof(struct vbt_header) > size || vbt->vbt_size > size) {
>>  		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>
>Nitpick #1, semantically you should check the VBT signature before you
>know ->vbt_size might make sense.
>
>Nitpick #2, the debug message becomes increasingly non-informative. But
>basically most messages in this function are less than stellar.

I can move this additional check after the signature check and then give
it a better error message. This is what I did in copy_vbt() anyway in
the next patch (but just for the pci rom).

thanks
Lucas De Marchi

>
>In any case, the goal is sane,
>
>Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>>  		return false;
>>  	}
>>
>> +	size = vbt->vbt_size;
>> +
>>  	if (memcmp(vbt->signature, "$VBT", 4)) {
>>  		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>>  		return false;
>
>-- 
>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: [Intel-gfx] [PATCH 3/4] drm/i915/bios: make sure to check vbt size

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 12:08:52PM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> When we call intel_bios_is_valid_vbt(), size may not actually be the
>> size of the VBT, but rather the size of the blob the VBT is contained
>> in. For example, when mapping the PCI oprom, size will be the entire
>> oprom size. We don't want to read beyond what is reported to be the
>> VBT. So make sure we vbt->vbt_size makes sense and use that for
>> the latter checks.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 1f83616cfc32..671bbce6ba5b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1777,11 +1777,13 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>>  	if (!vbt)
>>  		return false;
>>
>> -	if (sizeof(struct vbt_header) > size) {
>> +	if (sizeof(struct vbt_header) > size || vbt->vbt_size > size) {
>>  		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>
>Nitpick #1, semantically you should check the VBT signature before you
>know ->vbt_size might make sense.
>
>Nitpick #2, the debug message becomes increasingly non-informative. But
>basically most messages in this function are less than stellar.

I can move this additional check after the signature check and then give
it a better error message. This is what I did in copy_vbt() anyway in
the next patch (but just for the pci rom).

thanks
Lucas De Marchi

>
>In any case, the goal is sane,
>
>Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>>  		return false;
>>  	}
>>
>> +	size = vbt->vbt_size;
>> +
>>  	if (memcmp(vbt->signature, "$VBT", 4)) {
>>  		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>>  		return false;
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

When we are mapping the VBT through pci_map_rom() we may not be allowed
to simply discard the address space and go on reading the memory. After
checking on my test system that dumping the rom via sysfs I could
actually get the correct vbt, I decided to change the implementation to
use the same approach, by calling memcpy_fromio().

In order to avoid copying the entire oprom this implements a simple
memmem() searching for "$VBT". Contrary to the previous implementation
this also takes care of not issuing unaligned PCI reads that would
otherwise get translated into more even more reads. I also vaguely
remember unaligned reads failing in the past with some devices.

Also make sure we copy only the VBT and not the entire oprom that is
usually much larger.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
 1 file changed, 79 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 671bbce6ba5b..c401e90b7cf1 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
+void __iomem *find_vbt(void __iomem *oprom, size_t size)
 {
-	size_t i;
+	const u32 MAGIC = *((const u32 *)"$VBT");
+	size_t done = 0, cur = 0;
+	void __iomem *p;
+	u8 buf[128];
+	u32 val;
 
-	/* Scour memory looking for the VBT signature. */
-	for (i = 0; i + 4 < size; i++) {
-		void *vbt;
+	/*
+	 * poor's man memmem() with sizeof(buf) window to avoid frequent
+	 * wrap-arounds and using u32 for comparison. This gives us 4
+	 * comparisons per ioread32() and avoids unaligned io reads (although it
+	 * still does unaligned cpu access).
+	 */
+	for (p = oprom; p < oprom + size; p += 4) {
+		*(u32 *)(&buf[done]) = ioread32(p);
+		done += 4;
 
-		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
-			continue;
+		while (cur + 4 <= done) {
+			val = *(u32 *)(buf + cur);
+			if (val == MAGIC)
+				return p - (done - cur) + 4;
 
-		/*
-		 * This is the one place where we explicitly discard the address
-		 * space (__iomem) of the BIOS/VBT.
-		 */
-		vbt = (void __force *)oprom + i;
-		if (intel_bios_is_valid_vbt(vbt, size - i))
-			return vbt;
+			cur++;
+		}
 
-		break;
+		/* wrap-around */
+		if (done + 4 >= sizeof(buf)) {
+			buf[0] = buf[done - 3];
+			buf[1] = buf[done - 2];
+			buf[2] = buf[done - 1];
+			cur = 0;
+			done = 3;
+		}
 	}
 
+	/* Read the entire oprom and no VBT found */
 	return NULL;
 }
 
+/*
+ * Copy vbt to a new allocated buffer and update @psize to match the VBT
+ * size
+ */
+static struct vbt_header *copy_vbt(void __iomem *oprom, size_t *psize)
+{
+	off_t vbt_size_offset = offsetof(struct vbt_header, vbt_size);
+	struct vbt_header *vbt;
+	void __iomem *p;
+	u16 vbt_size;
+	size_t size;
+
+	size = *psize;
+	p = find_vbt(oprom, size);
+	if (!p)
+		return NULL;
+
+	size -= p - oprom;
+
+	/*
+	 * We need to at least be able to read the size and make sure it doesn't
+	 * overflow the oprom. The rest will be validated later.
+	 */
+	if (sizeof(*vbt) > size) {
+		DRM_DEBUG_DRIVER("VBT header incomplete\n");
+		return NULL;
+	}
+
+	vbt_size = ioread16(p + vbt_size_offset);
+	if (vbt_size > size) {
+		DRM_DEBUG_DRIVER("VBT incomplete\n");
+		return NULL;
+	}
+
+	vbt = kmalloc(vbt_size, GFP_KERNEL);
+	memcpy_fromio(vbt, p, vbt_size);
+
+	*psize = vbt_size;
+
+	return vbt;
+}
+
 /**
  * intel_bios_init - find VBT and initialize settings from the BIOS
  * @dev_priv: i915 device instance
@@ -1861,10 +1918,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 		if (!oprom)
 			goto out;
 
-		vbt = find_vbt(oprom, size);
+		vbt = copy_vbt(oprom, &size);
 		if (!vbt)
 			goto out;
 
+		if (!intel_bios_is_valid_vbt(vbt, size))
+			goto out;
+
 		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
 	}
 
@@ -1897,6 +1957,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 
 	if (oprom)
 		pci_unmap_rom(pdev, oprom);
+
+	if (vbt != dev_priv->opregion.vbt)
+		kfree(vbt);
 }
 
 /**
-- 
2.23.0

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

[Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

When we are mapping the VBT through pci_map_rom() we may not be allowed
to simply discard the address space and go on reading the memory. After
checking on my test system that dumping the rom via sysfs I could
actually get the correct vbt, I decided to change the implementation to
use the same approach, by calling memcpy_fromio().

In order to avoid copying the entire oprom this implements a simple
memmem() searching for "$VBT". Contrary to the previous implementation
this also takes care of not issuing unaligned PCI reads that would
otherwise get translated into more even more reads. I also vaguely
remember unaligned reads failing in the past with some devices.

Also make sure we copy only the VBT and not the entire oprom that is
usually much larger.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
 1 file changed, 79 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 671bbce6ba5b..c401e90b7cf1 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
+void __iomem *find_vbt(void __iomem *oprom, size_t size)
 {
-	size_t i;
+	const u32 MAGIC = *((const u32 *)"$VBT");
+	size_t done = 0, cur = 0;
+	void __iomem *p;
+	u8 buf[128];
+	u32 val;
 
-	/* Scour memory looking for the VBT signature. */
-	for (i = 0; i + 4 < size; i++) {
-		void *vbt;
+	/*
+	 * poor's man memmem() with sizeof(buf) window to avoid frequent
+	 * wrap-arounds and using u32 for comparison. This gives us 4
+	 * comparisons per ioread32() and avoids unaligned io reads (although it
+	 * still does unaligned cpu access).
+	 */
+	for (p = oprom; p < oprom + size; p += 4) {
+		*(u32 *)(&buf[done]) = ioread32(p);
+		done += 4;
 
-		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
-			continue;
+		while (cur + 4 <= done) {
+			val = *(u32 *)(buf + cur);
+			if (val == MAGIC)
+				return p - (done - cur) + 4;
 
-		/*
-		 * This is the one place where we explicitly discard the address
-		 * space (__iomem) of the BIOS/VBT.
-		 */
-		vbt = (void __force *)oprom + i;
-		if (intel_bios_is_valid_vbt(vbt, size - i))
-			return vbt;
+			cur++;
+		}
 
-		break;
+		/* wrap-around */
+		if (done + 4 >= sizeof(buf)) {
+			buf[0] = buf[done - 3];
+			buf[1] = buf[done - 2];
+			buf[2] = buf[done - 1];
+			cur = 0;
+			done = 3;
+		}
 	}
 
+	/* Read the entire oprom and no VBT found */
 	return NULL;
 }
 
+/*
+ * Copy vbt to a new allocated buffer and update @psize to match the VBT
+ * size
+ */
+static struct vbt_header *copy_vbt(void __iomem *oprom, size_t *psize)
+{
+	off_t vbt_size_offset = offsetof(struct vbt_header, vbt_size);
+	struct vbt_header *vbt;
+	void __iomem *p;
+	u16 vbt_size;
+	size_t size;
+
+	size = *psize;
+	p = find_vbt(oprom, size);
+	if (!p)
+		return NULL;
+
+	size -= p - oprom;
+
+	/*
+	 * We need to at least be able to read the size and make sure it doesn't
+	 * overflow the oprom. The rest will be validated later.
+	 */
+	if (sizeof(*vbt) > size) {
+		DRM_DEBUG_DRIVER("VBT header incomplete\n");
+		return NULL;
+	}
+
+	vbt_size = ioread16(p + vbt_size_offset);
+	if (vbt_size > size) {
+		DRM_DEBUG_DRIVER("VBT incomplete\n");
+		return NULL;
+	}
+
+	vbt = kmalloc(vbt_size, GFP_KERNEL);
+	memcpy_fromio(vbt, p, vbt_size);
+
+	*psize = vbt_size;
+
+	return vbt;
+}
+
 /**
  * intel_bios_init - find VBT and initialize settings from the BIOS
  * @dev_priv: i915 device instance
@@ -1861,10 +1918,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 		if (!oprom)
 			goto out;
 
-		vbt = find_vbt(oprom, size);
+		vbt = copy_vbt(oprom, &size);
 		if (!vbt)
 			goto out;
 
+		if (!intel_bios_is_valid_vbt(vbt, size))
+			goto out;
+
 		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
 	}
 
@@ -1897,6 +1957,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 
 	if (oprom)
 		pci_unmap_rom(pdev, oprom);
+
+	if (vbt != dev_priv->opregion.vbt)
+		kfree(vbt);
 }
 
 /**
-- 
2.23.0

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

Re: [PATCH 4/4] drm/i915/bios: do not discard address space

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> When we are mapping the VBT through pci_map_rom() we may not be allowed
> to simply discard the address space and go on reading the memory. After
> checking on my test system that dumping the rom via sysfs I could
> actually get the correct vbt, I decided to change the implementation to
> use the same approach, by calling memcpy_fromio().
>
> In order to avoid copying the entire oprom this implements a simple
> memmem() searching for "$VBT". Contrary to the previous implementation
> this also takes care of not issuing unaligned PCI reads that would
> otherwise get translated into more even more reads. I also vaguely
> remember unaligned reads failing in the past with some devices.
>
> Also make sure we copy only the VBT and not the entire oprom that is
> usually much larger.

So you have

1. a fix to unaligned reads

2. an optimization to avoid reading individual bytes four times

3. respecting __iomem and copying (I guess these are tied together)

Seems to me that really should be at least three patches. Not
necessarily in the above order.

Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
have debugfs i915_vbt() handle that properly.

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>  1 file changed, 79 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 671bbce6ba5b..c401e90b7cf1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	return vbt;
>  }
>  
> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>  {
> -	size_t i;
> +	const u32 MAGIC = *((const u32 *)"$VBT");
> +	size_t done = 0, cur = 0;
> +	void __iomem *p;
> +	u8 buf[128];
> +	u32 val;
>  
> -	/* Scour memory looking for the VBT signature. */
> -	for (i = 0; i + 4 < size; i++) {
> -		void *vbt;
> +	/*
> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
> +	 * wrap-arounds and using u32 for comparison. This gives us 4
> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
> +	 * still does unaligned cpu access).
> +	 */

If we're really worried about performance here, and use a local buffer
to optimize the wraparounds, would it actually be more efficient to use
memcpy_fromio() which has an arch specific implementation in asm?

In any case makes you think you should first have the patch that the
patch subject claims, fix unaligned reads and add optimizations
next. This one does too much.

BR,
Jani.



> +	for (p = oprom; p < oprom + size; p += 4) {
> +		*(u32 *)(&buf[done]) = ioread32(p);
> +		done += 4;
>  
> -		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
> -			continue;
> +		while (cur + 4 <= done) {
> +			val = *(u32 *)(buf + cur);
> +			if (val == MAGIC)
> +				return p - (done - cur) + 4;
>  
> -		/*
> -		 * This is the one place where we explicitly discard the address
> -		 * space (__iomem) of the BIOS/VBT.
> -		 */
> -		vbt = (void __force *)oprom + i;
> -		if (intel_bios_is_valid_vbt(vbt, size - i))
> -			return vbt;
> +			cur++;
> +		}
>  
> -		break;
> +		/* wrap-around */
> +		if (done + 4 >= sizeof(buf)) {
> +			buf[0] = buf[done - 3];
> +			buf[1] = buf[done - 2];
> +			buf[2] = buf[done - 1];
> +			cur = 0;
> +			done = 3;
> +		}
>  	}
>  
> +	/* Read the entire oprom and no VBT found */
>  	return NULL;
>  }
>  
> +/*
> + * Copy vbt to a new allocated buffer and update @psize to match the VBT
> + * size
> + */
> +static struct vbt_header *copy_vbt(void __iomem *oprom, size_t *psize)
> +{
> +	off_t vbt_size_offset = offsetof(struct vbt_header, vbt_size);
> +	struct vbt_header *vbt;
> +	void __iomem *p;
> +	u16 vbt_size;
> +	size_t size;
> +
> +	size = *psize;
> +	p = find_vbt(oprom, size);
> +	if (!p)
> +		return NULL;
> +
> +	size -= p - oprom;
> +
> +	/*
> +	 * We need to at least be able to read the size and make sure it doesn't
> +	 * overflow the oprom. The rest will be validated later.
> +	 */
> +	if (sizeof(*vbt) > size) {
> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> +		return NULL;
> +	}
> +
> +	vbt_size = ioread16(p + vbt_size_offset);
> +	if (vbt_size > size) {
> +		DRM_DEBUG_DRIVER("VBT incomplete\n");
> +		return NULL;
> +	}
> +
> +	vbt = kmalloc(vbt_size, GFP_KERNEL);
> +	memcpy_fromio(vbt, p, vbt_size);
> +
> +	*psize = vbt_size;
> +
> +	return vbt;
> +}
> +
>  /**
>   * intel_bios_init - find VBT and initialize settings from the BIOS
>   * @dev_priv: i915 device instance
> @@ -1861,10 +1918,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  		if (!oprom)
>  			goto out;
>  
> -		vbt = find_vbt(oprom, size);
> +		vbt = copy_vbt(oprom, &size);
>  		if (!vbt)
>  			goto out;
>  
> +		if (!intel_bios_is_valid_vbt(vbt, size))
> +			goto out;
> +
>  		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
>  	}
>  
> @@ -1897,6 +1957,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  
>  	if (oprom)
>  		pci_unmap_rom(pdev, oprom);
> +
> +	if (vbt != dev_priv->opregion.vbt)
> +		kfree(vbt);
>  }
>  
>  /**

-- 
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: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> When we are mapping the VBT through pci_map_rom() we may not be allowed
> to simply discard the address space and go on reading the memory. After
> checking on my test system that dumping the rom via sysfs I could
> actually get the correct vbt, I decided to change the implementation to
> use the same approach, by calling memcpy_fromio().
>
> In order to avoid copying the entire oprom this implements a simple
> memmem() searching for "$VBT". Contrary to the previous implementation
> this also takes care of not issuing unaligned PCI reads that would
> otherwise get translated into more even more reads. I also vaguely
> remember unaligned reads failing in the past with some devices.
>
> Also make sure we copy only the VBT and not the entire oprom that is
> usually much larger.

So you have

1. a fix to unaligned reads

2. an optimization to avoid reading individual bytes four times

3. respecting __iomem and copying (I guess these are tied together)

Seems to me that really should be at least three patches. Not
necessarily in the above order.

Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
have debugfs i915_vbt() handle that properly.

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>  1 file changed, 79 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 671bbce6ba5b..c401e90b7cf1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>  	return vbt;
>  }
>  
> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>  {
> -	size_t i;
> +	const u32 MAGIC = *((const u32 *)"$VBT");
> +	size_t done = 0, cur = 0;
> +	void __iomem *p;
> +	u8 buf[128];
> +	u32 val;
>  
> -	/* Scour memory looking for the VBT signature. */
> -	for (i = 0; i + 4 < size; i++) {
> -		void *vbt;
> +	/*
> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
> +	 * wrap-arounds and using u32 for comparison. This gives us 4
> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
> +	 * still does unaligned cpu access).
> +	 */

If we're really worried about performance here, and use a local buffer
to optimize the wraparounds, would it actually be more efficient to use
memcpy_fromio() which has an arch specific implementation in asm?

In any case makes you think you should first have the patch that the
patch subject claims, fix unaligned reads and add optimizations
next. This one does too much.

BR,
Jani.



> +	for (p = oprom; p < oprom + size; p += 4) {
> +		*(u32 *)(&buf[done]) = ioread32(p);
> +		done += 4;
>  
> -		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
> -			continue;
> +		while (cur + 4 <= done) {
> +			val = *(u32 *)(buf + cur);
> +			if (val == MAGIC)
> +				return p - (done - cur) + 4;
>  
> -		/*
> -		 * This is the one place where we explicitly discard the address
> -		 * space (__iomem) of the BIOS/VBT.
> -		 */
> -		vbt = (void __force *)oprom + i;
> -		if (intel_bios_is_valid_vbt(vbt, size - i))
> -			return vbt;
> +			cur++;
> +		}
>  
> -		break;
> +		/* wrap-around */
> +		if (done + 4 >= sizeof(buf)) {
> +			buf[0] = buf[done - 3];
> +			buf[1] = buf[done - 2];
> +			buf[2] = buf[done - 1];
> +			cur = 0;
> +			done = 3;
> +		}
>  	}
>  
> +	/* Read the entire oprom and no VBT found */
>  	return NULL;
>  }
>  
> +/*
> + * Copy vbt to a new allocated buffer and update @psize to match the VBT
> + * size
> + */
> +static struct vbt_header *copy_vbt(void __iomem *oprom, size_t *psize)
> +{
> +	off_t vbt_size_offset = offsetof(struct vbt_header, vbt_size);
> +	struct vbt_header *vbt;
> +	void __iomem *p;
> +	u16 vbt_size;
> +	size_t size;
> +
> +	size = *psize;
> +	p = find_vbt(oprom, size);
> +	if (!p)
> +		return NULL;
> +
> +	size -= p - oprom;
> +
> +	/*
> +	 * We need to at least be able to read the size and make sure it doesn't
> +	 * overflow the oprom. The rest will be validated later.
> +	 */
> +	if (sizeof(*vbt) > size) {
> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> +		return NULL;
> +	}
> +
> +	vbt_size = ioread16(p + vbt_size_offset);
> +	if (vbt_size > size) {
> +		DRM_DEBUG_DRIVER("VBT incomplete\n");
> +		return NULL;
> +	}
> +
> +	vbt = kmalloc(vbt_size, GFP_KERNEL);
> +	memcpy_fromio(vbt, p, vbt_size);
> +
> +	*psize = vbt_size;
> +
> +	return vbt;
> +}
> +
>  /**
>   * intel_bios_init - find VBT and initialize settings from the BIOS
>   * @dev_priv: i915 device instance
> @@ -1861,10 +1918,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  		if (!oprom)
>  			goto out;
>  
> -		vbt = find_vbt(oprom, size);
> +		vbt = copy_vbt(oprom, &size);
>  		if (!vbt)
>  			goto out;
>  
> +		if (!intel_bios_is_valid_vbt(vbt, size))
> +			goto out;
> +
>  		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
>  	}
>  
> @@ -1897,6 +1957,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>  
>  	if (oprom)
>  		pci_unmap_rom(pdev, oprom);
> +
> +	if (vbt != dev_priv->opregion.vbt)
> +		kfree(vbt);
>  }
>  
>  /**

-- 
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 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> When we are mapping the VBT through pci_map_rom() we may not be allowed
>> to simply discard the address space and go on reading the memory. After
>> checking on my test system that dumping the rom via sysfs I could
>> actually get the correct vbt, I decided to change the implementation to
>> use the same approach, by calling memcpy_fromio().
>>
>> In order to avoid copying the entire oprom this implements a simple
>> memmem() searching for "$VBT". Contrary to the previous implementation
>> this also takes care of not issuing unaligned PCI reads that would
>> otherwise get translated into more even more reads. I also vaguely
>> remember unaligned reads failing in the past with some devices.
>>
>> Also make sure we copy only the VBT and not the entire oprom that is
>> usually much larger.
>
>So you have
>
>1. a fix to unaligned reads

unaligned io reads, yes

>
>2. an optimization to avoid reading individual bytes four times

it was by no means an optimization. Not reading the same byte 4 bytes is
there actually to stop doing the unaligned IO reads. You can't have (2)
without (1) unless you switch to ioreadb() and add a shift (which may
not be a bad idea.

>
>3. respecting __iomem and copying (I guess these are tied together)
>
>Seems to me that really should be at least three patches. Not
>necessarily in the above order.

(3) is actually the most important I think, so I will start by that.

>
>Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>have debugfs i915_vbt() handle that properly.

I don't think this is needed. The thing I'm doing here is the same as
what can be accomplished by reading the rom from sysfs:

find /sys/bus/pci/devices/*/ -name rom
... choose one

echo 1 > rom # to allow reading the rom
hexdump -C rom


>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>>  1 file changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 671bbce6ba5b..c401e90b7cf1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>>  	return vbt;
>>  }
>>
>> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>>  {
>> -	size_t i;
>> +	const u32 MAGIC = *((const u32 *)"$VBT");
>> +	size_t done = 0, cur = 0;
>> +	void __iomem *p;
>> +	u8 buf[128];
>> +	u32 val;
>>
>> -	/* Scour memory looking for the VBT signature. */
>> -	for (i = 0; i + 4 < size; i++) {
>> -		void *vbt;
>> +	/*
>> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
>> +	 * wrap-arounds and using u32 for comparison. This gives us 4
>> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
>> +	 * still does unaligned cpu access).
>> +	 */
>
>If we're really worried about performance here, and use a local buffer
>to optimize the wraparounds, would it actually be more efficient to use
>memcpy_fromio() which has an arch specific implementation in asm?

Not really worried about performance. I actually did 3 implementations
that avoids the unaligned io reads.

1) this one
2) memcpy_fromio() to the local buffer + strnstr()
3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
pointer to it. Then free the oprom after the vbt is used

(2) and (1) had basically the same complexity involved of requiring a
wrap around local buffer, so I went with (1)

I didn't feel confortable with (3) because it would allocate much more
memory than really needed.

>
>In any case makes you think you should first have the patch that the
>patch subject claims, fix unaligned reads and add optimizations
>next. This one does too much.

Again, it was not really meant to be an optimization.

Ville told me that we may not really need to deal with the unaligned
access and change the implementation to expect the VBT to be aligned.
This I would be the simplest way to change it, but I'm not fond on
changing this and breaking old systems usin it... anyway, we can give it
a try and revert if it breaks.

I will send a new version with the split.

Lucas De Marchi

>
>BR,
>Jani.
>
>
>
>> +	for (p = oprom; p < oprom + size; p += 4) {
>> +		*(u32 *)(&buf[done]) = ioread32(p);
>> +		done += 4;
>>
>> -		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>> -			continue;
>> +		while (cur + 4 <= done) {
>> +			val = *(u32 *)(buf + cur);
>> +			if (val == MAGIC)
>> +				return p - (done - cur) + 4;
>>
>> -		/*
>> -		 * This is the one place where we explicitly discard the address
>> -		 * space (__iomem) of the BIOS/VBT.
>> -		 */
>> -		vbt = (void __force *)oprom + i;
>> -		if (intel_bios_is_valid_vbt(vbt, size - i))
>> -			return vbt;
>> +			cur++;
>> +		}
>>
>> -		break;
>> +		/* wrap-around */
>> +		if (done + 4 >= sizeof(buf)) {
>> +			buf[0] = buf[done - 3];
>> +			buf[1] = buf[done - 2];
>> +			buf[2] = buf[done - 1];
>> +			cur = 0;
>> +			done = 3;
>> +		}
>>  	}
>>
>> +	/* Read the entire oprom and no VBT found */
>>  	return NULL;
>>  }
>>
>> +/*
>> + * Copy vbt to a new allocated buffer and update @psize to match the VBT
>> + * size
>> + */
>> +static struct vbt_header *copy_vbt(void __iomem *oprom, size_t *psize)
>> +{
>> +	off_t vbt_size_offset = offsetof(struct vbt_header, vbt_size);
>> +	struct vbt_header *vbt;
>> +	void __iomem *p;
>> +	u16 vbt_size;
>> +	size_t size;
>> +
>> +	size = *psize;
>> +	p = find_vbt(oprom, size);
>> +	if (!p)
>> +		return NULL;
>> +
>> +	size -= p - oprom;
>> +
>> +	/*
>> +	 * We need to at least be able to read the size and make sure it doesn't
>> +	 * overflow the oprom. The rest will be validated later.
>> +	 */
>> +	if (sizeof(*vbt) > size) {
>> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>> +		return NULL;
>> +	}
>> +
>> +	vbt_size = ioread16(p + vbt_size_offset);
>> +	if (vbt_size > size) {
>> +		DRM_DEBUG_DRIVER("VBT incomplete\n");
>> +		return NULL;
>> +	}
>> +
>> +	vbt = kmalloc(vbt_size, GFP_KERNEL);
>> +	memcpy_fromio(vbt, p, vbt_size);
>> +
>> +	*psize = vbt_size;
>> +
>> +	return vbt;
>> +}
>> +
>>  /**
>>   * intel_bios_init - find VBT and initialize settings from the BIOS
>>   * @dev_priv: i915 device instance
>> @@ -1861,10 +1918,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>  		if (!oprom)
>>  			goto out;
>>
>> -		vbt = find_vbt(oprom, size);
>> +		vbt = copy_vbt(oprom, &size);
>>  		if (!vbt)
>>  			goto out;
>>
>> +		if (!intel_bios_is_valid_vbt(vbt, size))
>> +			goto out;
>> +
>>  		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
>>  	}
>>
>> @@ -1897,6 +1957,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>
>>  	if (oprom)
>>  		pci_unmap_rom(pdev, oprom);
>> +
>> +	if (vbt != dev_priv->opregion.vbt)
>> +		kfree(vbt);
>>  }
>>
>>  /**
>
>-- 
>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: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> When we are mapping the VBT through pci_map_rom() we may not be allowed
>> to simply discard the address space and go on reading the memory. After
>> checking on my test system that dumping the rom via sysfs I could
>> actually get the correct vbt, I decided to change the implementation to
>> use the same approach, by calling memcpy_fromio().
>>
>> In order to avoid copying the entire oprom this implements a simple
>> memmem() searching for "$VBT". Contrary to the previous implementation
>> this also takes care of not issuing unaligned PCI reads that would
>> otherwise get translated into more even more reads. I also vaguely
>> remember unaligned reads failing in the past with some devices.
>>
>> Also make sure we copy only the VBT and not the entire oprom that is
>> usually much larger.
>
>So you have
>
>1. a fix to unaligned reads

unaligned io reads, yes

>
>2. an optimization to avoid reading individual bytes four times

it was by no means an optimization. Not reading the same byte 4 bytes is
there actually to stop doing the unaligned IO reads. You can't have (2)
without (1) unless you switch to ioreadb() and add a shift (which may
not be a bad idea.

>
>3. respecting __iomem and copying (I guess these are tied together)
>
>Seems to me that really should be at least three patches. Not
>necessarily in the above order.

(3) is actually the most important I think, so I will start by that.

>
>Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>have debugfs i915_vbt() handle that properly.

I don't think this is needed. The thing I'm doing here is the same as
what can be accomplished by reading the rom from sysfs:

find /sys/bus/pci/devices/*/ -name rom
... choose one

echo 1 > rom # to allow reading the rom
hexdump -C rom


>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>>  1 file changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 671bbce6ba5b..c401e90b7cf1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>>  	return vbt;
>>  }
>>
>> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>>  {
>> -	size_t i;
>> +	const u32 MAGIC = *((const u32 *)"$VBT");
>> +	size_t done = 0, cur = 0;
>> +	void __iomem *p;
>> +	u8 buf[128];
>> +	u32 val;
>>
>> -	/* Scour memory looking for the VBT signature. */
>> -	for (i = 0; i + 4 < size; i++) {
>> -		void *vbt;
>> +	/*
>> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
>> +	 * wrap-arounds and using u32 for comparison. This gives us 4
>> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
>> +	 * still does unaligned cpu access).
>> +	 */
>
>If we're really worried about performance here, and use a local buffer
>to optimize the wraparounds, would it actually be more efficient to use
>memcpy_fromio() which has an arch specific implementation in asm?

Not really worried about performance. I actually did 3 implementations
that avoids the unaligned io reads.

1) this one
2) memcpy_fromio() to the local buffer + strnstr()
3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
pointer to it. Then free the oprom after the vbt is used

(2) and (1) had basically the same complexity involved of requiring a
wrap around local buffer, so I went with (1)

I didn't feel confortable with (3) because it would allocate much more
memory than really needed.

>
>In any case makes you think you should first have the patch that the
>patch subject claims, fix unaligned reads and add optimizations
>next. This one does too much.

Again, it was not really meant to be an optimization.

Ville told me that we may not really need to deal with the unaligned
access and change the implementation to expect the VBT to be aligned.
This I would be the simplest way to change it, but I'm not fond on
changing this and breaking old systems usin it... anyway, we can give it
a try and revert if it breaks.

I will send a new version with the split.

Lucas De Marchi

>
>BR,
>Jani.
>
>
>
>> +	for (p = oprom; p < oprom + size; p += 4) {
>> +		*(u32 *)(&buf[done]) = ioread32(p);
>> +		done += 4;
>>
>> -		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>> -			continue;
>> +		while (cur + 4 <= done) {
>> +			val = *(u32 *)(buf + cur);
>> +			if (val == MAGIC)
>> +				return p - (done - cur) + 4;
>>
>> -		/*
>> -		 * This is the one place where we explicitly discard the address
>> -		 * space (__iomem) of the BIOS/VBT.
>> -		 */
>> -		vbt = (void __force *)oprom + i;
>> -		if (intel_bios_is_valid_vbt(vbt, size - i))
>> -			return vbt;
>> +			cur++;
>> +		}
>>
>> -		break;
>> +		/* wrap-around */
>> +		if (done + 4 >= sizeof(buf)) {
>> +			buf[0] = buf[done - 3];
>> +			buf[1] = buf[done - 2];
>> +			buf[2] = buf[done - 1];
>> +			cur = 0;
>> +			done = 3;
>> +		}
>>  	}
>>
>> +	/* Read the entire oprom and no VBT found */
>>  	return NULL;
>>  }
>>
>> +/*
>> + * Copy vbt to a new allocated buffer and update @psize to match the VBT
>> + * size
>> + */
>> +static struct vbt_header *copy_vbt(void __iomem *oprom, size_t *psize)
>> +{
>> +	off_t vbt_size_offset = offsetof(struct vbt_header, vbt_size);
>> +	struct vbt_header *vbt;
>> +	void __iomem *p;
>> +	u16 vbt_size;
>> +	size_t size;
>> +
>> +	size = *psize;
>> +	p = find_vbt(oprom, size);
>> +	if (!p)
>> +		return NULL;
>> +
>> +	size -= p - oprom;
>> +
>> +	/*
>> +	 * We need to at least be able to read the size and make sure it doesn't
>> +	 * overflow the oprom. The rest will be validated later.
>> +	 */
>> +	if (sizeof(*vbt) > size) {
>> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>> +		return NULL;
>> +	}
>> +
>> +	vbt_size = ioread16(p + vbt_size_offset);
>> +	if (vbt_size > size) {
>> +		DRM_DEBUG_DRIVER("VBT incomplete\n");
>> +		return NULL;
>> +	}
>> +
>> +	vbt = kmalloc(vbt_size, GFP_KERNEL);
>> +	memcpy_fromio(vbt, p, vbt_size);
>> +
>> +	*psize = vbt_size;
>> +
>> +	return vbt;
>> +}
>> +
>>  /**
>>   * intel_bios_init - find VBT and initialize settings from the BIOS
>>   * @dev_priv: i915 device instance
>> @@ -1861,10 +1918,13 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>  		if (!oprom)
>>  			goto out;
>>
>> -		vbt = find_vbt(oprom, size);
>> +		vbt = copy_vbt(oprom, &size);
>>  		if (!vbt)
>>  			goto out;
>>
>> +		if (!intel_bios_is_valid_vbt(vbt, size))
>> +			goto out;
>> +
>>  		DRM_DEBUG_KMS("Found valid VBT in PCI ROM\n");
>>  	}
>>
>> @@ -1897,6 +1957,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
>>
>>  	if (oprom)
>>  		pci_unmap_rom(pdev, oprom);
>> +
>> +	if (vbt != dev_priv->opregion.vbt)
>> +		kfree(vbt);
>>  }
>>
>>  /**
>
>-- 
>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 4/4] drm/i915/bios: do not discard address space

[Ville Syrjälä]

On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
> >> to simply discard the address space and go on reading the memory. After
> >> checking on my test system that dumping the rom via sysfs I could
> >> actually get the correct vbt, I decided to change the implementation to
> >> use the same approach, by calling memcpy_fromio().
> >>
> >> In order to avoid copying the entire oprom this implements a simple
> >> memmem() searching for "$VBT". Contrary to the previous implementation
> >> this also takes care of not issuing unaligned PCI reads that would
> >> otherwise get translated into more even more reads. I also vaguely
> >> remember unaligned reads failing in the past with some devices.
> >>
> >> Also make sure we copy only the VBT and not the entire oprom that is
> >> usually much larger.
> >
> >So you have
> >
> >1. a fix to unaligned reads
> 
> unaligned io reads, yes
> 
> >
> >2. an optimization to avoid reading individual bytes four times
> 
> it was by no means an optimization. Not reading the same byte 4 bytes is
> there actually to stop doing the unaligned IO reads. You can't have (2)
> without (1) unless you switch to ioreadb() and add a shift (which may
> not be a bad idea.
> 
> >
> >3. respecting __iomem and copying (I guess these are tied together)
> >
> >Seems to me that really should be at least three patches. Not
> >necessarily in the above order.
> 
> (3) is actually the most important I think, so I will start by that.
> 
> >
> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
> >have debugfs i915_vbt() handle that properly.
> 
> I don't think this is needed. The thing I'm doing here is the same as
> what can be accomplished by reading the rom from sysfs:
> 
> find /sys/bus/pci/devices/*/ -name rom
> ... choose one
> 
> echo 1 > rom # to allow reading the rom
> hexdump -C rom
> 
> 
> >
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
> >>  1 file changed, 79 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> index 671bbce6ba5b..c401e90b7cf1 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
> >>  	return vbt;
> >>  }
> >>
> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
> >>  {
> >> -	size_t i;
> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
> >> +	size_t done = 0, cur = 0;
> >> +	void __iomem *p;
> >> +	u8 buf[128];
> >> +	u32 val;
> >>
> >> -	/* Scour memory looking for the VBT signature. */
> >> -	for (i = 0; i + 4 < size; i++) {
> >> -		void *vbt;
> >> +	/*
> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
> >> +	 * still does unaligned cpu access).
> >> +	 */
> >
> >If we're really worried about performance here, and use a local buffer
> >to optimize the wraparounds, would it actually be more efficient to use
> >memcpy_fromio() which has an arch specific implementation in asm?
> 
> Not really worried about performance. I actually did 3 implementations
> that avoids the unaligned io reads.
> 
> 1) this one
> 2) memcpy_fromio() to the local buffer + strnstr()
> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
> pointer to it. Then free the oprom after the vbt is used
> 
> (2) and (1) had basically the same complexity involved of requiring a
> wrap around local buffer, so I went with (1)
> 
> I didn't feel confortable with (3) because it would allocate much more
> memory than really needed.
> 
> >
> >In any case makes you think you should first have the patch that the
> >patch subject claims, fix unaligned reads and add optimizations
> >next. This one does too much.
> 
> Again, it was not really meant to be an optimization.
> 
> Ville told me that we may not really need to deal with the unaligned
> access and change the implementation to expect the VBT to be aligned.
> This I would be the simplest way to change it, but I'm not fond on
> changing this and breaking old systems usin it... anyway, we can give it
> a try and revert if it breaks.

The current code already assumes 4 byte alignment. So nothing would
change and so nothing can get broken.

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Ville Syrjälä]

On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
> >> to simply discard the address space and go on reading the memory. After
> >> checking on my test system that dumping the rom via sysfs I could
> >> actually get the correct vbt, I decided to change the implementation to
> >> use the same approach, by calling memcpy_fromio().
> >>
> >> In order to avoid copying the entire oprom this implements a simple
> >> memmem() searching for "$VBT". Contrary to the previous implementation
> >> this also takes care of not issuing unaligned PCI reads that would
> >> otherwise get translated into more even more reads. I also vaguely
> >> remember unaligned reads failing in the past with some devices.
> >>
> >> Also make sure we copy only the VBT and not the entire oprom that is
> >> usually much larger.
> >
> >So you have
> >
> >1. a fix to unaligned reads
> 
> unaligned io reads, yes
> 
> >
> >2. an optimization to avoid reading individual bytes four times
> 
> it was by no means an optimization. Not reading the same byte 4 bytes is
> there actually to stop doing the unaligned IO reads. You can't have (2)
> without (1) unless you switch to ioreadb() and add a shift (which may
> not be a bad idea.
> 
> >
> >3. respecting __iomem and copying (I guess these are tied together)
> >
> >Seems to me that really should be at least three patches. Not
> >necessarily in the above order.
> 
> (3) is actually the most important I think, so I will start by that.
> 
> >
> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
> >have debugfs i915_vbt() handle that properly.
> 
> I don't think this is needed. The thing I'm doing here is the same as
> what can be accomplished by reading the rom from sysfs:
> 
> find /sys/bus/pci/devices/*/ -name rom
> ... choose one
> 
> echo 1 > rom # to allow reading the rom
> hexdump -C rom
> 
> 
> >
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
> >>  1 file changed, 79 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> index 671bbce6ba5b..c401e90b7cf1 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
> >>  	return vbt;
> >>  }
> >>
> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
> >>  {
> >> -	size_t i;
> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
> >> +	size_t done = 0, cur = 0;
> >> +	void __iomem *p;
> >> +	u8 buf[128];
> >> +	u32 val;
> >>
> >> -	/* Scour memory looking for the VBT signature. */
> >> -	for (i = 0; i + 4 < size; i++) {
> >> -		void *vbt;
> >> +	/*
> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
> >> +	 * still does unaligned cpu access).
> >> +	 */
> >
> >If we're really worried about performance here, and use a local buffer
> >to optimize the wraparounds, would it actually be more efficient to use
> >memcpy_fromio() which has an arch specific implementation in asm?
> 
> Not really worried about performance. I actually did 3 implementations
> that avoids the unaligned io reads.
> 
> 1) this one
> 2) memcpy_fromio() to the local buffer + strnstr()
> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
> pointer to it. Then free the oprom after the vbt is used
> 
> (2) and (1) had basically the same complexity involved of requiring a
> wrap around local buffer, so I went with (1)
> 
> I didn't feel confortable with (3) because it would allocate much more
> memory than really needed.
> 
> >
> >In any case makes you think you should first have the patch that the
> >patch subject claims, fix unaligned reads and add optimizations
> >next. This one does too much.
> 
> Again, it was not really meant to be an optimization.
> 
> Ville told me that we may not really need to deal with the unaligned
> access and change the implementation to expect the VBT to be aligned.
> This I would be the simplest way to change it, but I'm not fond on
> changing this and breaking old systems usin it... anyway, we can give it
> a try and revert if it breaks.

The current code already assumes 4 byte alignment. So nothing would
change and so nothing can get broken.

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

Re: [PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 09:19:15PM +0200, Ville Syrjälä wrote:
>On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
>> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
>> >> to simply discard the address space and go on reading the memory. After
>> >> checking on my test system that dumping the rom via sysfs I could
>> >> actually get the correct vbt, I decided to change the implementation to
>> >> use the same approach, by calling memcpy_fromio().
>> >>
>> >> In order to avoid copying the entire oprom this implements a simple
>> >> memmem() searching for "$VBT". Contrary to the previous implementation
>> >> this also takes care of not issuing unaligned PCI reads that would
>> >> otherwise get translated into more even more reads. I also vaguely
>> >> remember unaligned reads failing in the past with some devices.
>> >>
>> >> Also make sure we copy only the VBT and not the entire oprom that is
>> >> usually much larger.
>> >
>> >So you have
>> >
>> >1. a fix to unaligned reads
>>
>> unaligned io reads, yes
>>
>> >
>> >2. an optimization to avoid reading individual bytes four times
>>
>> it was by no means an optimization. Not reading the same byte 4 bytes is
>> there actually to stop doing the unaligned IO reads. You can't have (2)
>> without (1) unless you switch to ioreadb() and add a shift (which may
>> not be a bad idea.
>>
>> >
>> >3. respecting __iomem and copying (I guess these are tied together)
>> >
>> >Seems to me that really should be at least three patches. Not
>> >necessarily in the above order.
>>
>> (3) is actually the most important I think, so I will start by that.
>>
>> >
>> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>> >have debugfs i915_vbt() handle that properly.
>>
>> I don't think this is needed. The thing I'm doing here is the same as
>> what can be accomplished by reading the rom from sysfs:
>>
>> find /sys/bus/pci/devices/*/ -name rom
>> ... choose one
>>
>> echo 1 > rom # to allow reading the rom
>> hexdump -C rom
>>
>>
>> >
>> >>
>> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>> >>  1 file changed, 79 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> index 671bbce6ba5b..c401e90b7cf1 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>> >>  	return vbt;
>> >>  }
>> >>
>> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>> >>  {
>> >> -	size_t i;
>> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
>> >> +	size_t done = 0, cur = 0;
>> >> +	void __iomem *p;
>> >> +	u8 buf[128];
>> >> +	u32 val;
>> >>
>> >> -	/* Scour memory looking for the VBT signature. */
>> >> -	for (i = 0; i + 4 < size; i++) {
>> >> -		void *vbt;
>> >> +	/*
>> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
>> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
>> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
>> >> +	 * still does unaligned cpu access).
>> >> +	 */
>> >
>> >If we're really worried about performance here, and use a local buffer
>> >to optimize the wraparounds, would it actually be more efficient to use
>> >memcpy_fromio() which has an arch specific implementation in asm?
>>
>> Not really worried about performance. I actually did 3 implementations
>> that avoids the unaligned io reads.
>>
>> 1) this one
>> 2) memcpy_fromio() to the local buffer + strnstr()
>> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
>> pointer to it. Then free the oprom after the vbt is used
>>
>> (2) and (1) had basically the same complexity involved of requiring a
>> wrap around local buffer, so I went with (1)
>>
>> I didn't feel confortable with (3) because it would allocate much more
>> memory than really needed.
>>
>> >
>> >In any case makes you think you should first have the patch that the
>> >patch subject claims, fix unaligned reads and add optimizations
>> >next. This one does too much.
>>
>> Again, it was not really meant to be an optimization.
>>
>> Ville told me that we may not really need to deal with the unaligned
>> access and change the implementation to expect the VBT to be aligned.
>> This I would be the simplest way to change it, but I'm not fond on
>> changing this and breaking old systems usin it... anyway, we can give it
>> a try and revert if it breaks.
>
>The current code already assumes 4 byte alignment. So nothing would
>change and so nothing can get broken.

the code for reading the oprom via pci is not assuming 4-byte alignment.
See above, it's doing

	for (i = 0; i + 4 < size; i++)

It might as well be using a ioread8() + a shift and it would be the
same, since it only advances 1 byte per loop.

Saying that from acpi it needs to be aligned so the oprom should be
aligned IMO is not valid because the pci method was there before the
acpi one.  I don't know exactly what I may be breaking if I switch to
4-byte alignment.

Anyway, my new patch splits the changes, as requested by Jani.
Just enforcing we don't ignore the address space already fixes my
issue. So maybe we can leave the alignment alone and not touch it.

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 09:19:15PM +0200, Ville Syrjälä wrote:
>On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
>> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
>> >> to simply discard the address space and go on reading the memory. After
>> >> checking on my test system that dumping the rom via sysfs I could
>> >> actually get the correct vbt, I decided to change the implementation to
>> >> use the same approach, by calling memcpy_fromio().
>> >>
>> >> In order to avoid copying the entire oprom this implements a simple
>> >> memmem() searching for "$VBT". Contrary to the previous implementation
>> >> this also takes care of not issuing unaligned PCI reads that would
>> >> otherwise get translated into more even more reads. I also vaguely
>> >> remember unaligned reads failing in the past with some devices.
>> >>
>> >> Also make sure we copy only the VBT and not the entire oprom that is
>> >> usually much larger.
>> >
>> >So you have
>> >
>> >1. a fix to unaligned reads
>>
>> unaligned io reads, yes
>>
>> >
>> >2. an optimization to avoid reading individual bytes four times
>>
>> it was by no means an optimization. Not reading the same byte 4 bytes is
>> there actually to stop doing the unaligned IO reads. You can't have (2)
>> without (1) unless you switch to ioreadb() and add a shift (which may
>> not be a bad idea.
>>
>> >
>> >3. respecting __iomem and copying (I guess these are tied together)
>> >
>> >Seems to me that really should be at least three patches. Not
>> >necessarily in the above order.
>>
>> (3) is actually the most important I think, so I will start by that.
>>
>> >
>> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>> >have debugfs i915_vbt() handle that properly.
>>
>> I don't think this is needed. The thing I'm doing here is the same as
>> what can be accomplished by reading the rom from sysfs:
>>
>> find /sys/bus/pci/devices/*/ -name rom
>> ... choose one
>>
>> echo 1 > rom # to allow reading the rom
>> hexdump -C rom
>>
>>
>> >
>> >>
>> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>> >>  1 file changed, 79 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> index 671bbce6ba5b..c401e90b7cf1 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>> >>  	return vbt;
>> >>  }
>> >>
>> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>> >>  {
>> >> -	size_t i;
>> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
>> >> +	size_t done = 0, cur = 0;
>> >> +	void __iomem *p;
>> >> +	u8 buf[128];
>> >> +	u32 val;
>> >>
>> >> -	/* Scour memory looking for the VBT signature. */
>> >> -	for (i = 0; i + 4 < size; i++) {
>> >> -		void *vbt;
>> >> +	/*
>> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
>> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
>> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
>> >> +	 * still does unaligned cpu access).
>> >> +	 */
>> >
>> >If we're really worried about performance here, and use a local buffer
>> >to optimize the wraparounds, would it actually be more efficient to use
>> >memcpy_fromio() which has an arch specific implementation in asm?
>>
>> Not really worried about performance. I actually did 3 implementations
>> that avoids the unaligned io reads.
>>
>> 1) this one
>> 2) memcpy_fromio() to the local buffer + strnstr()
>> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
>> pointer to it. Then free the oprom after the vbt is used
>>
>> (2) and (1) had basically the same complexity involved of requiring a
>> wrap around local buffer, so I went with (1)
>>
>> I didn't feel confortable with (3) because it would allocate much more
>> memory than really needed.
>>
>> >
>> >In any case makes you think you should first have the patch that the
>> >patch subject claims, fix unaligned reads and add optimizations
>> >next. This one does too much.
>>
>> Again, it was not really meant to be an optimization.
>>
>> Ville told me that we may not really need to deal with the unaligned
>> access and change the implementation to expect the VBT to be aligned.
>> This I would be the simplest way to change it, but I'm not fond on
>> changing this and breaking old systems usin it... anyway, we can give it
>> a try and revert if it breaks.
>
>The current code already assumes 4 byte alignment. So nothing would
>change and so nothing can get broken.

the code for reading the oprom via pci is not assuming 4-byte alignment.
See above, it's doing

	for (i = 0; i + 4 < size; i++)

It might as well be using a ioread8() + a shift and it would be the
same, since it only advances 1 byte per loop.

Saying that from acpi it needs to be aligned so the oprom should be
aligned IMO is not valid because the pci method was there before the
acpi one.  I don't know exactly what I may be breaking if I switch to
4-byte alignment.

Anyway, my new patch splits the changes, as requested by Jani.
Just enforcing we don't ignore the address space already fixes my
issue. So maybe we can leave the alignment alone and not touch it.

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

Re: [PATCH 4/4] drm/i915/bios: do not discard address space

[Ville Syrjälä]

On Fri, Nov 08, 2019 at 12:14:05PM -0800, Lucas De Marchi wrote:
> On Fri, Nov 08, 2019 at 09:19:15PM +0200, Ville Syrjälä wrote:
> >On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
> >> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
> >> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
> >> >> to simply discard the address space and go on reading the memory. After
> >> >> checking on my test system that dumping the rom via sysfs I could
> >> >> actually get the correct vbt, I decided to change the implementation to
> >> >> use the same approach, by calling memcpy_fromio().
> >> >>
> >> >> In order to avoid copying the entire oprom this implements a simple
> >> >> memmem() searching for "$VBT". Contrary to the previous implementation
> >> >> this also takes care of not issuing unaligned PCI reads that would
> >> >> otherwise get translated into more even more reads. I also vaguely
> >> >> remember unaligned reads failing in the past with some devices.
> >> >>
> >> >> Also make sure we copy only the VBT and not the entire oprom that is
> >> >> usually much larger.
> >> >
> >> >So you have
> >> >
> >> >1. a fix to unaligned reads
> >>
> >> unaligned io reads, yes
> >>
> >> >
> >> >2. an optimization to avoid reading individual bytes four times
> >>
> >> it was by no means an optimization. Not reading the same byte 4 bytes is
> >> there actually to stop doing the unaligned IO reads. You can't have (2)
> >> without (1) unless you switch to ioreadb() and add a shift (which may
> >> not be a bad idea.
> >>
> >> >
> >> >3. respecting __iomem and copying (I guess these are tied together)
> >> >
> >> >Seems to me that really should be at least three patches. Not
> >> >necessarily in the above order.
> >>
> >> (3) is actually the most important I think, so I will start by that.
> >>
> >> >
> >> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
> >> >have debugfs i915_vbt() handle that properly.
> >>
> >> I don't think this is needed. The thing I'm doing here is the same as
> >> what can be accomplished by reading the rom from sysfs:
> >>
> >> find /sys/bus/pci/devices/*/ -name rom
> >> ... choose one
> >>
> >> echo 1 > rom # to allow reading the rom
> >> hexdump -C rom
> >>
> >>
> >> >
> >> >>
> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
> >> >>  1 file changed, 79 insertions(+), 16 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> index 671bbce6ba5b..c401e90b7cf1 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
> >> >>  	return vbt;
> >> >>  }
> >> >>
> >> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> >> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
> >> >>  {
> >> >> -	size_t i;
> >> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
> >> >> +	size_t done = 0, cur = 0;
> >> >> +	void __iomem *p;
> >> >> +	u8 buf[128];
> >> >> +	u32 val;
> >> >>
> >> >> -	/* Scour memory looking for the VBT signature. */
> >> >> -	for (i = 0; i + 4 < size; i++) {
> >> >> -		void *vbt;
> >> >> +	/*
> >> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
> >> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
> >> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
> >> >> +	 * still does unaligned cpu access).
> >> >> +	 */
> >> >
> >> >If we're really worried about performance here, and use a local buffer
> >> >to optimize the wraparounds, would it actually be more efficient to use
> >> >memcpy_fromio() which has an arch specific implementation in asm?
> >>
> >> Not really worried about performance. I actually did 3 implementations
> >> that avoids the unaligned io reads.
> >>
> >> 1) this one
> >> 2) memcpy_fromio() to the local buffer + strnstr()
> >> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
> >> pointer to it. Then free the oprom after the vbt is used
> >>
> >> (2) and (1) had basically the same complexity involved of requiring a
> >> wrap around local buffer, so I went with (1)
> >>
> >> I didn't feel confortable with (3) because it would allocate much more
> >> memory than really needed.
> >>
> >> >
> >> >In any case makes you think you should first have the patch that the
> >> >patch subject claims, fix unaligned reads and add optimizations
> >> >next. This one does too much.
> >>
> >> Again, it was not really meant to be an optimization.
> >>
> >> Ville told me that we may not really need to deal with the unaligned
> >> access and change the implementation to expect the VBT to be aligned.
> >> This I would be the simplest way to change it, but I'm not fond on
> >> changing this and breaking old systems usin it... anyway, we can give it
> >> a try and revert if it breaks.
> >
> >The current code already assumes 4 byte alignment. So nothing would
> >change and so nothing can get broken.
> 
> the code for reading the oprom via pci is not assuming 4-byte alignment.
> See above, it's doing
> 
> 	for (i = 0; i + 4 < size; i++)
> 
> It might as well be using a ioread8() + a shift and it would be the
> same, since it only advances 1 byte per loop.

Hmm, indeed you are correct. I guess the +4 tricked me into thinking
otherwise.

> 
> Saying that from acpi it needs to be aligned so the oprom should be
> aligned IMO is not valid because the pci method was there before the
> acpi one.  I don't know exactly what I may be breaking if I switch to
> 4-byte alignment.
> 
> Anyway, my new patch splits the changes, as requested by Jani.
> Just enforcing we don't ignore the address space already fixes my
> issue. So maybe we can leave the alignment alone and not touch it.

I suppose we can stick to the i++ if the code can be kept simple
enough. But I'm totally willing to put my name on a i+=4 patch
if the complexity starts to rise alarmingly.

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Ville Syrjälä]

On Fri, Nov 08, 2019 at 12:14:05PM -0800, Lucas De Marchi wrote:
> On Fri, Nov 08, 2019 at 09:19:15PM +0200, Ville Syrjälä wrote:
> >On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
> >> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
> >> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
> >> >> to simply discard the address space and go on reading the memory. After
> >> >> checking on my test system that dumping the rom via sysfs I could
> >> >> actually get the correct vbt, I decided to change the implementation to
> >> >> use the same approach, by calling memcpy_fromio().
> >> >>
> >> >> In order to avoid copying the entire oprom this implements a simple
> >> >> memmem() searching for "$VBT". Contrary to the previous implementation
> >> >> this also takes care of not issuing unaligned PCI reads that would
> >> >> otherwise get translated into more even more reads. I also vaguely
> >> >> remember unaligned reads failing in the past with some devices.
> >> >>
> >> >> Also make sure we copy only the VBT and not the entire oprom that is
> >> >> usually much larger.
> >> >
> >> >So you have
> >> >
> >> >1. a fix to unaligned reads
> >>
> >> unaligned io reads, yes
> >>
> >> >
> >> >2. an optimization to avoid reading individual bytes four times
> >>
> >> it was by no means an optimization. Not reading the same byte 4 bytes is
> >> there actually to stop doing the unaligned IO reads. You can't have (2)
> >> without (1) unless you switch to ioreadb() and add a shift (which may
> >> not be a bad idea.
> >>
> >> >
> >> >3. respecting __iomem and copying (I guess these are tied together)
> >> >
> >> >Seems to me that really should be at least three patches. Not
> >> >necessarily in the above order.
> >>
> >> (3) is actually the most important I think, so I will start by that.
> >>
> >> >
> >> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
> >> >have debugfs i915_vbt() handle that properly.
> >>
> >> I don't think this is needed. The thing I'm doing here is the same as
> >> what can be accomplished by reading the rom from sysfs:
> >>
> >> find /sys/bus/pci/devices/*/ -name rom
> >> ... choose one
> >>
> >> echo 1 > rom # to allow reading the rom
> >> hexdump -C rom
> >>
> >>
> >> >
> >> >>
> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
> >> >>  1 file changed, 79 insertions(+), 16 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> index 671bbce6ba5b..c401e90b7cf1 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
> >> >>  	return vbt;
> >> >>  }
> >> >>
> >> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
> >> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
> >> >>  {
> >> >> -	size_t i;
> >> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
> >> >> +	size_t done = 0, cur = 0;
> >> >> +	void __iomem *p;
> >> >> +	u8 buf[128];
> >> >> +	u32 val;
> >> >>
> >> >> -	/* Scour memory looking for the VBT signature. */
> >> >> -	for (i = 0; i + 4 < size; i++) {
> >> >> -		void *vbt;
> >> >> +	/*
> >> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
> >> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
> >> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
> >> >> +	 * still does unaligned cpu access).
> >> >> +	 */
> >> >
> >> >If we're really worried about performance here, and use a local buffer
> >> >to optimize the wraparounds, would it actually be more efficient to use
> >> >memcpy_fromio() which has an arch specific implementation in asm?
> >>
> >> Not really worried about performance. I actually did 3 implementations
> >> that avoids the unaligned io reads.
> >>
> >> 1) this one
> >> 2) memcpy_fromio() to the local buffer + strnstr()
> >> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
> >> pointer to it. Then free the oprom after the vbt is used
> >>
> >> (2) and (1) had basically the same complexity involved of requiring a
> >> wrap around local buffer, so I went with (1)
> >>
> >> I didn't feel confortable with (3) because it would allocate much more
> >> memory than really needed.
> >>
> >> >
> >> >In any case makes you think you should first have the patch that the
> >> >patch subject claims, fix unaligned reads and add optimizations
> >> >next. This one does too much.
> >>
> >> Again, it was not really meant to be an optimization.
> >>
> >> Ville told me that we may not really need to deal with the unaligned
> >> access and change the implementation to expect the VBT to be aligned.
> >> This I would be the simplest way to change it, but I'm not fond on
> >> changing this and breaking old systems usin it... anyway, we can give it
> >> a try and revert if it breaks.
> >
> >The current code already assumes 4 byte alignment. So nothing would
> >change and so nothing can get broken.
> 
> the code for reading the oprom via pci is not assuming 4-byte alignment.
> See above, it's doing
> 
> 	for (i = 0; i + 4 < size; i++)
> 
> It might as well be using a ioread8() + a shift and it would be the
> same, since it only advances 1 byte per loop.

Hmm, indeed you are correct. I guess the +4 tricked me into thinking
otherwise.

> 
> Saying that from acpi it needs to be aligned so the oprom should be
> aligned IMO is not valid because the pci method was there before the
> acpi one.  I don't know exactly what I may be breaking if I switch to
> 4-byte alignment.
> 
> Anyway, my new patch splits the changes, as requested by Jani.
> Just enforcing we don't ignore the address space already fixes my
> issue. So maybe we can leave the alignment alone and not touch it.

I suppose we can stick to the i++ if the code can be kept simple
enough. But I'm totally willing to put my name on a i+=4 patch
if the complexity starts to rise alarmingly.

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

Re: [PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 11:02:46PM +0200, Ville Syrjälä wrote:
>On Fri, Nov 08, 2019 at 12:14:05PM -0800, Lucas De Marchi wrote:
>> On Fri, Nov 08, 2019 at 09:19:15PM +0200, Ville Syrjälä wrote:
>> >On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
>> >> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>> >> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
>> >> >> to simply discard the address space and go on reading the memory. After
>> >> >> checking on my test system that dumping the rom via sysfs I could
>> >> >> actually get the correct vbt, I decided to change the implementation to
>> >> >> use the same approach, by calling memcpy_fromio().
>> >> >>
>> >> >> In order to avoid copying the entire oprom this implements a simple
>> >> >> memmem() searching for "$VBT". Contrary to the previous implementation
>> >> >> this also takes care of not issuing unaligned PCI reads that would
>> >> >> otherwise get translated into more even more reads. I also vaguely
>> >> >> remember unaligned reads failing in the past with some devices.
>> >> >>
>> >> >> Also make sure we copy only the VBT and not the entire oprom that is
>> >> >> usually much larger.
>> >> >
>> >> >So you have
>> >> >
>> >> >1. a fix to unaligned reads
>> >>
>> >> unaligned io reads, yes
>> >>
>> >> >
>> >> >2. an optimization to avoid reading individual bytes four times
>> >>
>> >> it was by no means an optimization. Not reading the same byte 4 bytes is
>> >> there actually to stop doing the unaligned IO reads. You can't have (2)
>> >> without (1) unless you switch to ioreadb() and add a shift (which may
>> >> not be a bad idea.
>> >>
>> >> >
>> >> >3. respecting __iomem and copying (I guess these are tied together)
>> >> >
>> >> >Seems to me that really should be at least three patches. Not
>> >> >necessarily in the above order.
>> >>
>> >> (3) is actually the most important I think, so I will start by that.
>> >>
>> >> >
>> >> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>> >> >have debugfs i915_vbt() handle that properly.
>> >>
>> >> I don't think this is needed. The thing I'm doing here is the same as
>> >> what can be accomplished by reading the rom from sysfs:
>> >>
>> >> find /sys/bus/pci/devices/*/ -name rom
>> >> ... choose one
>> >>
>> >> echo 1 > rom # to allow reading the rom
>> >> hexdump -C rom
>> >>
>> >>
>> >> >
>> >> >>
>> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>> >> >>  1 file changed, 79 insertions(+), 16 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> >> index 671bbce6ba5b..c401e90b7cf1 100644
>> >> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>> >> >>  	return vbt;
>> >> >>  }
>> >> >>
>> >> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> >> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>> >> >>  {
>> >> >> -	size_t i;
>> >> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
>> >> >> +	size_t done = 0, cur = 0;
>> >> >> +	void __iomem *p;
>> >> >> +	u8 buf[128];
>> >> >> +	u32 val;
>> >> >>
>> >> >> -	/* Scour memory looking for the VBT signature. */
>> >> >> -	for (i = 0; i + 4 < size; i++) {
>> >> >> -		void *vbt;
>> >> >> +	/*
>> >> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
>> >> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
>> >> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
>> >> >> +	 * still does unaligned cpu access).
>> >> >> +	 */
>> >> >
>> >> >If we're really worried about performance here, and use a local buffer
>> >> >to optimize the wraparounds, would it actually be more efficient to use
>> >> >memcpy_fromio() which has an arch specific implementation in asm?
>> >>
>> >> Not really worried about performance. I actually did 3 implementations
>> >> that avoids the unaligned io reads.
>> >>
>> >> 1) this one
>> >> 2) memcpy_fromio() to the local buffer + strnstr()
>> >> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
>> >> pointer to it. Then free the oprom after the vbt is used
>> >>
>> >> (2) and (1) had basically the same complexity involved of requiring a
>> >> wrap around local buffer, so I went with (1)
>> >>
>> >> I didn't feel confortable with (3) because it would allocate much more
>> >> memory than really needed.
>> >>
>> >> >
>> >> >In any case makes you think you should first have the patch that the
>> >> >patch subject claims, fix unaligned reads and add optimizations
>> >> >next. This one does too much.
>> >>
>> >> Again, it was not really meant to be an optimization.
>> >>
>> >> Ville told me that we may not really need to deal with the unaligned
>> >> access and change the implementation to expect the VBT to be aligned.
>> >> This I would be the simplest way to change it, but I'm not fond on
>> >> changing this and breaking old systems usin it... anyway, we can give it
>> >> a try and revert if it breaks.
>> >
>> >The current code already assumes 4 byte alignment. So nothing would
>> >change and so nothing can get broken.
>>
>> the code for reading the oprom via pci is not assuming 4-byte alignment.
>> See above, it's doing
>>
>> 	for (i = 0; i + 4 < size; i++)
>>
>> It might as well be using a ioread8() + a shift and it would be the
>> same, since it only advances 1 byte per loop.
>
>Hmm, indeed you are correct. I guess the +4 tricked me into thinking
>otherwise.

it tricked me too, it took some time to realize that it was advancing
just one byte... maybe that was when I decided: ok, let me reimplement
this rather than focusing on the minimum viable fix.

>
>>
>> Saying that from acpi it needs to be aligned so the oprom should be
>> aligned IMO is not valid because the pci method was there before the
>> acpi one.  I don't know exactly what I may be breaking if I switch to
>> 4-byte alignment.
>>
>> Anyway, my new patch splits the changes, as requested by Jani.
>> Just enforcing we don't ignore the address space already fixes my
>> issue. So maybe we can leave the alignment alone and not touch it.
>
>I suppose we can stick to the i++ if the code can be kept simple
>enough. But I'm totally willing to put my name on a i+=4 patch
>if the complexity starts to rise alarmingly.

yep, we can try that on top.

thanks
Lucas De Marchi

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 11:02:46PM +0200, Ville Syrjälä wrote:
>On Fri, Nov 08, 2019 at 12:14:05PM -0800, Lucas De Marchi wrote:
>> On Fri, Nov 08, 2019 at 09:19:15PM +0200, Ville Syrjälä wrote:
>> >On Fri, Nov 08, 2019 at 10:18:52AM -0800, Lucas De Marchi wrote:
>> >> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>> >> >On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >> >> When we are mapping the VBT through pci_map_rom() we may not be allowed
>> >> >> to simply discard the address space and go on reading the memory. After
>> >> >> checking on my test system that dumping the rom via sysfs I could
>> >> >> actually get the correct vbt, I decided to change the implementation to
>> >> >> use the same approach, by calling memcpy_fromio().
>> >> >>
>> >> >> In order to avoid copying the entire oprom this implements a simple
>> >> >> memmem() searching for "$VBT". Contrary to the previous implementation
>> >> >> this also takes care of not issuing unaligned PCI reads that would
>> >> >> otherwise get translated into more even more reads. I also vaguely
>> >> >> remember unaligned reads failing in the past with some devices.
>> >> >>
>> >> >> Also make sure we copy only the VBT and not the entire oprom that is
>> >> >> usually much larger.
>> >> >
>> >> >So you have
>> >> >
>> >> >1. a fix to unaligned reads
>> >>
>> >> unaligned io reads, yes
>> >>
>> >> >
>> >> >2. an optimization to avoid reading individual bytes four times
>> >>
>> >> it was by no means an optimization. Not reading the same byte 4 bytes is
>> >> there actually to stop doing the unaligned IO reads. You can't have (2)
>> >> without (1) unless you switch to ioreadb() and add a shift (which may
>> >> not be a bad idea.
>> >>
>> >> >
>> >> >3. respecting __iomem and copying (I guess these are tied together)
>> >> >
>> >> >Seems to me that really should be at least three patches. Not
>> >> >necessarily in the above order.
>> >>
>> >> (3) is actually the most important I think, so I will start by that.
>> >>
>> >> >
>> >> >Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>> >> >have debugfs i915_vbt() handle that properly.
>> >>
>> >> I don't think this is needed. The thing I'm doing here is the same as
>> >> what can be accomplished by reading the rom from sysfs:
>> >>
>> >> find /sys/bus/pci/devices/*/ -name rom
>> >> ... choose one
>> >>
>> >> echo 1 > rom # to allow reading the rom
>> >> hexdump -C rom
>> >>
>> >>
>> >> >
>> >> >>
>> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/i915/display/intel_bios.c | 95 +++++++++++++++++++----
>> >> >>  1 file changed, 79 insertions(+), 16 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> >> index 671bbce6ba5b..c401e90b7cf1 100644
>> >> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> >> @@ -1806,31 +1806,88 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
>> >> >>  	return vbt;
>> >> >>  }
>> >> >>
>> >> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size)
>> >> >> +void __iomem *find_vbt(void __iomem *oprom, size_t size)
>> >> >>  {
>> >> >> -	size_t i;
>> >> >> +	const u32 MAGIC = *((const u32 *)"$VBT");
>> >> >> +	size_t done = 0, cur = 0;
>> >> >> +	void __iomem *p;
>> >> >> +	u8 buf[128];
>> >> >> +	u32 val;
>> >> >>
>> >> >> -	/* Scour memory looking for the VBT signature. */
>> >> >> -	for (i = 0; i + 4 < size; i++) {
>> >> >> -		void *vbt;
>> >> >> +	/*
>> >> >> +	 * poor's man memmem() with sizeof(buf) window to avoid frequent
>> >> >> +	 * wrap-arounds and using u32 for comparison. This gives us 4
>> >> >> +	 * comparisons per ioread32() and avoids unaligned io reads (although it
>> >> >> +	 * still does unaligned cpu access).
>> >> >> +	 */
>> >> >
>> >> >If we're really worried about performance here, and use a local buffer
>> >> >to optimize the wraparounds, would it actually be more efficient to use
>> >> >memcpy_fromio() which has an arch specific implementation in asm?
>> >>
>> >> Not really worried about performance. I actually did 3 implementations
>> >> that avoids the unaligned io reads.
>> >>
>> >> 1) this one
>> >> 2) memcpy_fromio() to the local buffer + strnstr()
>> >> 3) allocate a oprom buffer, memcpy_fromio() the entire rom and keep a
>> >> pointer to it. Then free the oprom after the vbt is used
>> >>
>> >> (2) and (1) had basically the same complexity involved of requiring a
>> >> wrap around local buffer, so I went with (1)
>> >>
>> >> I didn't feel confortable with (3) because it would allocate much more
>> >> memory than really needed.
>> >>
>> >> >
>> >> >In any case makes you think you should first have the patch that the
>> >> >patch subject claims, fix unaligned reads and add optimizations
>> >> >next. This one does too much.
>> >>
>> >> Again, it was not really meant to be an optimization.
>> >>
>> >> Ville told me that we may not really need to deal with the unaligned
>> >> access and change the implementation to expect the VBT to be aligned.
>> >> This I would be the simplest way to change it, but I'm not fond on
>> >> changing this and breaking old systems usin it... anyway, we can give it
>> >> a try and revert if it breaks.
>> >
>> >The current code already assumes 4 byte alignment. So nothing would
>> >change and so nothing can get broken.
>>
>> the code for reading the oprom via pci is not assuming 4-byte alignment.
>> See above, it's doing
>>
>> 	for (i = 0; i + 4 < size; i++)
>>
>> It might as well be using a ioread8() + a shift and it would be the
>> same, since it only advances 1 byte per loop.
>
>Hmm, indeed you are correct. I guess the +4 tricked me into thinking
>otherwise.

it tricked me too, it took some time to realize that it was advancing
just one byte... maybe that was when I decided: ok, let me reimplement
this rather than focusing on the minimum viable fix.

>
>>
>> Saying that from acpi it needs to be aligned so the oprom should be
>> aligned IMO is not valid because the pci method was there before the
>> acpi one.  I don't know exactly what I may be breaking if I switch to
>> 4-byte alignment.
>>
>> Anyway, my new patch splits the changes, as requested by Jani.
>> Just enforcing we don't ignore the address space already fixes my
>> issue. So maybe we can leave the alignment alone and not touch it.
>
>I suppose we can stick to the i++ if the code can be kept simple
>enough. But I'm totally willing to put my name on a i+=4 patch
>if the complexity starts to rise alarmingly.

yep, we can try that on top.

thanks
Lucas De Marchi

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

Re: [PATCH 4/4] drm/i915/bios: do not discard address space

[Jani Nikula]

On Fri, 08 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>>Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>>have debugfs i915_vbt() handle that properly.
>
> I don't think this is needed. The thing I'm doing here is the same as
> what can be accomplished by reading the rom from sysfs:
>
> find /sys/bus/pci/devices/*/ -name rom
> ... choose one
>
> echo 1 > rom # to allow reading the rom
> hexdump -C rom

I think it'll eventually be needed because using debugfs i915_vbt is how
we tell people to dump the VBT for debugging, and that's what they're
accustomed to. (And yeah, we're missing that for pre-opregion machines.)

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[Jani Nikula]

On Fri, 08 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Fri, Nov 08, 2019 at 01:14:03PM +0200, Jani Nikula wrote:
>>Follow-up: store pointer to the oprom vbt somewhere under i915->vbt, and
>>have debugfs i915_vbt() handle that properly.
>
> I don't think this is needed. The thing I'm doing here is the same as
> what can be accomplished by reading the rom from sysfs:
>
> find /sys/bus/pci/devices/*/ -name rom
> ... choose one
>
> echo 1 > rom # to allow reading the rom
> hexdump -C rom

I think it'll eventually be needed because using debugfs i915_vbt is how
we tell people to dump the VBT for debugging, and that's what they're
accustomed to. (And yeah, we're missing that for pre-opregion machines.)

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

Re: [PATCH 4/4] drm/i915/bios: do not discard address space

[kbuild test robot]

Hi Lucas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v5.4-rc6 next-20191108]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/drm-i915-opregion-fix-leaking-fw-on-error-path/20191110-000822
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-21-gb31adac-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/display/intel_bios.c:1809:14: sparse: sparse: symbol 'find_vbt' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

Hi Lucas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v5.4-rc6 next-20191108]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/drm-i915-opregion-fix-leaking-fw-on-error-path/20191110-000822
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-21-gb31adac-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/display/intel_bios.c:1809:14: sparse: sparse: symbol 'find_vbt' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

Re: [Intel-gfx] [PATCH 4/4] drm/i915/bios: do not discard address space

[kbuild test robot]

Hi Lucas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v5.4-rc6 next-20191108]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/drm-i915-opregion-fix-leaking-fw-on-error-path/20191110-000822
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-21-gb31adac-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/display/intel_bios.c:1809:14: sparse: sparse: symbol 'find_vbt' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH] drm/i915/bios: find_vbt() can be static

[kbuild test robot]

Fixes: bee6eeb3ee67 ("drm/i915/bios: do not discard address space")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 intel_bios.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 8dae8eb90d0da..0cd039217d06f 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1806,7 +1806,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-void __iomem *find_vbt(void __iomem *oprom, size_t size)
+static void __iomem *find_vbt(void __iomem *oprom, size_t size)
 {
 	const u32 MAGIC = *((const u32 *)"$VBT");
 	size_t done = 0, cur = 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH] drm/i915/bios: find_vbt() can be static

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]


Fixes: bee6eeb3ee67 ("drm/i915/bios: do not discard address space")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 intel_bios.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 8dae8eb90d0da..0cd039217d06f 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1806,7 +1806,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-void __iomem *find_vbt(void __iomem *oprom, size_t size)
+static void __iomem *find_vbt(void __iomem *oprom, size_t size)
 {
 	const u32 MAGIC = *((const u32 *)"$VBT");
 	size_t done = 0, cur = 0;

[Intel-gfx] [RFC PATCH] drm/i915/bios: find_vbt() can be static

[kbuild test robot]

Fixes: bee6eeb3ee67 ("drm/i915/bios: do not discard address space")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 intel_bios.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 8dae8eb90d0da..0cd039217d06f 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1806,7 +1806,7 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size)
 	return vbt;
 }
 
-void __iomem *find_vbt(void __iomem *oprom, size_t size)
+static void __iomem *find_vbt(void __iomem *oprom, size_t size)
 {
 	const u32 MAGIC = *((const u32 *)"$VBT");
 	size_t done = 0, cur = 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/opregion: fix leaking fw on error path

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/opregion: fix leaking fw on error path
URL   : https://patchwork.freedesktop.org/series/69167/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915/opregion: fix leaking fw on error path
Okay!

Commit: drm/i915/bios: rename bios to oprom when mapping pci rom
Okay!

Commit: drm/i915/bios: make sure to check vbt size
Okay!

Commit: drm/i915/bios: do not discard address space
-
+drivers/gpu/drm/i915/display/intel_bios.c:1809:14: warning: symbol 'find_vbt' was not declared. Should it be static?

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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/opregion: fix leaking fw on error path

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/opregion: fix leaking fw on error path
URL   : https://patchwork.freedesktop.org/series/69167/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915/opregion: fix leaking fw on error path
Okay!

Commit: drm/i915/bios: rename bios to oprom when mapping pci rom
Okay!

Commit: drm/i915/bios: make sure to check vbt size
Okay!

Commit: drm/i915/bios: do not discard address space
-
+drivers/gpu/drm/i915/display/intel_bios.c:1809:14: warning: symbol 'find_vbt' was not declared. Should it be static?

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915/opregion: fix leaking fw on error path

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/opregion: fix leaking fw on error path
URL   : https://patchwork.freedesktop.org/series/69167/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7290 -> Patchwork_15190
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/index.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [PASS][1] -> [FAIL][2] ([fdo#109483])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@gem_exec_create@basic:
    - {fi-tgl-u}:         [INCOMPLETE][3] ([fdo#111736]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/fi-tgl-u/igt@gem_exec_create@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/fi-tgl-u/igt@gem_exec_create@basic.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-cml-s:           [DMESG-WARN][5] ([fdo#111764]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/fi-cml-s/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/fi-cml-s/igt@gem_exec_suspend@basic-s3.html

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

  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
  [fdo#111764]: https://bugs.freedesktop.org/show_bug.cgi?id=111764


Participating hosts (50 -> 45)
------------------------------

  Additional (1): fi-hsw-4770r 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-skl-6770hq fi-byt-squawks fi-bsw-cyan fi-byt-clapper 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7290 -> Patchwork_15190

  CI-20190529: 20190529
  CI_DRM_7290: 869abae66a356231cfa6645cf491adde3590cba8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5266: 60a67653613c87a69ebecf12cf00aa362ac87597 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15190: 6c4afeac18aea52418f2f73899a4dda47fc456e3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6c4afeac18ae drm/i915/bios: do not discard address space
468f511e0df0 drm/i915/bios: make sure to check vbt size
57ae972e104c drm/i915/bios: rename bios to oprom when mapping pci rom
578cdda6a749 drm/i915/opregion: fix leaking fw on error path

== Logs ==

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

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915/opregion: fix leaking fw on error path

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/opregion: fix leaking fw on error path
URL   : https://patchwork.freedesktop.org/series/69167/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7290 -> Patchwork_15190
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/index.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [PASS][1] -> [FAIL][2] ([fdo#109483])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@gem_exec_create@basic:
    - {fi-tgl-u}:         [INCOMPLETE][3] ([fdo#111736]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/fi-tgl-u/igt@gem_exec_create@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/fi-tgl-u/igt@gem_exec_create@basic.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-cml-s:           [DMESG-WARN][5] ([fdo#111764]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/fi-cml-s/igt@gem_exec_suspend@basic-s3.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/fi-cml-s/igt@gem_exec_suspend@basic-s3.html

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

  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111736]: https://bugs.freedesktop.org/show_bug.cgi?id=111736
  [fdo#111764]: https://bugs.freedesktop.org/show_bug.cgi?id=111764


Participating hosts (50 -> 45)
------------------------------

  Additional (1): fi-hsw-4770r 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-skl-6770hq fi-byt-squawks fi-bsw-cyan fi-byt-clapper 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7290 -> Patchwork_15190

  CI-20190529: 20190529
  CI_DRM_7290: 869abae66a356231cfa6645cf491adde3590cba8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5266: 60a67653613c87a69ebecf12cf00aa362ac87597 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15190: 6c4afeac18aea52418f2f73899a4dda47fc456e3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6c4afeac18ae drm/i915/bios: do not discard address space
468f511e0df0 drm/i915/bios: make sure to check vbt size
57ae972e104c drm/i915/bios: rename bios to oprom when mapping pci rom
578cdda6a749 drm/i915/opregion: fix leaking fw on error path

== Logs ==

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

Re: [PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Convert the code to return-early style and fix missing calls
> to release_firmware() if vbt is not valid.

I don't understand where anything would leak in the current code. Please
elaborate.

You could make a case for a change in style to avoid so much
indentation, but don't claim it fixes stuff if it doesn't.

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 969ade623691..9738511147b1 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>  		return ret;
>  	}
>  
> -	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
> -		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
> -		if (opregion->vbt_firmware) {
> -			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
> -			opregion->vbt = opregion->vbt_firmware;
> -			opregion->vbt_size = fw->size;
> -			ret = 0;
> -		} else {
> -			ret = -ENOMEM;
> -		}
> -	} else {
> +	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
>  		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
>  		ret = -EINVAL;
> +		goto err_release_fw;
> +	}
> +
> +	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
> +	if (!opregion->vbt_firmware) {
> +		ret = -ENOMEM;
> +		goto err_release_fw;
>  	}
>  
> +	opregion->vbt = opregion->vbt_firmware;
> +	opregion->vbt_size = fw->size;
> +
> +	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
> +
>  	release_firmware(fw);
>  
> +	return 0;

With ret = 0 at the beginning you could just remove the the above three
lines and let this run through the below code.

> +
> +err_release_fw:
> +	release_firmware(fw);

Usually we'd have a blank line before the ret.

BR,
Jani.

>  	return ret;
>  }

-- 
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: [Intel-gfx] [PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[Jani Nikula]

On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Convert the code to return-early style and fix missing calls
> to release_firmware() if vbt is not valid.

I don't understand where anything would leak in the current code. Please
elaborate.

You could make a case for a change in style to avoid so much
indentation, but don't claim it fixes stuff if it doesn't.

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 969ade623691..9738511147b1 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>  		return ret;
>  	}
>  
> -	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
> -		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
> -		if (opregion->vbt_firmware) {
> -			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
> -			opregion->vbt = opregion->vbt_firmware;
> -			opregion->vbt_size = fw->size;
> -			ret = 0;
> -		} else {
> -			ret = -ENOMEM;
> -		}
> -	} else {
> +	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
>  		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
>  		ret = -EINVAL;
> +		goto err_release_fw;
> +	}
> +
> +	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
> +	if (!opregion->vbt_firmware) {
> +		ret = -ENOMEM;
> +		goto err_release_fw;
>  	}
>  
> +	opregion->vbt = opregion->vbt_firmware;
> +	opregion->vbt_size = fw->size;
> +
> +	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
> +
>  	release_firmware(fw);
>  
> +	return 0;

With ret = 0 at the beginning you could just remove the the above three
lines and let this run through the below code.

> +
> +err_release_fw:
> +	release_firmware(fw);

Usually we'd have a blank line before the ret.

BR,
Jani.

>  	return ret;
>  }

-- 
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/4] drm/i915/opregion: fix leaking fw on error path

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 11:16:47AM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Convert the code to return-early style and fix missing calls
>> to release_firmware() if vbt is not valid.
>
>I don't understand where anything would leak in the current code. Please
>elaborate.

It was my failure reading the current code... not sure why on a first
read I thought it was leaking.

>
>You could make a case for a change in style to avoid so much
>indentation, but don't claim it fixes stuff if it doesn't.

yeah. I will keep it out for now.

Thanks
Lucas De Marchi

>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
>>  1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
>> index 969ade623691..9738511147b1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
>> @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>>  		return ret;
>>  	}
>>
>> -	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
>> -		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> -		if (opregion->vbt_firmware) {
>> -			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> -			opregion->vbt = opregion->vbt_firmware;
>> -			opregion->vbt_size = fw->size;
>> -			ret = 0;
>> -		} else {
>> -			ret = -ENOMEM;
>> -		}
>> -	} else {
>> +	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
>>  		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
>>  		ret = -EINVAL;
>> +		goto err_release_fw;
>> +	}
>> +
>> +	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> +	if (!opregion->vbt_firmware) {
>> +		ret = -ENOMEM;
>> +		goto err_release_fw;
>>  	}
>>
>> +	opregion->vbt = opregion->vbt_firmware;
>> +	opregion->vbt_size = fw->size;
>> +
>> +	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> +
>>  	release_firmware(fw);
>>
>> +	return 0;
>
>With ret = 0 at the beginning you could just remove the the above three
>lines and let this run through the below code.
>
>> +
>> +err_release_fw:
>> +	release_firmware(fw);
>
>Usually we'd have a blank line before the ret.
>
>BR,
>Jani.
>
>>  	return ret;
>>  }
>
>-- 
>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: [Intel-gfx] [PATCH 1/4] drm/i915/opregion: fix leaking fw on error path

[Lucas De Marchi]

On Fri, Nov 08, 2019 at 11:16:47AM +0200, Jani Nikula wrote:
>On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Convert the code to return-early style and fix missing calls
>> to release_firmware() if vbt is not valid.
>
>I don't understand where anything would leak in the current code. Please
>elaborate.

It was my failure reading the current code... not sure why on a first
read I thought it was leaking.

>
>You could make a case for a change in style to avoid so much
>indentation, but don't claim it fixes stuff if it doesn't.

yeah. I will keep it out for now.

Thanks
Lucas De Marchi

>
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++--------
>>  1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
>> index 969ade623691..9738511147b1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
>> @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>>  		return ret;
>>  	}
>>
>> -	if (intel_bios_is_valid_vbt(fw->data, fw->size)) {
>> -		opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> -		if (opregion->vbt_firmware) {
>> -			DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> -			opregion->vbt = opregion->vbt_firmware;
>> -			opregion->vbt_size = fw->size;
>> -			ret = 0;
>> -		} else {
>> -			ret = -ENOMEM;
>> -		}
>> -	} else {
>> +	if (!intel_bios_is_valid_vbt(fw->data, fw->size)) {
>>  		DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name);
>>  		ret = -EINVAL;
>> +		goto err_release_fw;
>> +	}
>> +
>> +	opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> +	if (!opregion->vbt_firmware) {
>> +		ret = -ENOMEM;
>> +		goto err_release_fw;
>>  	}
>>
>> +	opregion->vbt = opregion->vbt_firmware;
>> +	opregion->vbt_size = fw->size;
>> +
>> +	DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name);
>> +
>>  	release_firmware(fw);
>>
>> +	return 0;
>
>With ret = 0 at the beginning you could just remove the the above three
>lines and let this run through the below code.
>
>> +
>> +err_release_fw:
>> +	release_firmware(fw);
>
>Usually we'd have a blank line before the ret.
>
>BR,
>Jani.
>
>>  	return ret;
>>  }
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915/opregion: fix leaking fw on error path

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/opregion: fix leaking fw on error path
URL   : https://patchwork.freedesktop.org/series/69167/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7290_full -> Patchwork_15190_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@vcs1-clean:
    - shard-iclb:         [PASS][1] -> [SKIP][2] ([fdo#109276] / [fdo#112080])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_ctx_isolation@vcs1-clean.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_ctx_isolation@vcs1-clean.html

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-tglb:         [PASS][3] -> [INCOMPLETE][4] ([fdo#111832])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb4/igt@gem_ctx_isolation@vcs1-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb4/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@gem_eio@in-flight-contexts-1us:
    - shard-snb:          [PASS][5] -> [FAIL][6] ([fdo#111946])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb2/igt@gem_eio@in-flight-contexts-1us.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb1/igt@gem_eio@in-flight-contexts-1us.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#110854])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_exec_balancer@smoke.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_schedule@preempt-queue-bsd1:
    - shard-iclb:         [PASS][9] -> [SKIP][10] ([fdo#109276]) +7 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_exec_schedule@preempt-queue-bsd1.html

  * igt@gem_exec_schedule@promotion-bsd:
    - shard-iclb:         [PASS][11] -> [SKIP][12] ([fdo#112146]) +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb3/igt@gem_exec_schedule@promotion-bsd.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb4/igt@gem_exec_schedule@promotion-bsd.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-tglb:         [PASS][13] -> [INCOMPLETE][14] ([fdo#111736] / [fdo#111850])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb4/igt@gem_exec_suspend@basic-s3.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb8/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-snb:          [PASS][15] -> [FAIL][16] ([fdo#112037])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb6/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb4/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          [PASS][17] -> [INCOMPLETE][18] ([fdo#104108])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl8/igt@gem_softpin@noreloc-s3.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl2/igt@gem_softpin@noreloc-s3.html

  * igt@gem_sync@basic-store-all:
    - shard-tglb:         [PASS][19] -> [INCOMPLETE][20] ([fdo#111647])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb7/igt@gem_sync@basic-store-all.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb6/igt@gem_sync@basic-store-all.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy:
    - shard-hsw:          [PASS][21] -> [DMESG-WARN][22] ([fdo#111870])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-hsw6/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-hsw5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
    - shard-snb:          [PASS][23] -> [DMESG-WARN][24] ([fdo#111870]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html

  * igt@i915_pm_backlight@fade_with_suspend:
    - shard-tglb:         [PASS][25] -> [INCOMPLETE][26] ([fdo#111832] / [fdo#111850]) +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb8/igt@i915_pm_backlight@fade_with_suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb3/igt@i915_pm_backlight@fade_with_suspend.html

  * igt@i915_pm_dc@dc5-dpms:
    - shard-iclb:         [PASS][27] -> [FAIL][28] ([fdo#111795 ])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb8/igt@i915_pm_dc@dc5-dpms.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb3/igt@i915_pm_dc@dc5-dpms.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-iclb:         [PASS][29] -> [FAIL][30] ([fdo#110548])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb7/igt@i915_pm_dc@dc6-dpms.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb3/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-iclb:         [PASS][31] -> [INCOMPLETE][32] ([fdo#107713] / [fdo#108840])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb8/igt@i915_pm_rpm@system-suspend-execbuf.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@i915_pm_rpm@system-suspend-execbuf.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [PASS][33] -> [DMESG-WARN][34] ([fdo#108566]) +6 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [PASS][35] -> [DMESG-WARN][36] ([fdo#108566]) +3 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-apl7/igt@kms_flip@flip-vs-suspend-interruptible.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt:
    - shard-iclb:         [PASS][37] -> [FAIL][38] ([fdo#103167]) +4 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-tglb:         [PASS][39] -> [FAIL][40] ([fdo#103167]) +2 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb3/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb4/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [PASS][41] -> [FAIL][42] ([fdo#103166])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_sprite_mmap_cpu:
    - shard-iclb:         [PASS][43] -> [SKIP][44] ([fdo#109441]) +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_cpu.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@kms_psr@psr2_sprite_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-apl:          [PASS][45] -> [FAIL][46] ([fdo#99912])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-apl4/igt@kms_setmode@basic.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-apl8/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-d-ts-continuation-dpms-suspend:
    - shard-tglb:         [PASS][47] -> [INCOMPLETE][48] ([fdo#111850])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb7/igt@kms_vblank@pipe-d-ts-continuation-dpms-suspend.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb5/igt@kms_vblank@pipe-d-ts-continuation-dpms-suspend.html

  * igt@perf_pmu@busy-vcs1:
    - shard-iclb:         [PASS][49] -> [SKIP][50] ([fdo#112080]) +5 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb2/igt@perf_pmu@busy-vcs1.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@perf_pmu@busy-vcs1.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs0-s3:
    - shard-tglb:         [INCOMPLETE][51] ([fdo#111832]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb5/igt@gem_ctx_isolation@vcs0-s3.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb6/igt@gem_ctx_isolation@vcs0-s3.html

  * igt@gem_ctx_persistence@vcs1-mixed-process:
    - shard-iclb:         [SKIP][53] ([fdo#109276] / [fdo#112080]) -> [PASS][54] +2 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@gem_ctx_persistence@vcs1-mixed-process.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@gem_ctx_persistence@vcs1-mixed-process.html

  * igt@gem_exec_nop@basic-series:
    - shard-tglb:         [INCOMPLETE][55] ([fdo#111747]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb6/igt@gem_exec_nop@basic-series.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb2/igt@gem_exec_nop@basic-series.html

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [SKIP][57] ([fdo#112146]) -> [PASS][58] +3 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@gem_exec_schedule@smoketest-blt:
    - shard-tglb:         [INCOMPLETE][59] ([fdo#111867]) -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb6/igt@gem_exec_schedule@smoketest-blt.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb8/igt@gem_exec_schedule@smoketest-blt.html

  * igt@gem_mmap_gtt@hang:
    - shard-snb:          [INCOMPLETE][61] ([fdo#105411]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb2/igt@gem_mmap_gtt@hang.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb7/igt@gem_mmap_gtt@hang.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy:
    - shard-snb:          [DMESG-WARN][63] ([fdo#110789] / [fdo#111870]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb2/igt@gem_userptr_blits@map-fixed-invalidate-busy.html

  * igt@i915_hangman@error-state-capture-vcs1:
    - shard-iclb:         [SKIP][65] ([fdo#112080]) -> [PASS][66] +8 similar issues
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@i915_hangman@error-state-capture-vcs1.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb4/igt@i915_hangman@error-state-capture-vcs1.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-iclb:         [FAIL][67] ([fdo#110548]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@i915_pm_dc@dc6-psr.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][69] ([fdo#108566]) -> [PASS][70] +1 similar issue
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@i915_suspend@fence-restore-untiled:
    - shard-tglb:         [INCOMPLETE][71] ([fdo#111832] / [fdo#111850]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb7/igt@i915_suspend@fence-restore-untiled.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb8/igt@i915_suspend@fence-restore-untiled.html

  * igt@kms_color@pipe-a-gamma:
    - shard-skl:          [FAIL][73] ([fdo#104782]) -> [PASS][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_color@pipe-a-gamma.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_color@pipe-a-gamma.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen:
    - shard-skl:          [FAIL][75] ([fdo#103232]) -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][77] ([fdo#108566]) -> [PASS][78] +3 similar issues
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
    - shard-skl:          [INCOMPLETE][79] ([fdo#110741]) -> [PASS][80]
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl6/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled:
    - shard-skl:          [FAIL][81] ([fdo#103184] / [fdo#103232]) -> [PASS][82] +1 similar issue
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [FAIL][83] ([fdo#105363]) -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl2/igt@kms_flip@flip-vs-expired-vblank.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl4/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
    - shard-iclb:         [FAIL][85] ([fdo#103167]) -> [PASS][86] +4 similar issues
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-iclb:         [FAIL][87] ([fdo#103167] / [fdo#110378]) -> [PASS][88]
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt:
    - shard-tglb:         [FAIL][89] ([fdo#103167]) -> [PASS][90] +6 similar issues
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@fbcpsr-suspend:
    - shard-tglb:         [INCOMPLETE][91] ([fdo#111832] / [fdo#111850] / [fdo#111884]) -> [PASS][92]
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb7/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt:
    - shard-skl:          [FAIL][93] ([fdo#103167]) -> [PASS][94]
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - shard-skl:          [FAIL][95] ([fdo#103191]) -> [PASS][96]
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][97] ([fdo#108145] / [fdo#110403]) -> [PASS][98]
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [SKIP][99] ([fdo#109642] / [fdo#111068]) -> [PASS][100]
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@kms_psr2_su@frontbuffer.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [SKIP][101] ([fdo#109441]) -> [PASS][102]
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb7/igt@kms_psr@psr2_basic.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@kms_psr@psr2_basic.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-tglb:         [INCOMPLETE][103] ([fdo#111850]) -> [PASS][104]
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb3/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb6/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [SKIP][105] ([fdo#109276]) -> [PASS][106] +14 similar issues
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb7/igt@prime_vgem@fence-wait-bsd2.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@prime_vgem@fence-wait-bsd2.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv-switch:
    - shard-iclb:         [SKIP][107] ([fdo#109276] / [fdo#112080]) -> [FAIL][108] ([fdo#111329])
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
   [108]: https://intel-gfx-ci.01.org/tree/

== Logs ==

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

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915/opregion: fix leaking fw on error path

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/opregion: fix leaking fw on error path
URL   : https://patchwork.freedesktop.org/series/69167/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7290_full -> Patchwork_15190_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@vcs1-clean:
    - shard-iclb:         [PASS][1] -> [SKIP][2] ([fdo#109276] / [fdo#112080])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_ctx_isolation@vcs1-clean.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_ctx_isolation@vcs1-clean.html

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-tglb:         [PASS][3] -> [INCOMPLETE][4] ([fdo#111832])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb4/igt@gem_ctx_isolation@vcs1-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb4/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@gem_eio@in-flight-contexts-1us:
    - shard-snb:          [PASS][5] -> [FAIL][6] ([fdo#111946])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb2/igt@gem_eio@in-flight-contexts-1us.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb1/igt@gem_eio@in-flight-contexts-1us.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#110854])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_exec_balancer@smoke.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_schedule@preempt-queue-bsd1:
    - shard-iclb:         [PASS][9] -> [SKIP][10] ([fdo#109276]) +7 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_exec_schedule@preempt-queue-bsd1.html

  * igt@gem_exec_schedule@promotion-bsd:
    - shard-iclb:         [PASS][11] -> [SKIP][12] ([fdo#112146]) +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb3/igt@gem_exec_schedule@promotion-bsd.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb4/igt@gem_exec_schedule@promotion-bsd.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-tglb:         [PASS][13] -> [INCOMPLETE][14] ([fdo#111736] / [fdo#111850])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb4/igt@gem_exec_suspend@basic-s3.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb8/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-snb:          [PASS][15] -> [FAIL][16] ([fdo#112037])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb6/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb4/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          [PASS][17] -> [INCOMPLETE][18] ([fdo#104108])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl8/igt@gem_softpin@noreloc-s3.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl2/igt@gem_softpin@noreloc-s3.html

  * igt@gem_sync@basic-store-all:
    - shard-tglb:         [PASS][19] -> [INCOMPLETE][20] ([fdo#111647])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb7/igt@gem_sync@basic-store-all.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb6/igt@gem_sync@basic-store-all.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy:
    - shard-hsw:          [PASS][21] -> [DMESG-WARN][22] ([fdo#111870])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-hsw6/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-hsw5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
    - shard-snb:          [PASS][23] -> [DMESG-WARN][24] ([fdo#111870]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html

  * igt@i915_pm_backlight@fade_with_suspend:
    - shard-tglb:         [PASS][25] -> [INCOMPLETE][26] ([fdo#111832] / [fdo#111850]) +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb8/igt@i915_pm_backlight@fade_with_suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb3/igt@i915_pm_backlight@fade_with_suspend.html

  * igt@i915_pm_dc@dc5-dpms:
    - shard-iclb:         [PASS][27] -> [FAIL][28] ([fdo#111795 ])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb8/igt@i915_pm_dc@dc5-dpms.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb3/igt@i915_pm_dc@dc5-dpms.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-iclb:         [PASS][29] -> [FAIL][30] ([fdo#110548])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb7/igt@i915_pm_dc@dc6-dpms.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb3/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-iclb:         [PASS][31] -> [INCOMPLETE][32] ([fdo#107713] / [fdo#108840])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb8/igt@i915_pm_rpm@system-suspend-execbuf.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@i915_pm_rpm@system-suspend-execbuf.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [PASS][33] -> [DMESG-WARN][34] ([fdo#108566]) +6 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [PASS][35] -> [DMESG-WARN][36] ([fdo#108566]) +3 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-apl7/igt@kms_flip@flip-vs-suspend-interruptible.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt:
    - shard-iclb:         [PASS][37] -> [FAIL][38] ([fdo#103167]) +4 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-tglb:         [PASS][39] -> [FAIL][40] ([fdo#103167]) +2 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb3/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb4/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [PASS][41] -> [FAIL][42] ([fdo#103166])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_sprite_mmap_cpu:
    - shard-iclb:         [PASS][43] -> [SKIP][44] ([fdo#109441]) +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_cpu.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@kms_psr@psr2_sprite_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-apl:          [PASS][45] -> [FAIL][46] ([fdo#99912])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-apl4/igt@kms_setmode@basic.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-apl8/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-d-ts-continuation-dpms-suspend:
    - shard-tglb:         [PASS][47] -> [INCOMPLETE][48] ([fdo#111850])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb7/igt@kms_vblank@pipe-d-ts-continuation-dpms-suspend.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb5/igt@kms_vblank@pipe-d-ts-continuation-dpms-suspend.html

  * igt@perf_pmu@busy-vcs1:
    - shard-iclb:         [PASS][49] -> [SKIP][50] ([fdo#112080]) +5 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb2/igt@perf_pmu@busy-vcs1.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb7/igt@perf_pmu@busy-vcs1.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs0-s3:
    - shard-tglb:         [INCOMPLETE][51] ([fdo#111832]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb5/igt@gem_ctx_isolation@vcs0-s3.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb6/igt@gem_ctx_isolation@vcs0-s3.html

  * igt@gem_ctx_persistence@vcs1-mixed-process:
    - shard-iclb:         [SKIP][53] ([fdo#109276] / [fdo#112080]) -> [PASS][54] +2 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@gem_ctx_persistence@vcs1-mixed-process.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@gem_ctx_persistence@vcs1-mixed-process.html

  * igt@gem_exec_nop@basic-series:
    - shard-tglb:         [INCOMPLETE][55] ([fdo#111747]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb6/igt@gem_exec_nop@basic-series.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb2/igt@gem_exec_nop@basic-series.html

  * igt@gem_exec_schedule@preempt-other-chain-bsd:
    - shard-iclb:         [SKIP][57] ([fdo#112146]) -> [PASS][58] +3 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@gem_exec_schedule@preempt-other-chain-bsd.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@gem_exec_schedule@preempt-other-chain-bsd.html

  * igt@gem_exec_schedule@smoketest-blt:
    - shard-tglb:         [INCOMPLETE][59] ([fdo#111867]) -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb6/igt@gem_exec_schedule@smoketest-blt.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb8/igt@gem_exec_schedule@smoketest-blt.html

  * igt@gem_mmap_gtt@hang:
    - shard-snb:          [INCOMPLETE][61] ([fdo#105411]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb2/igt@gem_mmap_gtt@hang.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb7/igt@gem_mmap_gtt@hang.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy:
    - shard-snb:          [DMESG-WARN][63] ([fdo#110789] / [fdo#111870]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-snb2/igt@gem_userptr_blits@map-fixed-invalidate-busy.html

  * igt@i915_hangman@error-state-capture-vcs1:
    - shard-iclb:         [SKIP][65] ([fdo#112080]) -> [PASS][66] +8 similar issues
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@i915_hangman@error-state-capture-vcs1.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb4/igt@i915_hangman@error-state-capture-vcs1.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-iclb:         [FAIL][67] ([fdo#110548]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb4/igt@i915_pm_dc@dc6-psr.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb8/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][69] ([fdo#108566]) -> [PASS][70] +1 similar issue
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@i915_suspend@fence-restore-untiled:
    - shard-tglb:         [INCOMPLETE][71] ([fdo#111832] / [fdo#111850]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb7/igt@i915_suspend@fence-restore-untiled.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb8/igt@i915_suspend@fence-restore-untiled.html

  * igt@kms_color@pipe-a-gamma:
    - shard-skl:          [FAIL][73] ([fdo#104782]) -> [PASS][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_color@pipe-a-gamma.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_color@pipe-a-gamma.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen:
    - shard-skl:          [FAIL][75] ([fdo#103232]) -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][77] ([fdo#108566]) -> [PASS][78] +3 similar issues
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
    - shard-skl:          [INCOMPLETE][79] ([fdo#110741]) -> [PASS][80]
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl6/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled:
    - shard-skl:          [FAIL][81] ([fdo#103184] / [fdo#103232]) -> [PASS][82] +1 similar issue
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [FAIL][83] ([fdo#105363]) -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl2/igt@kms_flip@flip-vs-expired-vblank.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl4/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
    - shard-iclb:         [FAIL][85] ([fdo#103167]) -> [PASS][86] +4 similar issues
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-iclb:         [FAIL][87] ([fdo#103167] / [fdo#110378]) -> [PASS][88]
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt:
    - shard-tglb:         [FAIL][89] ([fdo#103167]) -> [PASS][90] +6 similar issues
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@fbcpsr-suspend:
    - shard-tglb:         [INCOMPLETE][91] ([fdo#111832] / [fdo#111850] / [fdo#111884]) -> [PASS][92]
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb7/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt:
    - shard-skl:          [FAIL][93] ([fdo#103167]) -> [PASS][94]
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - shard-skl:          [FAIL][95] ([fdo#103191]) -> [PASS][96]
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl5/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl7/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][97] ([fdo#108145] / [fdo#110403]) -> [PASS][98]
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [SKIP][99] ([fdo#109642] / [fdo#111068]) -> [PASS][100]
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@kms_psr2_su@frontbuffer.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [SKIP][101] ([fdo#109441]) -> [PASS][102]
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb7/igt@kms_psr@psr2_basic.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@kms_psr@psr2_basic.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-tglb:         [INCOMPLETE][103] ([fdo#111850]) -> [PASS][104]
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-tglb3/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-tglb6/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [SKIP][105] ([fdo#109276]) -> [PASS][106] +14 similar issues
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb7/igt@prime_vgem@fence-wait-bsd2.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15190/shard-iclb2/igt@prime_vgem@fence-wait-bsd2.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv-switch:
    - shard-iclb:         [SKIP][107] ([fdo#109276] / [fdo#112080]) -> [FAIL][108] ([fdo#111329])
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7290/shard-iclb5/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
   [108]: https://intel-gfx-ci.01.org/tree/

== Logs ==

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