[PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation

[PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Revert to the original explicit approach and document the reasoning
behind it.

v2:
 * DG2 needs to be covered too. (Matt)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
---
Matt, does DG1 need to be in the MCR branch or plain Gen12?
---
 drivers/gpu/drm/i915/gt/intel_gt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 7eeee5a7cb33..b2556a3d8a3f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,7 +1070,19 @@ static void mmio_invalidate_full(struct intel_gt *gt)
 	unsigned int num = 0;
 	unsigned long flags;
 
-	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+	/*
+	 * New platforms should not be added with catch-all-newer (>=)
+	 * condition so that any later platform added triggers the below warning
+	 * and in turn mandates a human cross-check of whether the invalidation
+	 * flows have compatible semantics.
+	 *
+	 * For instance with the 11.00 -> 12.00 transition three out of five
+	 * respective engine registers were moved to masked type. Then after the
+	 * 12.00 -> 12.50 transition multi cast handling is required too.
+	 */
+
+	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
 		regs = NULL;
 		num = ARRAY_SIZE(xehp_regs);
 	} else if (GRAPHICS_VER(i915) == 12) {
-- 
2.34.1

[Intel-gfx] [PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Revert to the original explicit approach and document the reasoning
behind it.

v2:
 * DG2 needs to be covered too. (Matt)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
---
Matt, does DG1 need to be in the MCR branch or plain Gen12?
---
 drivers/gpu/drm/i915/gt/intel_gt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 7eeee5a7cb33..b2556a3d8a3f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,7 +1070,19 @@ static void mmio_invalidate_full(struct intel_gt *gt)
 	unsigned int num = 0;
 	unsigned long flags;
 
-	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+	/*
+	 * New platforms should not be added with catch-all-newer (>=)
+	 * condition so that any later platform added triggers the below warning
+	 * and in turn mandates a human cross-check of whether the invalidation
+	 * flows have compatible semantics.
+	 *
+	 * For instance with the 11.00 -> 12.00 transition three out of five
+	 * respective engine registers were moved to masked type. Then after the
+	 * 12.00 -> 12.50 transition multi cast handling is required too.
+	 */
+
+	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
 		regs = NULL;
 		num = ARRAY_SIZE(xehp_regs);
 	} else if (GRAPHICS_VER(i915) == 12) {
-- 
2.34.1

Re: [PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation

[Matt Roper]

On Fri, Jan 06, 2023 at 10:38:35AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Revert to the original explicit approach and document the reasoning
> behind it.
> 
> v2:
>  * DG2 needs to be covered too. (Matt)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
> ---
> Matt, does DG1 need to be in the MCR branch or plain Gen12?

DG1 should use the same "gen12" branch as TGL/RKL/ADL.  Bspec page 66696
is the relevant MMIO table for DG1 and the range containing the TLB
invalidation registers is not a multicast/replicated range.  The types
of engines supported, and the register details for each engine are also
the same as TGL/RKL/ADL.

> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7eeee5a7cb33..b2556a3d8a3f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,7 +1070,19 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>  	unsigned int num = 0;
>  	unsigned long flags;
>  
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * New platforms should not be added with catch-all-newer (>=)
> +	 * condition so that any later platform added triggers the below warning
> +	 * and in turn mandates a human cross-check of whether the invalidation
> +	 * flows have compatible semantics.
> +	 *
> +	 * For instance with the 11.00 -> 12.00 transition three out of five
> +	 * respective engine registers were moved to masked type. Then after the
> +	 * 12.00 -> 12.50 transition multi cast handling is required too.
> +	 */
> +
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
> +	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>  		regs = NULL;
>  		num = ARRAY_SIZE(xehp_regs);
>  	} else if (GRAPHICS_VER(i915) == 12) {

Did you want to switch this one to

        GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
        GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)

so that it won't automatically pick up future 12.xx platforms like PVC,
MTL, and whatever else might show up in that category in the future?


Matt

> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [Intel-gfx] [PATCH v2] drm/i915: Do not cover all future platforms in TLB invalidation

[Matt Roper]

On Fri, Jan 06, 2023 at 10:38:35AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Revert to the original explicit approach and document the reasoning
> behind it.
> 
> v2:
>  * DG2 needs to be covered too. (Matt)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
> ---
> Matt, does DG1 need to be in the MCR branch or plain Gen12?

DG1 should use the same "gen12" branch as TGL/RKL/ADL.  Bspec page 66696
is the relevant MMIO table for DG1 and the range containing the TLB
invalidation registers is not a multicast/replicated range.  The types
of engines supported, and the register details for each engine are also
the same as TGL/RKL/ADL.

> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7eeee5a7cb33..b2556a3d8a3f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,7 +1070,19 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>  	unsigned int num = 0;
>  	unsigned long flags;
>  
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * New platforms should not be added with catch-all-newer (>=)
> +	 * condition so that any later platform added triggers the below warning
> +	 * and in turn mandates a human cross-check of whether the invalidation
> +	 * flows have compatible semantics.
> +	 *
> +	 * For instance with the 11.00 -> 12.00 transition three out of five
> +	 * respective engine registers were moved to masked type. Then after the
> +	 * 12.00 -> 12.50 transition multi cast handling is required too.
> +	 */
> +
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
> +	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>  		regs = NULL;
>  		num = ARRAY_SIZE(xehp_regs);
>  	} else if (GRAPHICS_VER(i915) == 12) {

Did you want to switch this one to

        GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
        GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)

so that it won't automatically pick up future 12.xx platforms like PVC,
MTL, and whatever else might show up in that category in the future?


Matt

> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

[PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Revert to the original explicit approach and document the reasoning
behind it.

v2:
 * DG2 needs to be covered too. (Matt)

v3:
 * Full version check for Gen12 to avoid catching all future platforms.
   (Matt)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
---
 drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 7eeee5a7cb33..5521fa057aab 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
 	unsigned int num = 0;
 	unsigned long flags;
 
-	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+	/*
+	 * New platforms should not be added with catch-all-newer (>=)
+	 * condition so that any later platform added triggers the below warning
+	 * and in turn mandates a human cross-check of whether the invalidation
+	 * flows have compatible semantics.
+	 *
+	 * For instance with the 11.00 -> 12.00 transition three out of five
+	 * respective engine registers were moved to masked type. Then after the
+	 * 12.00 -> 12.50 transition multi cast handling is required too.
+	 */
+
+	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
 		regs = NULL;
 		num = ARRAY_SIZE(xehp_regs);
-	} else if (GRAPHICS_VER(i915) == 12) {
+	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
+		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
 		regs = gen12_regs;
 		num = ARRAY_SIZE(gen12_regs);
 	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
-- 
2.34.1

[Intel-gfx] [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Revert to the original explicit approach and document the reasoning
behind it.

v2:
 * DG2 needs to be covered too. (Matt)

v3:
 * Full version check for Gen12 to avoid catching all future platforms.
   (Matt)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
---
 drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 7eeee5a7cb33..5521fa057aab 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
 	unsigned int num = 0;
 	unsigned long flags;
 
-	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+	/*
+	 * New platforms should not be added with catch-all-newer (>=)
+	 * condition so that any later platform added triggers the below warning
+	 * and in turn mandates a human cross-check of whether the invalidation
+	 * flows have compatible semantics.
+	 *
+	 * For instance with the 11.00 -> 12.00 transition three out of five
+	 * respective engine registers were moved to masked type. Then after the
+	 * 12.00 -> 12.50 transition multi cast handling is required too.
+	 */
+
+	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
 		regs = NULL;
 		num = ARRAY_SIZE(xehp_regs);
-	} else if (GRAPHICS_VER(i915) == 12) {
+	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
+		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
 		regs = gen12_regs;
 		num = ARRAY_SIZE(gen12_regs);
 	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
-- 
2.34.1

Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Matt Roper]

On Mon, Jan 09, 2023 at 12:24:42PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Revert to the original explicit approach and document the reasoning
> behind it.
> 
> v2:
>  * DG2 needs to be covered too. (Matt)
> 
> v3:
>  * Full version check for Gen12 to avoid catching all future platforms.
>    (Matt)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7eeee5a7cb33..5521fa057aab 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>  	unsigned int num = 0;
>  	unsigned long flags;
>  
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * New platforms should not be added with catch-all-newer (>=)
> +	 * condition so that any later platform added triggers the below warning
> +	 * and in turn mandates a human cross-check of whether the invalidation
> +	 * flows have compatible semantics.
> +	 *
> +	 * For instance with the 11.00 -> 12.00 transition three out of five
> +	 * respective engine registers were moved to masked type. Then after the
> +	 * 12.00 -> 12.50 transition multi cast handling is required too.
> +	 */
> +
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
> +	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>  		regs = NULL;
>  		num = ARRAY_SIZE(xehp_regs);
> -	} else if (GRAPHICS_VER(i915) == 12) {
> +	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> +		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>  		regs = gen12_regs;
>  		num = ARRAY_SIZE(gen12_regs);
>  	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [Intel-gfx] [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Matt Roper]

On Mon, Jan 09, 2023 at 12:24:42PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Revert to the original explicit approach and document the reasoning
> behind it.
> 
> v2:
>  * DG2 needs to be covered too. (Matt)
> 
> v3:
>  * Full version check for Gen12 to avoid catching all future platforms.
>    (Matt)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7eeee5a7cb33..5521fa057aab 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>  	unsigned int num = 0;
>  	unsigned long flags;
>  
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * New platforms should not be added with catch-all-newer (>=)
> +	 * condition so that any later platform added triggers the below warning
> +	 * and in turn mandates a human cross-check of whether the invalidation
> +	 * flows have compatible semantics.
> +	 *
> +	 * For instance with the 11.00 -> 12.00 transition three out of five
> +	 * respective engine registers were moved to masked type. Then after the
> +	 * 12.00 -> 12.50 transition multi cast handling is required too.
> +	 */
> +
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
> +	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>  		regs = NULL;
>  		num = ARRAY_SIZE(xehp_regs);
> -	} else if (GRAPHICS_VER(i915) == 12) {
> +	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> +		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>  		regs = gen12_regs;
>  		num = ARRAY_SIZE(gen12_regs);
>  	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Andrzej Hajda]

On 09.01.2023 13:24, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Revert to the original explicit approach and document the reasoning
> behind it.
>
> v2:
>   * DG2 needs to be covered too. (Matt)
>
> v3:
>   * Full version check for Gen12 to avoid catching all future platforms.
>     (Matt)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
> ---
>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7eeee5a7cb33..5521fa057aab 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>   	unsigned int num = 0;
>   	unsigned long flags;
>   
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * New platforms should not be added with catch-all-newer (>=)
> +	 * condition so that any later platform added triggers the below warning
> +	 * and in turn mandates a human cross-check of whether the invalidation
> +	 * flows have compatible semantics.
> +	 *
> +	 * For instance with the 11.00 -> 12.00 transition three out of five
> +	 * respective engine registers were moved to masked type. Then after the
> +	 * 12.00 -> 12.50 transition multi cast handling is required too.
> +	 */
> +
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
> +	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>   		regs = NULL;
>   		num = ARRAY_SIZE(xehp_regs);
> -	} else if (GRAPHICS_VER(i915) == 12) {
> +	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> +		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {

MTL support is lost? IP_VER(12, 70)

And again it looks for me inconsistent, some unknown platforms are 
covered, for example 12.54, some not, for example 12.11.

Regards
Andrzej

>   		regs = gen12_regs;
>   		num = ARRAY_SIZE(gen12_regs);
>   	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {

Re: [Intel-gfx] [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Andrzej Hajda]

On 09.01.2023 13:24, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Revert to the original explicit approach and document the reasoning
> behind it.
>
> v2:
>   * DG2 needs to be covered too. (Matt)
>
> v3:
>   * Full version check for Gen12 to avoid catching all future platforms.
>     (Matt)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
> ---
>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7eeee5a7cb33..5521fa057aab 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>   	unsigned int num = 0;
>   	unsigned long flags;
>   
> -	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> +	/*
> +	 * New platforms should not be added with catch-all-newer (>=)
> +	 * condition so that any later platform added triggers the below warning
> +	 * and in turn mandates a human cross-check of whether the invalidation
> +	 * flows have compatible semantics.
> +	 *
> +	 * For instance with the 11.00 -> 12.00 transition three out of five
> +	 * respective engine registers were moved to masked type. Then after the
> +	 * 12.00 -> 12.50 transition multi cast handling is required too.
> +	 */
> +
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
> +	    GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>   		regs = NULL;
>   		num = ARRAY_SIZE(xehp_regs);
> -	} else if (GRAPHICS_VER(i915) == 12) {
> +	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> +		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {

MTL support is lost? IP_VER(12, 70)

And again it looks for me inconsistent, some unknown platforms are 
covered, for example 12.54, some not, for example 12.11.

Regards
Andrzej

>   		regs = gen12_regs;
>   		num = ARRAY_SIZE(gen12_regs);
>   	} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {

Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Tvrtko Ursulin]

On 10/01/2023 08:23, Andrzej Hajda wrote:
> 
> 
> On 09.01.2023 13:24, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Revert to the original explicit approach and document the reasoning
>> behind it.
>>
>> v2:
>>   * DG2 needs to be covered too. (Matt)
>>
>> v3:
>>   * Full version check for Gen12 to avoid catching all future platforms.
>>     (Matt)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index 7eeee5a7cb33..5521fa057aab 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct 
>> intel_gt *gt)
>>       unsigned int num = 0;
>>       unsigned long flags;
>> -    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>> +    /*
>> +     * New platforms should not be added with catch-all-newer (>=)
>> +     * condition so that any later platform added triggers the below 
>> warning
>> +     * and in turn mandates a human cross-check of whether the 
>> invalidation
>> +     * flows have compatible semantics.
>> +     *
>> +     * For instance with the 11.00 -> 12.00 transition three out of five
>> +     * respective engine registers were moved to masked type. Then 
>> after the
>> +     * 12.00 -> 12.50 transition multi cast handling is required too.
>> +     */
>> +
>> +    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
>> +        GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>>           regs = NULL;
>>           num = ARRAY_SIZE(xehp_regs);
>> -    } else if (GRAPHICS_VER(i915) == 12) {
>> +    } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>> +           GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> 
> MTL support is lost? IP_VER(12, 70)

AFAIU Matt says MTL is still incomplete anyway, so that would be added 
in an explicit patch here.

> And again it looks for me inconsistent, some unknown platforms are 
> covered, for example 12.54, some not, for example 12.11.

.11 and .54 as hypotheticals? You suggest this instead:

	if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
	    GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
		regs = NULL;
		num = ARRAY_SIZE(xehp_regs);
	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
		regs = gen12_regs;
		num = ARRAY_SIZE(gen12_regs);

?

It's fine by me if that covers all currently known platforms.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Tvrtko Ursulin]

On 10/01/2023 08:23, Andrzej Hajda wrote:
> 
> 
> On 09.01.2023 13:24, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Revert to the original explicit approach and document the reasoning
>> behind it.
>>
>> v2:
>>   * DG2 needs to be covered too. (Matt)
>>
>> v3:
>>   * Full version check for Gen12 to avoid catching all future platforms.
>>     (Matt)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index 7eeee5a7cb33..5521fa057aab 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct 
>> intel_gt *gt)
>>       unsigned int num = 0;
>>       unsigned long flags;
>> -    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>> +    /*
>> +     * New platforms should not be added with catch-all-newer (>=)
>> +     * condition so that any later platform added triggers the below 
>> warning
>> +     * and in turn mandates a human cross-check of whether the 
>> invalidation
>> +     * flows have compatible semantics.
>> +     *
>> +     * For instance with the 11.00 -> 12.00 transition three out of five
>> +     * respective engine registers were moved to masked type. Then 
>> after the
>> +     * 12.00 -> 12.50 transition multi cast handling is required too.
>> +     */
>> +
>> +    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
>> +        GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>>           regs = NULL;
>>           num = ARRAY_SIZE(xehp_regs);
>> -    } else if (GRAPHICS_VER(i915) == 12) {
>> +    } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>> +           GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> 
> MTL support is lost? IP_VER(12, 70)

