drm/i915: virtual PCH and PCH_NOP fixes

drm/i915: virtual PCH and PCH_NOP fixes

[Jani Nikula]

Just a resend of [1] with patch 4 dropped.

BR,
Jani.


[1] http://mid.mail-archive.com/20180531055524.21855-1-jani.nikula@intel.com


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

[PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Jani Nikula]

Virtualized non-PCH systems such as Broxton or Geminilake should use
PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
specific case to indicate a PCH system without south display.

Reported-by: Colin Xu <Colin.Xu@intel.com>
Cc: Colin Xu <Colin.Xu@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8f002ae22e62..c42e389a27f3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -287,7 +287,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
 				if (WARN_ON(pch_type == PCH_NONE))
 					pch_type = PCH_NOP;
 			} else {
-				pch_type = PCH_NOP;
+				pch_type = PCH_NONE;
 			}
 			dev_priv->pch_type = pch_type;
 			dev_priv->pch_id = id;
-- 
2.11.0

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

[PATCH 2/4] drm/i915: clean up virtual PCH special case handling

[Jani Nikula]

Use intel_pch_type() also for mapping the no PCH case (PCH id 0) to
PCH_NONE to simplify code.

Also make sure that intel_pch_type() knows all the PCH ids returned by
intel_virt_detect_pch(). Loudly fail if this isn't the case; this
shouldn't happen anyway.

Cc: Colin Xu <Colin.Xu@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c42e389a27f3..1842a067a604 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -282,13 +282,12 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
 		} else if (intel_is_virt_pch(id, pch->subsystem_vendor,
 					 pch->subsystem_device)) {
 			id = intel_virt_detect_pch(dev_priv);
-			if (id) {
-				pch_type = intel_pch_type(dev_priv, id);
-				if (WARN_ON(pch_type == PCH_NONE))
-					pch_type = PCH_NOP;
-			} else {
-				pch_type = PCH_NONE;
-			}
+			pch_type = intel_pch_type(dev_priv, id);
+
+			/* Sanity check virtual PCH id */
+			if (WARN_ON(id && pch_type == PCH_NONE))
+				id = 0;
+
 			dev_priv->pch_type = pch_type;
 			dev_priv->pch_id = id;
 			break;
-- 
2.11.0

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

[PATCH 3/4] drm/i915: be more strict about HAS_PCH_NOP() usage

[Jani Nikula]

HAS_PCH_NOP() implies a PCH platform without south display, not generic
disabled display. Prefer num_pipes == 0 for PCH independent checks.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 2 +-
 drivers/gpu/drm/i915/intel_i2c.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 1cf073b6ac8a..94428633a9d3 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1700,7 +1700,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv)
 	const struct bdb_header *bdb;
 	u8 __iomem *bios = NULL;
 