AFAIU Matt says MTL is still incomplete anyway, so that would be added 
in an explicit patch here.

> And again it looks for me inconsistent, some unknown platforms are 
> covered, for example 12.54, some not, for example 12.11.

.11 and .54 as hypotheticals? You suggest this instead:

	if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
	    GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
		regs = NULL;
		num = ARRAY_SIZE(xehp_regs);
	} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
		   GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
		regs = gen12_regs;
		num = ARRAY_SIZE(gen12_regs);

?

It's fine by me if that covers all currently known platforms.

Regards,

Tvrtko

Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Andrzej Hajda]

On 10.01.2023 10:16, Tvrtko Ursulin wrote:
>
> On 10/01/2023 08:23, Andrzej Hajda wrote:
>>
>>
>> On 09.01.2023 13:24, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Revert to the original explicit approach and document the reasoning
>>> behind it.
>>>
>>> v2:
>>>   * DG2 needs to be covered too. (Matt)
>>>
>>> v3:
>>>   * Full version check for Gen12 to avoid catching all future 
>>> platforms.
>>>     (Matt)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> index 7eeee5a7cb33..5521fa057aab 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct 
>>> intel_gt *gt)
>>>       unsigned int num = 0;
>>>       unsigned long flags;
>>> -    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>>> +    /*
>>> +     * New platforms should not be added with catch-all-newer (>=)
>>> +     * condition so that any later platform added triggers the 
>>> below warning
>>> +     * and in turn mandates a human cross-check of whether the 
>>> invalidation
>>> +     * flows have compatible semantics.
>>> +     *
>>> +     * For instance with the 11.00 -> 12.00 transition three out of 
>>> five
>>> +     * respective engine registers were moved to masked type. Then 
>>> after the
>>> +     * 12.00 -> 12.50 transition multi cast handling is required too.
>>> +     */
>>> +
>>> +    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
>>> +        GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>>>           regs = NULL;
>>>           num = ARRAY_SIZE(xehp_regs);
>>> -    } else if (GRAPHICS_VER(i915) == 12) {
>>> +    } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>>> +           GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>>
>> MTL support is lost? IP_VER(12, 70)
>
> AFAIU Matt says MTL is still incomplete anyway, so that would be added 
> in an explicit patch here.