-	if (HAS_PCH_NOP(dev_priv)) {
+	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
 		DRM_DEBUG_KMS("Skipping VBT init due to disabled display.\n");
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index e6875509bcd9..61729bf84e08 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -771,7 +771,7 @@ int intel_setup_gmbus(struct drm_i915_private *dev_priv)
 	unsigned int pin;
 	int ret;
 
-	if (HAS_PCH_NOP(dev_priv))
+	if (INTEL_INFO(dev_priv)->num_pipes == 0)
 		return 0;
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-- 
2.11.0

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

[PATCH 4/4] drm/i915: fix PCH_NOP setting for non-PCH platforms

[Jani Nikula]

Setting PCH type to PCH_NOP before checking whether we actually have a
PCH ends up returning true for HAS_PCH_SPLIT() on all non-PCH split
platforms. Fix this by using PCH_NOP only for platforms that actually
have a PCH.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1842a067a604..5deee698881b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -246,14 +246,6 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
 {
 	struct pci_dev *pch = NULL;
 
-	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
-	 * (which really amounts to a PCH but no South Display).
-	 */
-	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
-		dev_priv->pch_type = PCH_NOP;
-		return;
-	}
-
 	/*
 	 * The reason to probe ISA bridge instead of Dev31:Fun0 is to
 	 * make graphics device passthrough work easy for VMM, that only
@@ -293,6 +285,17 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
 			break;
 		}
 	}
+
+	/*
+	 * Use PCH_NOP (PCH but no South Display) for PCH platforms without
+	 * display.
+	 */
+	if (pch && INTEL_INFO(dev_priv)->num_pipes == 0) {
+		DRM_DEBUG_KMS("Display disabled, reverting to NOP PCH\n");
+		dev_priv->pch_type = PCH_NOP;
+		dev_priv->pch_id = 0;
+	}
+
 	if (!pch)
 		DRM_DEBUG_KMS("No PCH found.\n");
 
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems
URL   : https://patchwork.freedesktop.org/series/44023/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4264 -> Patchwork_9157 =

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-r:           PASS -> DMESG-WARN

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-4200u:       PASS -> DMESG-FAIL (fdo#106103, fdo#102614)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload-inject:
      fi-glk-j4005:       DMESG-WARN (fdo#106725, fdo#106248) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128) -> PASS

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-glk-j4005:       FAIL (fdo#103481) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-cnl-psr:         DMESG-WARN (fdo#104951) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725


== Participating hosts (43 -> 40) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4264 -> Patchwork_9157

  CI_DRM_4264: e267b381ac5a28004f13ed2dbbbab9e9230a264c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4502: 63f0bf3d50b70f011b9d600a1a4bc1a1c7720654 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9157: 1c143950174ee16778b4f5cbad002b18bb852526 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1c143950174e drm/i915: fix PCH_NOP setting for non-PCH platforms
92691863d185 drm/i915: be more strict about HAS_PCH_NOP() usage
1e1c684fabb9 drm/i915: clean up virtual PCH special case handling
ff8c1be37217 drm/i915: fix guest virtual PCH detection on non-PCH systems

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9157/issues.html
_______________________________________________
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: fix guest virtual PCH detection on non-PCH systems

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems
URL   : https://patchwork.freedesktop.org/series/44023/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4265 -> Patchwork_9158 =

== Summary - WARNING ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-skl-6700k2:      PASS -> FAIL (fdo#103191, fdo#104724)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-read-write-distinct:
      fi-glk-j4005:       DMESG-WARN (fdo#106745, fdo#106000) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745


== Participating hosts (43 -> 40) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4265 -> Patchwork_9158

  CI_DRM_4265: 1bef5a8b1f02a5a612ac38bc6d7950becc2a9aa8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4502: 63f0bf3d50b70f011b9d600a1a4bc1a1c7720654 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9158: c90e187ee0ec334c3b084706abe2d2dcd4eefb89 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c90e187ee0ec drm/i915: fix PCH_NOP setting for non-PCH platforms
43bae6f1935e drm/i915: be more strict about HAS_PCH_NOP() usage
46ccb38e6b56 drm/i915: clean up virtual PCH special case handling
42d7eee0bf47 drm/i915: fix guest virtual PCH detection on non-PCH systems

== Logs ==

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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Lucas De Marchi]

On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> Virtualized non-PCH systems such as Broxton or Geminilake should use
> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> specific case to indicate a PCH system without south display.

Then let's go ahead and document it?

-------------
Subject: [PATCH] drm/i915: document PCH_NOP

There's a difference between PCH_NONE and PCH_NOP: the former means we
don't have a PCH while in the latter we do, but it doesn't have the
south display.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72150f89f200..aa395a898258 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -631,7 +631,7 @@ enum intel_pch {
 	PCH_KBP,        /* Kaby Lake PCH */
 	PCH_CNP,        /* Cannon Lake PCH */
 	PCH_ICP,	/* Ice Lake PCH */
-	PCH_NOP,
+	PCH_NOP,	/* PCH without south display */
 };
 
 enum intel_sbi_destination {
-- 


Feel free to squash something like that if you agree.

Lucas De Marchi



> 
> Reported-by: Colin Xu <Colin.Xu@intel.com>
> Cc: Colin Xu <Colin.Xu@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8f002ae22e62..c42e389a27f3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -287,7 +287,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  				if (WARN_ON(pch_type == PCH_NONE))
>  					pch_type = PCH_NOP;
>  			} else {
> -				pch_type = PCH_NOP;
> +				pch_type = PCH_NONE;
>  			}
>  			dev_priv->pch_type = pch_type;
>  			dev_priv->pch_id = id;
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems (rev2)
URL   : https://patchwork.freedesktop.org/series/44023/
State : failure

== Summary ==

Applying: drm/i915: fix guest virtual PCH detection on non-PCH systems
Applying: drm/i915: clean up virtual PCH special case handling
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_drv.c
Patch failed at 0002 drm/i915: clean up virtual PCH special case handling
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH 4/4] drm/i915: fix PCH_NOP setting for non-PCH platforms

[Lucas De Marchi]

On Thu, May 31, 2018 at 02:56:24PM +0300, Jani Nikula wrote:
> Setting PCH type to PCH_NOP before checking whether we actually have a
> PCH ends up returning true for HAS_PCH_SPLIT() on all non-PCH split
> platforms. Fix this by using PCH_NOP only for platforms that actually
> have a PCH.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1842a067a604..5deee698881b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -246,14 +246,6 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  {
>  	struct pci_dev *pch = NULL;
>  
> -	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
> -	 * (which really amounts to a PCH but no South Display).
> -	 */
> -	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> -		dev_priv->pch_type = PCH_NOP;
> -		return;
> -	}
> -
>  	/*
>  	 * The reason to probe ISA bridge instead of Dev31:Fun0 is to
>  	 * make graphics device passthrough work easy for VMM, that only
> @@ -293,6 +285,17 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>  			break;
>  		}
>  	}
> +
> +	/*
> +	 * Use PCH_NOP (PCH but no South Display) for PCH platforms without

Like I said on patch 1 I'd rather document in the enum what it is and
here just a "Use PCH_NOP for PCH platforms without display"

Lucas De Marchi

> +	 * display.
> +	 */
> +	if (pch && INTEL_INFO(dev_priv)->num_pipes == 0) {
> +		DRM_DEBUG_KMS("Display disabled, reverting to NOP PCH\n");
> +		dev_priv->pch_type = PCH_NOP;
> +		dev_priv->pch_id = 0;
> +	}
> +
>  	if (!pch)
>  		DRM_DEBUG_KMS("No PCH found.\n");
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
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: fix guest virtual PCH detection on non-PCH systems

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems
URL   : https://patchwork.freedesktop.org/series/44023/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4265_full -> Patchwork_9158_full =

== Summary - WARNING ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          PASS -> SKIP +1

    igt@gem_exec_schedule@deep-bsd2:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#106509, fdo#105454)

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          PASS -> FAIL (fdo#102887)

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103822)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS
      shard-glk:          INCOMPLETE (k.org#198133, fdo#103359) -> PASS

    igt@gem_tiled_blits@interruptible:
      shard-glk:          FAIL -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS +1

    igt@kms_flip@2x-modeset-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip_tiling@flip-to-x-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-pgflip-blt:
      shard-glk:          FAIL (fdo#104724, fdo#103167) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4265 -> Patchwork_9158

  CI_DRM_4265: 1bef5a8b1f02a5a612ac38bc6d7950becc2a9aa8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4502: 63f0bf3d50b70f011b9d600a1a4bc1a1c7720654 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9158: c90e187ee0ec334c3b084706abe2d2dcd4eefb89 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Colin Xu]

On 05/31/2018 07:56 PM, Jani Nikula wrote:
> Virtualized non-PCH systems such as Broxton or Geminilake should use
> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> specific case to indicate a PCH system without south display.
>
> Reported-by: Colin Xu <Colin.Xu@intel.com>
> Cc: Colin Xu <Colin.Xu@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8f002ae22e62..c42e389a27f3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -287,7 +287,7 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>   				if (WARN_ON(pch_type == PCH_NONE))
>   					pch_type = PCH_NOP;
>   			} else {
> -				pch_type = PCH_NOP;
> +				pch_type = PCH_NONE;
>   			}
>   			dev_priv->pch_type = pch_type;
>   			dev_priv->pch_id = id;

Tested on BXT gvt-g and got expected behaviour.

Tested-by: Colin Xu <Colin.Xu@intel.com>
Reviewed-by: Colin Xu <Colin.Xu@intel.com>

-- 
Best Regards,
Colin Xu

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

Re: [PATCH 2/4] drm/i915: clean up virtual PCH special case handling

[Colin Xu]

On 05/31/2018 07:56 PM, Jani Nikula wrote:
> Use intel_pch_type() also for mapping the no PCH case (PCH id 0) to
> PCH_NONE to simplify code.
>
> Also make sure that intel_pch_type() knows all the PCH ids returned by
> intel_virt_detect_pch(). Loudly fail if this isn't the case; this
> shouldn't happen anyway.
>
> Cc: Colin Xu <Colin.Xu@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c42e389a27f3..1842a067a604 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -282,13 +282,12 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>   		} else if (intel_is_virt_pch(id, pch->subsystem_vendor,
>   					 pch->subsystem_device)) {
>   			id = intel_virt_detect_pch(dev_priv);
> -			if (id) {
> -				pch_type = intel_pch_type(dev_priv, id);
> -				if (WARN_ON(pch_type == PCH_NONE))
> -					pch_type = PCH_NOP;
> -			} else {
> -				pch_type = PCH_NONE;
> -			}
> +			pch_type = intel_pch_type(dev_priv, id);
> +
> +			/* Sanity check virtual PCH id */
> +			if (WARN_ON(id && pch_type == PCH_NONE))
> +				id = 0;
> +
>   			dev_priv->pch_type = pch_type;
>   			dev_priv->pch_id = id;
>   			break;

Tested on BXT gvt-g and got expected behaviour.

Tested-by: Colin Xu <Colin.Xu@intel.com>
Reviewed-by: Colin Xu <Colin.Xu@intel.com>

-- 
Best Regards,
Colin Xu

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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Jani Nikula]

On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
>> Virtualized non-PCH systems such as Broxton or Geminilake should use
>> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
>> specific case to indicate a PCH system without south display.
>
> Then let's go ahead and document it?

Please avoid sending suggestion patches in-reply-to existing
series. This confused patchwork and screwed up CI for the series, which
was already a resend just to get CI. :(

I'm resending the series, with your documentation patch added, but I'm
keeping the extra explanatory text in the last patch. I think it's
warranted.

BR,
Jani.


>
> -------------
> Subject: [PATCH] drm/i915: document PCH_NOP
>
> There's a difference between PCH_NONE and PCH_NOP: the former means we
> don't have a PCH while in the latter we do, but it doesn't have the
> south display.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72150f89f200..aa395a898258 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -631,7 +631,7 @@ enum intel_pch {
>  	PCH_KBP,        /* Kaby Lake PCH */
>  	PCH_CNP,        /* Cannon Lake PCH */
>  	PCH_ICP,	/* Ice Lake PCH */
> -	PCH_NOP,
> +	PCH_NOP,	/* PCH without south display */
>  };
>  
>  enum intel_sbi_destination {

-- 
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: fix guest virtual PCH detection on non-PCH systems

[Lucas De Marchi]

On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> >> specific case to indicate a PCH system without south display.
> >
> > Then let's go ahead and document it?
>
> Please avoid sending suggestion patches in-reply-to existing
> series. This confused patchwork and screwed up CI for the series, which
> was already a resend just to get CI. :(

ugh, sorry.  Sometimes just adding a oneline diff is much better than
a hundred words explaining :( ... IMO pw is trying to be smarter than
it should here or not being smart enough. Nonetheless I won't do that
anymore.

Lucas De Marchi

>
> I'm resending the series, with your documentation patch added, but I'm
> keeping the extra explanatory text in the last patch. I think it's
> warranted.
>
> BR,
> Jani.
>
>
> >
> > -------------
> > Subject: [PATCH] drm/i915: document PCH_NOP
> >
> > There's a difference between PCH_NONE and PCH_NOP: the former means we
> > don't have a PCH while in the latter we do, but it doesn't have the
> > south display.
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 72150f89f200..aa395a898258 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -631,7 +631,7 @@ enum intel_pch {
> >       PCH_KBP,        /* Kaby Lake PCH */
> >       PCH_CNP,        /* Cannon Lake PCH */
> >       PCH_ICP,        /* Ice Lake PCH */
> > -     PCH_NOP,
> > +     PCH_NOP,        /* PCH without south display */
> >  };
> >
> >  enum intel_sbi_destination {
>
> --
> Jani Nikula, Intel Open Source Graphics Center



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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Jani Nikula]

On Tue, 12 Jun 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
>> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
>> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
>> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
>> >> specific case to indicate a PCH system without south display.
>> >
>> > Then let's go ahead and document it?
>>
>> Please avoid sending suggestion patches in-reply-to existing
>> series. This confused patchwork and screwed up CI for the series, which
>> was already a resend just to get CI. :(
>
> ugh, sorry.  Sometimes just adding a oneline diff is much better than
> a hundred words explaining :( ...

I feel you, a patch is more precise.

> IMO pw is trying to be smarter than it should here or not being smart
> enough. Nonetheless I won't do that anymore.

I think there were earlier complaints about what it did recognize and
what it didn't. I'd be open to only accepting new versions of patches
from whoever sent the original patch. Or requiring patch subjects don't
start with "Re:". Or both.

I was annoyed, but I'm perhaps more annoyed that you can't do this
without confusing patchwork. In the end, I wouldn't want to hamper
review by blocking something that comes naturally to people.

Arek?

BR,
Jani.




>
> Lucas De Marchi
>
>>
>> I'm resending the series, with your documentation patch added, but I'm
>> keeping the extra explanatory text in the last patch. I think it's
>> warranted.
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > -------------
>> > Subject: [PATCH] drm/i915: document PCH_NOP
>> >
>> > There's a difference between PCH_NONE and PCH_NOP: the former means we
>> > don't have a PCH while in the latter we do, but it doesn't have the
>> > south display.
>> >
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > index 72150f89f200..aa395a898258 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -631,7 +631,7 @@ enum intel_pch {
>> >       PCH_KBP,        /* Kaby Lake PCH */
>> >       PCH_CNP,        /* Cannon Lake PCH */
>> >       PCH_ICP,        /* Ice Lake PCH */
>> > -     PCH_NOP,
>> > +     PCH_NOP,        /* PCH without south display */
>> >  };
>> >
>> >  enum intel_sbi_destination {
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
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: fix guest virtual PCH detection on non-PCH systems

[Arkadiusz Hiler]

On Wed, Jun 13, 2018 at 09:49:09AM +0300, Jani Nikula wrote:
> On Tue, 12 Jun 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
> >>
> >> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> >> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> >> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
> >> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> >> >> specific case to indicate a PCH system without south display.
> >> >
> >> > Then let's go ahead and document it?
> >>
> >> Please avoid sending suggestion patches in-reply-to existing
> >> series. This confused patchwork and screwed up CI for the series, which
> >> was already a resend just to get CI. :(
> >
> > ugh, sorry.  Sometimes just adding a oneline diff is much better than
> > a hundred words explaining :( ...
> 
> I feel you, a patch is more precise.
> 
> > IMO pw is trying to be smarter than it should here or not being smart
> > enough. Nonetheless I won't do that anymore.
> 
> I think there were earlier complaints about what it did recognize and
> what it didn't. I'd be open to only accepting new versions of patches
> from whoever sent the original patch. Or requiring patch subjects don't
> start with "Re:". Or both.

No matter what you do here it will misbehave in some scenarios and
break someone's workflow :<

Originally we required the patches to have X-Mailer set to
git-send-email, which seems reasonable, but that annoyed people because
their servers were stripping out those headers.

Other people send out the patches by feeding them to the drafts folder
through IMAP and then sending them out. This, depending on the
provider's configuration, also gobbles up a thing or two.

Because of the above I am not sure about trusting "Re:" and matching
"From:" headers as good enough indicators either.

It just adds more opaque "smartness". I already can foresee questions
asking "why my v2 was not picked up?" and someone would have to debug it
down the line.

Was the address different (+XYZ before @)? Has that someone used
--subject-prefix=re:? Is it an actual bug? Etc.


> I was annoyed, but I'm perhaps more annoyed that you can't do this
> without confusing patchwork. In the end, I wouldn't want to hamper
> review by blocking something that comes naturally to people.
> 
> Arek?

Just add the following header:
"X-Patchwork-Hint: comment"
to emails that you type out manually.

For mutt it's as easy as adding:
"my_hdr X-Patchwork-Hint: comment"
to your .muttrc

https://github.com/dlespiau/patchwork/commit/148f10115525

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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Lucas De Marchi]

On Wed, Jun 13, 2018 at 1:11 AM Arkadiusz Hiler
<arkadiusz.hiler@intel.com> wrote:
>
> On Wed, Jun 13, 2018 at 09:49:09AM +0300, Jani Nikula wrote:
> > On Tue, 12 Jun 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
> > >>
> > >> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > >> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> > >> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
> > >> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> > >> >> specific case to indicate a PCH system without south display.
> > >> >
> > >> > Then let's go ahead and document it?
> > >>
> > >> Please avoid sending suggestion patches in-reply-to existing
> > >> series. This confused patchwork and screwed up CI for the series, which
> > >> was already a resend just to get CI. :(
> > >
> > > ugh, sorry.  Sometimes just adding a oneline diff is much better than
> > > a hundred words explaining :( ...
> >
> > I feel you, a patch is more precise.
> >
> > > IMO pw is trying to be smarter than it should here or not being smart
> > > enough. Nonetheless I won't do that anymore.
> >
> > I think there were earlier complaints about what it did recognize and
> > what it didn't. I'd be open to only accepting new versions of patches
> > from whoever sent the original patch. Or requiring patch subjects don't
> > start with "Re:". Or both.
>
> No matter what you do here it will misbehave in some scenarios and
> break someone's workflow :<
>
> Originally we required the patches to have X-Mailer set to
> git-send-email, which seems reasonable, but that annoyed people because
> their servers were stripping out those headers.
>
> Other people send out the patches by feeding them to the drafts folder
> through IMAP and then sending them out. This, depending on the
> provider's configuration, also gobbles up a thing or two.
>
> Because of the above I am not sure about trusting "Re:" and matching
> "From:" headers as good enough indicators either.
>
> It just adds more opaque "smartness". I already can foresee questions
> asking "why my v2 was not picked up?" and someone would have to debug it
> down the line.
>
> Was the address different (+XYZ before @)? Has that someone used
> --subject-prefix=re:? Is it an actual bug? Etc.
>
>
> > I was annoyed, but I'm perhaps more annoyed that you can't do this
> > without confusing patchwork. In the end, I wouldn't want to hamper
> > review by blocking something that comes naturally to people.
> >
> > Arek?
>
> Just add the following header:
> "X-Patchwork-Hint: comment"
> to emails that you type out manually.
>
> For mutt it's as easy as adding:
> "my_hdr X-Patchwork-Hint: comment"
> to your .muttrc

This may not work for the same reasons you pointed out that wouldn't
work if people are sending patches.  Is there a format I can use that
will not trigger patchwork from parsing a _reply_? Does removing the
"--------" help? Under the hood does it use git am --scissors or
similar?


Lucas De Marchi

>
> https://github.com/dlespiau/patchwork/commit/148f10115525
>
> --
> Cheers,
> Arek



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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Lucas De Marchi]

On Wed, Jun 13, 2018 at 10:09 AM Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
>
> On Wed, Jun 13, 2018 at 1:11 AM Arkadiusz Hiler
> <arkadiusz.hiler@intel.com> wrote:
> >
> > On Wed, Jun 13, 2018 at 09:49:09AM +0300, Jani Nikula wrote:
> > > On Tue, 12 Jun 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > > On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
> > > >>
> > > >> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > >> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> > > >> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
> > > >> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> > > >> >> specific case to indicate a PCH system without south display.
> > > >> >
> > > >> > Then let's go ahead and document it?
> > > >>
> > > >> Please avoid sending suggestion patches in-reply-to existing
> > > >> series. This confused patchwork and screwed up CI for the series, which
> > > >> was already a resend just to get CI. :(
> > > >
> > > > ugh, sorry.  Sometimes just adding a oneline diff is much better than
> > > > a hundred words explaining :( ...
> > >
> > > I feel you, a patch is more precise.
> > >
> > > > IMO pw is trying to be smarter than it should here or not being smart
> > > > enough. Nonetheless I won't do that anymore.
> > >
> > > I think there were earlier complaints about what it did recognize and
> > > what it didn't. I'd be open to only accepting new versions of patches
> > > from whoever sent the original patch. Or requiring patch subjects don't
> > > start with "Re:". Or both.
> >
> > No matter what you do here it will misbehave in some scenarios and
> > break someone's workflow :<
> >
> > Originally we required the patches to have X-Mailer set to
> > git-send-email, which seems reasonable, but that annoyed people because
> > their servers were stripping out those headers.
> >
> > Other people send out the patches by feeding them to the drafts folder
> > through IMAP and then sending them out. This, depending on the
> > provider's configuration, also gobbles up a thing or two.
> >
> > Because of the above I am not sure about trusting "Re:" and matching
> > "From:" headers as good enough indicators either.
> >
> > It just adds more opaque "smartness". I already can foresee questions
> > asking "why my v2 was not picked up?" and someone would have to debug it
> > down the line.
> >
> > Was the address different (+XYZ before @)? Has that someone used
> > --subject-prefix=re:? Is it an actual bug? Etc.
> >
> >
> > > I was annoyed, but I'm perhaps more annoyed that you can't do this
> > > without confusing patchwork. In the end, I wouldn't want to hamper
> > > review by blocking something that comes naturally to people.
> > >
> > > Arek?
> >
> > Just add the following header:
> > "X-Patchwork-Hint: comment"
> > to emails that you type out manually.
> >
> > For mutt it's as easy as adding:
> > "my_hdr X-Patchwork-Hint: comment"
> > to your .muttrc
>
> This may not work for the same reasons you pointed out that wouldn't
> work if people are sending patches.  Is there a format I can use that
> will not trigger patchwork from parsing a _reply_? Does removing the
> "--------" help? Under the hood does it use git am --scissors or
> similar?

Humn... it has its own parser. So if I read
https://github.com/dlespiau/patchwork/blob/master/patchwork/parser.py#L36
correctly, it should be just a matter of omitting the "diff | ---"
lines (or prepending with a "#").

It does make things more difficult if the other person would use "git
am --scissors" though.


Lucas De Marchi

>
>
> Lucas De Marchi
>
> >
> > https://github.com/dlespiau/patchwork/commit/148f10115525
> >
> > --
> > Cheers,
> > Arek
>
>
>
> --
> Lucas De Marchi



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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Arkadiusz Hiler]

On Wed, Jun 13, 2018 at 10:16:07AM -0700, Lucas De Marchi wrote:
> On Wed, Jun 13, 2018 at 10:09 AM Lucas De Marchi
> <lucas.de.marchi@gmail.com> wrote:
> >
> > On Wed, Jun 13, 2018 at 1:11 AM Arkadiusz Hiler
> > <arkadiusz.hiler@intel.com> wrote:
> > >
> > > On Wed, Jun 13, 2018 at 09:49:09AM +0300, Jani Nikula wrote:
> > > > On Tue, 12 Jun 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > > > On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
> > > > >>
> > > > >> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > > >> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> > > > >> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
> > > > >> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> > > > >> >> specific case to indicate a PCH system without south display.
> > > > >> >
> > > > >> > Then let's go ahead and document it?
> > > > >>
> > > > >> Please avoid sending suggestion patches in-reply-to existing
> > > > >> series. This confused patchwork and screwed up CI for the series, which
> > > > >> was already a resend just to get CI. :(
> > > > >
> > > > > ugh, sorry.  Sometimes just adding a oneline diff is much better than
> > > > > a hundred words explaining :( ...
> > > >
> > > > I feel you, a patch is more precise.
> > > >
> > > > > IMO pw is trying to be smarter than it should here or not being smart
> > > > > enough. Nonetheless I won't do that anymore.
> > > >
> > > > I think there were earlier complaints about what it did recognize and
> > > > what it didn't. I'd be open to only accepting new versions of patches
> > > > from whoever sent the original patch. Or requiring patch subjects don't
> > > > start with "Re:". Or both.
> > >
> > > No matter what you do here it will misbehave in some scenarios and
> > > break someone's workflow :<
> > >
> > > Originally we required the patches to have X-Mailer set to
> > > git-send-email, which seems reasonable, but that annoyed people because
> > > their servers were stripping out those headers.
> > >
> > > Other people send out the patches by feeding them to the drafts folder
> > > through IMAP and then sending them out. This, depending on the
> > > provider's configuration, also gobbles up a thing or two.
> > >
> > > Because of the above I am not sure about trusting "Re:" and matching
> > > "From:" headers as good enough indicators either.
> > >
> > > It just adds more opaque "smartness". I already can foresee questions
> > > asking "why my v2 was not picked up?" and someone would have to debug it
> > > down the line.
> > >
> > > Was the address different (+XYZ before @)? Has that someone used
> > > --subject-prefix=re:? Is it an actual bug? Etc.
> > >
> > >
> > > > I was annoyed, but I'm perhaps more annoyed that you can't do this
> > > > without confusing patchwork. In the end, I wouldn't want to hamper
> > > > review by blocking something that comes naturally to people.
> > > >
> > > > Arek?
> > >
> > > Just add the following header:
> > > "X-Patchwork-Hint: comment"
> > > to emails that you type out manually.
> > >
> > > For mutt it's as easy as adding:
> > > "my_hdr X-Patchwork-Hint: comment"
> > > to your .muttrc
> >
> > This may not work for the same reasons you pointed out that wouldn't
> > work if people are sending patches.  Is there a format I can use that
> > will not trigger patchwork from parsing a _reply_? Does removing the
> > "--------" help? Under the hood does it use git am --scissors or
> > similar?

Yeah, it's far for perfection and needs additional effort to set the
header up. For me it works, but I always send patches via git-send-email
and always send replies via mutt - I am the simple case.

> Humn... it has its own parser. So if I read
> https://github.com/dlespiau/patchwork/blob/master/patchwork/parser.py#L36
> correctly, it should be just a matter of omitting the "diff | ---"
> lines (or prepending with a "#").
> 
> It does make things more difficult if the other person would use "git
> am --scissors" though.
> 
> Lucas De Marchi

Yes, that is my understanding as well - if you would ommit the "diff
header" it should not recognize your mail as a patch. But that's yet
another behavior you have to know upfront.

It's really hard to strike a balance here.

One idea is to optimize for the default case (i.e. behavior of
git-send-email), sans the known quirks we have seen so far,
and write this down.

Then, if some patches are getting ignored, this would make a handy
checklist that can be use for troubleshooting and we can also link to
it, kindly asking to adhere to a more standard way of sending patches.

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

Re: [PATCH 1/4] drm/i915: fix guest virtual PCH detection on non-PCH systems

[Lucas De Marchi]

On Wed, Jun 13, 2018 at 11:57 PM Arkadiusz Hiler
<arkadiusz.hiler@intel.com> wrote:
>
> On Wed, Jun 13, 2018 at 10:16:07AM -0700, Lucas De Marchi wrote:
> > On Wed, Jun 13, 2018 at 10:09 AM Lucas De Marchi
> > <lucas.de.marchi@gmail.com> wrote:
> > >
> > > On Wed, Jun 13, 2018 at 1:11 AM Arkadiusz Hiler
> > > <arkadiusz.hiler@intel.com> wrote:
> > > >
> > > > On Wed, Jun 13, 2018 at 09:49:09AM +0300, Jani Nikula wrote:
> > > > > On Tue, 12 Jun 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > > > > On Fri, Jun 8, 2018 at 5:34 AM Jani Nikula <jani.nikula@intel.com> wrote:
> > > > > >>
> > > > > >> On Thu, 31 May 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> > > > > >> > On Thu, May 31, 2018 at 02:56:21PM +0300, Jani Nikula wrote:
> > > > > >> >> Virtualized non-PCH systems such as Broxton or Geminilake should use
> > > > > >> >> PCH_NONE to indicate no PCH rather than PCH_NOP. The latter is a
> > > > > >> >> specific case to indicate a PCH system without south display.
> > > > > >> >
> > > > > >> > Then let's go ahead and document it?
> > > > > >>
> > > > > >> Please avoid sending suggestion patches in-reply-to existing
> > > > > >> series. This confused patchwork and screwed up CI for the series, which
> > > > > >> was already a resend just to get CI. :(
> > > > > >
> > > > > > ugh, sorry.  Sometimes just adding a oneline diff is much better than
> > > > > > a hundred words explaining :( ...
> > > > >
> > > > > I feel you, a patch is more precise.
> > > > >
> > > > > > IMO pw is trying to be smarter than it should here or not being smart
> > > > > > enough. Nonetheless I won't do that anymore.
> > > > >
> > > > > I think there were earlier complaints about what it did recognize and
> > > > > what it didn't. I'd be open to only accepting new versions of patches
> > > > > from whoever sent the original patch. Or requiring patch subjects don't
> > > > > start with "Re:". Or both.
> > > >
> > > > No matter what you do here it will misbehave in some scenarios and
> > > > break someone's workflow :<
> > > >
> > > > Originally we required the patches to have X-Mailer set to
> > > > git-send-email, which seems reasonable, but that annoyed people because
> > > > their servers were stripping out those headers.
> > > >
> > > > Other people send out the patches by feeding them to the drafts folder
> > > > through IMAP and then sending them out. This, depending on the
> > > > provider's configuration, also gobbles up a thing or two.
> > > >
> > > > Because of the above I am not sure about trusting "Re:" and matching
> > > > "From:" headers as good enough indicators either.
> > > >
> > > > It just adds more opaque "smartness". I already can foresee questions
> > > > asking "why my v2 was not picked up?" and someone would have to debug it
> > > > down the line.
> > > >
> > > > Was the address different (+XYZ before @)? Has that someone used
> > > > --subject-prefix=re:? Is it an actual bug? Etc.
> > > >
> > > >
> > > > > I was annoyed, but I'm perhaps more annoyed that you can't do this
> > > > > without confusing patchwork. In the end, I wouldn't want to hamper
> > > > > review by blocking something that comes naturally to people.
> > > > >
> > > > > Arek?
> > > >
> > > > Just add the following header:
> > > > "X-Patchwork-Hint: comment"
> > > > to emails that you type out manually.
> > > >
> > > > For mutt it's as easy as adding:
> > > > "my_hdr X-Patchwork-Hint: comment"
> > > > to your .muttrc
> > >
> > > This may not work for the same reasons you pointed out that wouldn't
> > > work if people are sending patches.  Is there a format I can use that
> > > will not trigger patchwork from parsing a _reply_? Does removing the
> > > "--------" help? Under the hood does it use git am --scissors or
> > > similar?
>
> Yeah, it's far for perfection and needs additional effort to set the
> header up. For me it works, but I always send patches via git-send-email
> and always send replies via mutt - I am the simple case.
>
> > Humn... it has its own parser. So if I read
> > https://github.com/dlespiau/patchwork/blob/master/patchwork/parser.py#L36
> > correctly, it should be just a matter of omitting the "diff | ---"
> > lines (or prepending with a "#").
> >
> > It does make things more difficult if the other person would use "git
> > am --scissors" though.
> >
> > Lucas De Marchi
>
> Yes, that is my understanding as well - if you would ommit the "diff
> header" it should not recognize your mail as a patch. But that's yet
> another behavior you have to know upfront.
>
> It's really hard to strike a balance here.
>
> One idea is to optimize for the default case (i.e. behavior of
> git-send-email), sans the known quirks we have seen so far,
> and write this down.
>
> Then, if some patches are getting ignored, this would make a handy
> checklist that can be use for troubleshooting and we can also link to
> it, kindly asking to adhere to a more standard way of sending patches.

Agreed.

Lucas De Marchi

>
> --
> Cheers,
> Arek



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