I've missed this part, sorry for the noise then :)
And as I see PVC is similar story.

>
>> And again it looks for me inconsistent, some unknown platforms are 
>> covered, for example 12.54, some not, for example 12.11.
>
> .11 and .54 as hypotheticals? You suggest this instead:
>
>     if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
>         GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
>         regs = NULL;
>         num = ARRAY_SIZE(xehp_regs);
>     } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>            GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>         regs = gen12_regs;
>         num = ARRAY_SIZE(gen12_regs);
>
> ?

For me this perfectly follows the 'strict' approach :)

>
> It's fine by me if that covers all currently known platforms.

My grep in i915_pci.c agrees.

Regards
Andrzej

>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation

[Andrzej Hajda]

On 10.01.2023 10:16, Tvrtko Ursulin wrote:
>
> On 10/01/2023 08:23, Andrzej Hajda wrote:
>>
>>
>> On 09.01.2023 13:24, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Revert to the original explicit approach and document the reasoning
>>> behind it.
>>>
>>> v2:
>>>   * DG2 needs to be covered too. (Matt)
>>>
>>> v3:
>>>   * Full version check for Gen12 to avoid catching all future 
>>> platforms.
>>>     (Matt)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v1
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++++++++++++++--
>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> index 7eeee5a7cb33..5521fa057aab 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct 
>>> intel_gt *gt)
>>>       unsigned int num = 0;
>>>       unsigned long flags;
>>> -    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
>>> +    /*
>>> +     * New platforms should not be added with catch-all-newer (>=)
>>> +     * condition so that any later platform added triggers the 
>>> below warning
>>> +     * and in turn mandates a human cross-check of whether the 
>>> invalidation
>>> +     * flows have compatible semantics.
>>> +     *
>>> +     * For instance with the 11.00 -> 12.00 transition three out of 
>>> five
>>> +     * respective engine registers were moved to masked type. Then 
>>> after the
>>> +     * 12.00 -> 12.50 transition multi cast handling is required too.
>>> +     */
>>> +
>>> +    if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
>>> +        GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
>>>           regs = NULL;
>>>           num = ARRAY_SIZE(xehp_regs);
>>> -    } else if (GRAPHICS_VER(i915) == 12) {
>>> +    } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>>> +           GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>>
>> MTL support is lost? IP_VER(12, 70)
>
> AFAIU Matt says MTL is still incomplete anyway, so that would be added 
> in an explicit patch here.

I've missed this part, sorry for the noise then :)
And as I see PVC is similar story.

>
>> And again it looks for me inconsistent, some unknown platforms are 
>> covered, for example 12.54, some not, for example 12.11.
>
> .11 and .54 as hypotheticals? You suggest this instead:
>
>     if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
>         GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
>         regs = NULL;
>         num = ARRAY_SIZE(xehp_regs);
>     } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
>            GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
>         regs = gen12_regs;
>         num = ARRAY_SIZE(gen12_regs);
>
> ?

For me this perfectly follows the 'strict' approach :)

>
> It's fine by me if that covers all currently known platforms.

My grep in i915_pci.c agrees.

Regards
Andrzej

>
> Regards,
>
> Tvrtko