[PATCH 00/10] accel/ivpu fixes for 6.8

[PATCH 00/10] accel/ivpu fixes for 6.8

[Jacek Lawrynowicz]

Various driver fixes:
 - Fixes for infinite loops, missing locks and DMA-API debug warnings
 - Deprecate DRM_IVPU_PARAM_CONTEXT_PRIORITY
 - Improve diagnostic messages

Jacek Lawrynowicz (5):
  accel/ivpu: Fix potential infinite loops in IRQ handlers
  accel/ivpu: Fix for missing lock around drm_gem_shmem_vmap()
  accel/ivpu: Free buffer sgt on unbind
  accel/ivpu: Disable buffer sharing among VPU contexts
  accel/ivpu: Improve buffer object debug logs

Wachowski, Karol (5):
  accel/ivpu: Dump MMU events in case of VPU boot timeout
  accel/ivpu: Call diagnose failure in ivpu_mmu_cmdq_sync()
  accel/ivpu: Add debug prints for MMU map/unmap operations
  accel/ivpu: Add diagnostic messages when VPU fails to boot or suspend
  accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY

 drivers/accel/ivpu/ivpu_drv.c         |  17 +---
 drivers/accel/ivpu/ivpu_drv.h         |   2 +-
 drivers/accel/ivpu/ivpu_gem.c         | 133 +++++++++-----------------
 drivers/accel/ivpu/ivpu_gem.h         |   1 -
 drivers/accel/ivpu/ivpu_ipc.c         |   6 ++
 drivers/accel/ivpu/ivpu_job.c         |   3 +
 drivers/accel/ivpu/ivpu_mmu.c         |  29 ++++--
 drivers/accel/ivpu/ivpu_mmu.h         |   1 +
 drivers/accel/ivpu/ivpu_mmu_context.c |   9 ++
 drivers/accel/ivpu/ivpu_pm.c          |   4 +-
 include/uapi/drm/ivpu_accel.h         |  21 +++-
 11 files changed, 114 insertions(+), 112 deletions(-)

--
2.43.0

[PATCH 01/10] accel/ivpu: Dump MMU events in case of VPU boot timeout

[Jacek Lawrynowicz]

From: "Wachowski, Karol" <karol.wachowski@intel.com>

Add ivpu_mmu_evtq_dump() function that dumps existing MMU events from
MMU event queue. Call this function if VPU boot failed.

Previously MMU events were only checked in interrupt handler, but if VPU
failed to boot due to MMU faults, those faults were missed because of
interrupts not yet being enabled. This will allow checking potential
fault reason of VPU not booting.

Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_drv.c | 1 +
 drivers/accel/ivpu/ivpu_mmu.c | 8 ++++++++
 drivers/accel/ivpu/ivpu_mmu.h | 1 +
 3 files changed, 10 insertions(+)

diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index 64927682161b..0c3180411b0e 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -369,6 +369,7 @@ int ivpu_boot(struct ivpu_device *vdev)
 	ret = ivpu_wait_for_ready(vdev);
 	if (ret) {
 		ivpu_err(vdev, "Failed to boot the firmware: %d\n", ret);
+		ivpu_mmu_evtq_dump(vdev);
 		return ret;
 	}
 
diff --git a/drivers/accel/ivpu/ivpu_mmu.c b/drivers/accel/ivpu/ivpu_mmu.c
index 2228c44b115f..92ef651098d8 100644
--- a/drivers/accel/ivpu/ivpu_mmu.c
+++ b/drivers/accel/ivpu/ivpu_mmu.c
@@ -905,6 +905,14 @@ void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev)
 		ivpu_pm_schedule_recovery(vdev);
 }
 
+void ivpu_mmu_evtq_dump(struct ivpu_device *vdev)
+{
+	u32 *event;
+
+	while ((event = ivpu_mmu_get_event(vdev)) != NULL)
+		ivpu_mmu_dump_event(vdev, event);
+}
+
 void ivpu_mmu_irq_gerr_handler(struct ivpu_device *vdev)
 {
 	u32 gerror_val, gerrorn_val, active;
diff --git a/drivers/accel/ivpu/ivpu_mmu.h b/drivers/accel/ivpu/ivpu_mmu.h
index cb551126806b..6fa35c240710 100644
--- a/drivers/accel/ivpu/ivpu_mmu.h
+++ b/drivers/accel/ivpu/ivpu_mmu.h
@@ -46,5 +46,6 @@ int ivpu_mmu_invalidate_tlb(struct ivpu_device *vdev, u16 ssid);
 
 void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev);
 void ivpu_mmu_irq_gerr_handler(struct ivpu_device *vdev);
+void ivpu_mmu_evtq_dump(struct ivpu_device *vdev);
 
 #endif /* __IVPU_MMU_H__ */
-- 
2.43.0

Re: [PATCH 01/10] accel/ivpu: Dump MMU events in case of VPU boot timeout

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <karol.wachowski@intel.com>
> 
> Add ivpu_mmu_evtq_dump() function that dumps existing MMU events from
> MMU event queue. Call this function if VPU boot failed.
> 
> Previously MMU events were only checked in interrupt handler, but if VPU
> failed to boot due to MMU faults, those faults were missed because of
> interrupts not yet being enabled. This will allow checking potential
> fault reason of VPU not booting.
> 
> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

[PATCH 02/10] accel/ivpu: Call diagnose failure in ivpu_mmu_cmdq_sync()

[Jacek Lawrynowicz]

From: "Wachowski, Karol" <karol.wachowski@intel.com>

Check for possible failure reasons in the buttress.
Some errors (like external abort) should have corresponding buttress errors
registers set indicating the real reason of failure.

Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/accel/ivpu/ivpu_mmu.c b/drivers/accel/ivpu/ivpu_mmu.c
index 92ef651098d8..1f813625aab3 100644
--- a/drivers/accel/ivpu/ivpu_mmu.c
+++ b/drivers/accel/ivpu/ivpu_mmu.c
@@ -7,6 +7,7 @@
 #include <linux/highmem.h>
 
 #include "ivpu_drv.h"
+#include "ivpu_hw.h"
 #include "ivpu_hw_reg_io.h"
 #include "ivpu_mmu.h"
 #include "ivpu_mmu_context.h"
@@ -518,6 +519,7 @@ static int ivpu_mmu_cmdq_sync(struct ivpu_device *vdev)
 
 		ivpu_err(vdev, "Timed out waiting for MMU consumer: %d, error: %s\n", ret,
 			 ivpu_mmu_cmdq_err_to_str(err));
+		ivpu_hw_diagnose_failure(vdev);
 	}
 
 	return ret;
-- 
2.43.0

Re: [PATCH 02/10] accel/ivpu: Call diagnose failure in ivpu_mmu_cmdq_sync()

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <karol.wachowski@intel.com>
> 
> Check for possible failure reasons in the buttress.
> Some errors (like external abort) should have corresponding buttress errors
> registers set indicating the real reason of failure.
> 
> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

[PATCH 03/10] accel/ivpu: Add debug prints for MMU map/unmap operations

[Jacek Lawrynowicz]

From: "Wachowski, Karol" <karol.wachowski@intel.com>

It is common need to be able to  see IOVA/physical to VPU addresses
mappings. Especially when debugging different kind of memory related
issues. Lack of such logs forces user to modify and recompile KMD manually.

This commit adds those logs under MMU debug mask which can be turned on
dynamically with module param during KMD load.

Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_drv.h         | 1 +
 drivers/accel/ivpu/ivpu_mmu_context.c | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
index ebc4b84f27b2..9b6e336626e3 100644
--- a/drivers/accel/ivpu/ivpu_drv.h
+++ b/drivers/accel/ivpu/ivpu_drv.h
@@ -56,6 +56,7 @@
 #define IVPU_DBG_JSM	 BIT(10)
 #define IVPU_DBG_KREF	 BIT(11)
 #define IVPU_DBG_RPM	 BIT(12)
+#define IVPU_DBG_MMU_MAP BIT(13)
 
 #define ivpu_err(vdev, fmt, ...) \
 	drm_err(&(vdev)->drm, "%s(): " fmt, __func__, ##__VA_ARGS__)
diff --git a/drivers/accel/ivpu/ivpu_mmu_context.c b/drivers/accel/ivpu/ivpu_mmu_context.c
index 12a8c09d4547..fe6161299236 100644
--- a/drivers/accel/ivpu/ivpu_mmu_context.c
+++ b/drivers/accel/ivpu/ivpu_mmu_context.c
@@ -355,6 +355,9 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx,
 		dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
 		size_t size = sg_dma_len(sg) + sg->offset;
 
+		ivpu_dbg(vdev, MMU_MAP, "Map ctx: %u dma_addr: 0x%llx vpu_addr: 0x%llx size: %lu\n",
+			 ctx->id, dma_addr, vpu_addr, size);
+
 		ret = ivpu_mmu_context_map_pages(vdev, ctx, vpu_addr, dma_addr, size, prot);
 		if (ret) {
 			ivpu_err(vdev, "Failed to map context pages\n");
@@ -366,6 +369,7 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx,
 
 	/* Ensure page table modifications are flushed from wc buffers to memory */
 	wmb();
+
 	mutex_unlock(&ctx->lock);
 
 	ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
@@ -388,14 +392,19 @@ ivpu_mmu_context_unmap_sgt(struct ivpu_device *vdev, struct ivpu_mmu_context *ct
 	mutex_lock(&ctx->lock);
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
+		dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
 		size_t size = sg_dma_len(sg) + sg->offset;
 
+		ivpu_dbg(vdev, MMU_MAP, "Unmap ctx: %u dma_addr: 0x%llx vpu_addr: 0x%llx size: %lu\n",
+			 ctx->id, dma_addr, vpu_addr, size);
+
 		ivpu_mmu_context_unmap_pages(ctx, vpu_addr, size);
 		vpu_addr += size;
 	}
 
 	/* Ensure page table modifications are flushed from wc buffers to memory */
 	wmb();
+
 	mutex_unlock(&ctx->lock);
 
 	ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
-- 
2.43.0

Re: [PATCH 03/10] accel/ivpu: Add debug prints for MMU map/unmap operations

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <karol.wachowski@intel.com>
> 
> It is common need to be able to  see IOVA/physical to VPU addresses

Errant double space between "to" and "see"

> mappings. Especially when debugging different kind of memory related
> issues. Lack of such logs forces user to modify and recompile KMD manually.
> 
> This commit adds those logs under MMU debug mask which can be turned on
> dynamically with module param during KMD load.
As far as I understand, the preference is to not expose any kind of raw 
addresses as it is seen as a security issue, and usually the addresses 
don't have any real value to someone reading logs, etc.  I beleive I 
picked this up from GregKH.

However, this commit text suggests there is value, and I see that one 
needs to be root to enable this which could probably be considered a 
sufficent gate to avoiding the data getting into the wrong hands.

Is it possible to provide more details as a justification for this? 
Perhaps an example of a past issue where this data was necessary for debug?

> 
> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_drv.h         | 1 +
>   drivers/accel/ivpu/ivpu_mmu_context.c | 9 +++++++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> index ebc4b84f27b2..9b6e336626e3 100644
> --- a/drivers/accel/ivpu/ivpu_drv.h
> +++ b/drivers/accel/ivpu/ivpu_drv.h
> @@ -56,6 +56,7 @@
>   #define IVPU_DBG_JSM	 BIT(10)
>   #define IVPU_DBG_KREF	 BIT(11)
>   #define IVPU_DBG_RPM	 BIT(12)
> +#define IVPU_DBG_MMU_MAP BIT(13)
>   
>   #define ivpu_err(vdev, fmt, ...) \
>   	drm_err(&(vdev)->drm, "%s(): " fmt, __func__, ##__VA_ARGS__)
> diff --git a/drivers/accel/ivpu/ivpu_mmu_context.c b/drivers/accel/ivpu/ivpu_mmu_context.c
> index 12a8c09d4547..fe6161299236 100644
> --- a/drivers/accel/ivpu/ivpu_mmu_context.c
> +++ b/drivers/accel/ivpu/ivpu_mmu_context.c
> @@ -355,6 +355,9 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx,
>   		dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
>   		size_t size = sg_dma_len(sg) + sg->offset;
>   
> +		ivpu_dbg(vdev, MMU_MAP, "Map ctx: %u dma_addr: 0x%llx vpu_addr: 0x%llx size: %lu\n",
> +			 ctx->id, dma_addr, vpu_addr, size);
> +
>   		ret = ivpu_mmu_context_map_pages(vdev, ctx, vpu_addr, dma_addr, size, prot);
>   		if (ret) {
>   			ivpu_err(vdev, "Failed to map context pages\n");
> @@ -366,6 +369,7 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx,
>   
>   	/* Ensure page table modifications are flushed from wc buffers to memory */
>   	wmb();
> +

This looks like an unrelated whitespace change (although I see it pairs 
with the whitespace change below).

>   	mutex_unlock(&ctx->lock);
>   
>   	ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
> @@ -388,14 +392,19 @@ ivpu_mmu_context_unmap_sgt(struct ivpu_device *vdev, struct ivpu_mmu_context *ct
>   	mutex_lock(&ctx->lock);
>   
>   	for_each_sgtable_dma_sg(sgt, sg, i) {
> +		dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
>   		size_t size = sg_dma_len(sg) + sg->offset;
>   
> +		ivpu_dbg(vdev, MMU_MAP, "Unmap ctx: %u dma_addr: 0x%llx vpu_addr: 0x%llx size: %lu\n",
> +			 ctx->id, dma_addr, vpu_addr, size);
> +
>   		ivpu_mmu_context_unmap_pages(ctx, vpu_addr, size);
>   		vpu_addr += size;
>   	}
>   
>   	/* Ensure page table modifications are flushed from wc buffers to memory */
>   	wmb();
> +

This looks like an unrelated whitespace change.

>   	mutex_unlock(&ctx->lock);
>   
>   	ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);

Re: [PATCH 03/10] accel/ivpu: Add debug prints for MMU map/unmap operations

[Jacek Lawrynowicz]

On 05.01.2024 16:32, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> From: "Wachowski, Karol" <karol.wachowski@intel.com>
>>
>> It is common need to be able to  see IOVA/physical to VPU addresses
> 
> Errant double space between "to" and "see"

Sure

>> mappings. Especially when debugging different kind of memory related
>> issues. Lack of such logs forces user to modify and recompile KMD manually.
>>
>> This commit adds those logs under MMU debug mask which can be turned on
>> dynamically with module param during KMD load.
> As far as I understand, the preference is to not expose any kind of raw addresses as it is seen as a security issue, and usually the addresses don't have any real value to someone reading logs, etc.  I beleive I picked this up from GregKH.
> 
> However, this commit text suggests there is value, and I see that one needs to be root to enable this which could probably be considered a sufficent gate to avoiding the data getting into the wrong hands.
> 
> Is it possible to provide more details as a justification for this? Perhaps an example of a past issue where this data was necessary for debug?

There were multiple occasions were these messages were useful:
  - Debugging IOMMU issues on pre-production hardware
  - Enabling DevTLB cache on a functional simulator
  - Debugging performance issues with fragmented buffers

There is always some security tradeoff when enabling debug features but in this case it seems to be worth it.

[PATCH 04/10] accel/ivpu: Add diagnostic messages when VPU fails to boot or suspend

[Jacek Lawrynowicz]

From: "Wachowski, Karol" <karol.wachowski@intel.com>

Make boot/suspend failure debugging easier by dumping FW logs and error
registers.

Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_drv.c | 5 +++--
 drivers/accel/ivpu/ivpu_pm.c  | 4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index 0c3180411b0e..ec66c2c39877 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -17,6 +17,7 @@
 #include "ivpu_debugfs.h"
 #include "ivpu_drv.h"
 #include "ivpu_fw.h"
+#include "ivpu_fw_log.h"
 #include "ivpu_gem.h"
 #include "ivpu_hw.h"
 #include "ivpu_ipc.h"
@@ -340,8 +341,6 @@ static int ivpu_wait_for_ready(struct ivpu_device *vdev)
 
 	if (!ret)
 		ivpu_dbg(vdev, PM, "VPU ready message received successfully\n");
-	else
-		ivpu_hw_diagnose_failure(vdev);
 
 	return ret;
 }
@@ -369,7 +368,9 @@ int ivpu_boot(struct ivpu_device *vdev)
 	ret = ivpu_wait_for_ready(vdev);
 	if (ret) {
 		ivpu_err(vdev, "Failed to boot the firmware: %d\n", ret);
+		ivpu_hw_diagnose_failure(vdev);
 		ivpu_mmu_evtq_dump(vdev);
+		ivpu_fw_log_dump(vdev);
 		return ret;
 	}
 
diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c
index 0af8864cb3b5..8407f1d8c99c 100644
--- a/drivers/accel/ivpu/ivpu_pm.c
+++ b/drivers/accel/ivpu/ivpu_pm.c
@@ -13,6 +13,7 @@
 #include "ivpu_drv.h"
 #include "ivpu_hw.h"
 #include "ivpu_fw.h"
+#include "ivpu_fw_log.h"
 #include "ivpu_ipc.h"
 #include "ivpu_job.h"
 #include "ivpu_jsm_msg.h"
@@ -247,7 +248,8 @@ int ivpu_pm_runtime_suspend_cb(struct device *dev)
 		ivpu_err(vdev, "Failed to set suspend VPU: %d\n", ret);
 
 	if (!hw_is_idle) {
-		ivpu_warn(vdev, "VPU failed to enter idle, force suspended.\n");
+		ivpu_err(vdev, "VPU failed to enter idle, force suspended.\n");
+		ivpu_fw_log_dump(vdev);
 		ivpu_pm_prepare_cold_boot(vdev);
 	} else {
 		ivpu_pm_prepare_warm_boot(vdev);
-- 
2.43.0

Re: [PATCH 04/10] accel/ivpu: Add diagnostic messages when VPU fails to boot or suspend

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <karol.wachowski@intel.com>
> 
> Make boot/suspend failure debugging easier by dumping FW logs and error
> registers.
> 
> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

[PATCH 05/10] accel/ivpu: Fix potential infinite loops in IRQ handlers

[Jacek Lawrynowicz]

Limit number of iterations in ivpu_mmu_irq_evtq_handler() and
ivpu_ipc_irq_handler().

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_ipc.c |  6 ++++++
 drivers/accel/ivpu/ivpu_mmu.c | 21 +++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c
index e86621f16f85..f69780248803 100644
--- a/drivers/accel/ivpu/ivpu_ipc.c
+++ b/drivers/accel/ivpu/ivpu_ipc.c
@@ -389,12 +389,18 @@ void ivpu_ipc_irq_handler(struct ivpu_device *vdev, bool *wake_thread)
 	unsigned long flags;
 	bool dispatched;
 	u32 vpu_addr;
+	int msg_count = 0;
 
 	/*
 	 * Driver needs to purge all messages from IPC FIFO to clear IPC interrupt.
 	 * Without purge IPC FIFO to 0 next IPC interrupts won't be generated.
 	 */
 	while (ivpu_hw_reg_ipc_rx_count_get(vdev)) {
+		if (++msg_count > IPC_MAX_RX_MSG) {
+			ivpu_pm_schedule_recovery(vdev);
+			return;
+		}
+
 		vpu_addr = ivpu_hw_reg_ipc_rx_addr_get(vdev);
 		if (vpu_addr == REG_IO_ERROR) {
 			ivpu_err_ratelimited(vdev, "Failed to read IPC rx addr register\n");
diff --git a/drivers/accel/ivpu/ivpu_mmu.c b/drivers/accel/ivpu/ivpu_mmu.c
index 1f813625aab3..c82929b0ae9d 100644
--- a/drivers/accel/ivpu/ivpu_mmu.c
+++ b/drivers/accel/ivpu/ivpu_mmu.c
@@ -236,6 +236,8 @@
 #define IVPU_MMU_CERROR_ABT          0x2
 #define IVPU_MMU_CERROR_ATC_INV_SYNC 0x3
 
+#define IVPU_MMU_MAX_EVENT_COUNT     100
+
 static const char *ivpu_mmu_event_to_str(u32 cmd)
 {
 	switch (cmd) {
@@ -887,7 +889,7 @@ static u32 *ivpu_mmu_get_event(struct ivpu_device *vdev)
 
 void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev)
 {
-	bool schedule_recovery = false;
+	int event_count = 0;
 	u32 *event;
 	u32 ssid;
 
@@ -895,16 +897,19 @@ void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev)
 
 	while ((event = ivpu_mmu_get_event(vdev)) != NULL) {
 		ivpu_mmu_dump_event(vdev, event);
+		if (++event_count > IVPU_MMU_MAX_EVENT_COUNT) {
+			ivpu_pm_schedule_recovery(vdev);
+			return;
+		}
 
 		ssid = FIELD_GET(IVPU_MMU_EVT_SSID_MASK, event[0]);
-		if (ssid == IVPU_GLOBAL_CONTEXT_MMU_SSID)
-			schedule_recovery = true;
-		else
-			ivpu_mmu_user_context_mark_invalid(vdev, ssid);
-	}
+		if (ssid == IVPU_GLOBAL_CONTEXT_MMU_SSID) {
+			ivpu_pm_schedule_recovery(vdev);
+			return;
+		}
 
-	if (schedule_recovery)
-		ivpu_pm_schedule_recovery(vdev);
+		ivpu_mmu_user_context_mark_invalid(vdev, ssid);
+	}
 }
 
 void ivpu_mmu_evtq_dump(struct ivpu_device *vdev)
-- 
2.43.0

Re: [PATCH 05/10] accel/ivpu: Fix potential infinite loops in IRQ handlers

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> Limit number of iterations in ivpu_mmu_irq_evtq_handler() and
> ivpu_ipc_irq_handler().

"potential infinite loops" sounds like something that has not been 
observed.  Has a problem actually occurred?

Are you concerned that the FW is broken and spamming Linux with events?

Why a limit of 100 events?  Seems arbitrary.

I suspect threaded irqs might be useful here, but it is hard to tell.

> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_ipc.c |  6 ++++++
>   drivers/accel/ivpu/ivpu_mmu.c | 21 +++++++++++++--------
>   2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_ipc.c b/drivers/accel/ivpu/ivpu_ipc.c
> index e86621f16f85..f69780248803 100644
> --- a/drivers/accel/ivpu/ivpu_ipc.c
> +++ b/drivers/accel/ivpu/ivpu_ipc.c
> @@ -389,12 +389,18 @@ void ivpu_ipc_irq_handler(struct ivpu_device *vdev, bool *wake_thread)
>   	unsigned long flags;
>   	bool dispatched;
>   	u32 vpu_addr;
> +	int msg_count = 0;
>   
>   	/*
>   	 * Driver needs to purge all messages from IPC FIFO to clear IPC interrupt.
>   	 * Without purge IPC FIFO to 0 next IPC interrupts won't be generated.
>   	 */
>   	while (ivpu_hw_reg_ipc_rx_count_get(vdev)) {
> +		if (++msg_count > IPC_MAX_RX_MSG) {
> +			ivpu_pm_schedule_recovery(vdev);
> +			return;
> +		}
> +
>   		vpu_addr = ivpu_hw_reg_ipc_rx_addr_get(vdev);
>   		if (vpu_addr == REG_IO_ERROR) {
>   			ivpu_err_ratelimited(vdev, "Failed to read IPC rx addr register\n");
> diff --git a/drivers/accel/ivpu/ivpu_mmu.c b/drivers/accel/ivpu/ivpu_mmu.c
> index 1f813625aab3..c82929b0ae9d 100644
> --- a/drivers/accel/ivpu/ivpu_mmu.c
> +++ b/drivers/accel/ivpu/ivpu_mmu.c
> @@ -236,6 +236,8 @@
>   #define IVPU_MMU_CERROR_ABT          0x2
>   #define IVPU_MMU_CERROR_ATC_INV_SYNC 0x3
>   
> +#define IVPU_MMU_MAX_EVENT_COUNT     100
> +
>   static const char *ivpu_mmu_event_to_str(u32 cmd)
>   {
>   	switch (cmd) {
> @@ -887,7 +889,7 @@ static u32 *ivpu_mmu_get_event(struct ivpu_device *vdev)
>   
>   void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev)
>   {
> -	bool schedule_recovery = false;
> +	int event_count = 0;
>   	u32 *event;
>   	u32 ssid;
>   
> @@ -895,16 +897,19 @@ void ivpu_mmu_irq_evtq_handler(struct ivpu_device *vdev)
>   
>   	while ((event = ivpu_mmu_get_event(vdev)) != NULL) {
>   		ivpu_mmu_dump_event(vdev, event);
> +		if (++event_count > IVPU_MMU_MAX_EVENT_COUNT) {
> +			ivpu_pm_schedule_recovery(vdev);
> +			return;
> +		}
>   
>   		ssid = FIELD_GET(IVPU_MMU_EVT_SSID_MASK, event[0]);
> -		if (ssid == IVPU_GLOBAL_CONTEXT_MMU_SSID)
> -			schedule_recovery = true;
> -		else
> -			ivpu_mmu_user_context_mark_invalid(vdev, ssid);
> -	}
> +		if (ssid == IVPU_GLOBAL_CONTEXT_MMU_SSID) {
> +			ivpu_pm_schedule_recovery(vdev);
> +			return;
> +		}
>   
> -	if (schedule_recovery)
> -		ivpu_pm_schedule_recovery(vdev);
> +		ivpu_mmu_user_context_mark_invalid(vdev, ssid);
> +	}
>   }
>   
>   void ivpu_mmu_evtq_dump(struct ivpu_device *vdev)

Re: [PATCH 05/10] accel/ivpu: Fix potential infinite loops in IRQ handlers

[Jacek Lawrynowicz]

On 05.01.2024 17:35, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> Limit number of iterations in ivpu_mmu_irq_evtq_handler() and
>> ivpu_ipc_irq_handler().
> 
> "potential infinite loops" sounds like something that has not been observed.  Has a problem actually occurred?
> 
> Are you concerned that the FW is broken and spamming Linux with events?
> 
> Why a limit of 100 events?  Seems arbitrary.
> 
> I suspect threaded irqs might be useful here, but it is hard to tell.

No, these problems were not observed yet.
I think that the small size of the IPC/MMU queues and relatively fast host CPU prevent the host from being overloaded.
Maybe let's drop this patch until there is a real issue then :)

[PATCH 06/10] accel/ivpu: Fix for missing lock around drm_gem_shmem_vmap()

[Jacek Lawrynowicz]

drm_gem_shmem_vmap/vunmap requires dma resv lock to be held.
This was missed during conversion to shmem helper.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_gem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
index 1dda4f38ea25..6890d33cf352 100644
--- a/drivers/accel/ivpu/ivpu_gem.c
+++ b/drivers/accel/ivpu/ivpu_gem.c
@@ -361,7 +361,9 @@ ivpu_bo_alloc_internal(struct ivpu_device *vdev, u64 vpu_addr, u64 size, u32 fla
 	if (ret)
 		goto err_put;
 
+	dma_resv_lock(bo->base.base.resv, NULL);
 	ret = drm_gem_shmem_vmap(&bo->base, &map);
+	dma_resv_unlock(bo->base.base.resv);
 	if (ret)
 		goto err_put;
 
@@ -376,7 +378,10 @@ void ivpu_bo_free_internal(struct ivpu_bo *bo)
 {
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(bo->base.vaddr);
 
+	dma_resv_lock(bo->base.base.resv, NULL);
 	drm_gem_shmem_vunmap(&bo->base, &map);
+	dma_resv_unlock(bo->base.base.resv);
+
 	drm_gem_object_put(&bo->base.base);
 }
 
-- 
2.43.0

Re: [PATCH 06/10] accel/ivpu: Fix for missing lock around drm_gem_shmem_vmap()

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> drm_gem_shmem_vmap/vunmap requires dma resv lock to be held.
> This was missed during conversion to shmem helper.
>  > Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Fixes tag?

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Re: [PATCH 06/10] accel/ivpu: Fix for missing lock around drm_gem_shmem_vmap()

[Jacek Lawrynowicz]

On 05.01.2024 17:36, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> drm_gem_shmem_vmap/vunmap requires dma resv lock to be held.
>> This was missed during conversion to shmem helper.
>>  > Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> 
> Fixes tag?

Sure, I'll add it.

[PATCH 07/10] accel/ivpu: Free buffer sgt on unbind

[Jacek Lawrynowicz]

Call dma_unmap() on all buffers before to VPU is unbinded to avoid
"device driver has pending DMA allocations while released from device"
warning when DMA-API debug is enabled.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_gem.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
index 6890d33cf352..4de454bfbf91 100644
--- a/drivers/accel/ivpu/ivpu_gem.c
+++ b/drivers/accel/ivpu/ivpu_gem.c
@@ -112,8 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
 
 	ivpu_dbg_bo(vdev, bo, "unbind");
 
-	/* TODO: dma_unmap */
-
 	if (bo->mmu_mapped) {
 		drm_WARN_ON(&vdev->drm, !bo->ctx);
 		drm_WARN_ON(&vdev->drm, !bo->vpu_addr);
@@ -127,6 +125,18 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
 		bo->vpu_addr = 0;
 		bo->ctx = NULL;
 	}
+
+	if (bo->base.base.import_attach)
+		return;
+
+	dma_resv_lock(bo->base.base.resv, NULL);
+	if (bo->base.sgt) {
+		dma_unmap_sgtable(vdev->drm.dev, bo->base.sgt, DMA_BIDIRECTIONAL, 0);
+		sg_free_table(bo->base.sgt);
+		kfree(bo->base.sgt);
+		bo->base.sgt = NULL;
+	}
+	dma_resv_unlock(bo->base.base.resv);
 }
 
 static void ivpu_bo_unbind(struct ivpu_bo *bo)
-- 
2.43.0

Re: [PATCH 07/10] accel/ivpu: Free buffer sgt on unbind

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> Call dma_unmap() on all buffers before to VPU is unbinded to avoid

to -> the ?

> "device driver has pending DMA allocations while released from device"
> warning when DMA-API debug is enabled.
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

[PATCH 08/10] accel/ivpu: Disable buffer sharing among VPU contexts

[Jacek Lawrynowicz]

This was not supported properly. A buffer was imported to another VPU
context as a separate buffer object with duplicated sgt.
Both exported and imported buffers could be DMA mapped causing a double
mapping on the same device.

Imported buffer from another VPU context will now have just reference
increased and there will be a single sgt fixing above problem but
buffers still can't be shared among VPU contexts because each context
have its own MMU mapping and ivpu_bo supports only single MMU mapping.

The solution would be to use a mapping list as in panfrost or etnaviv
drivers and it will be implemented in future if required.

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_gem.c | 44 +++++------------------------------
 1 file changed, 6 insertions(+), 38 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
index 4de454bfbf91..8cb4d337552e 100644
--- a/drivers/accel/ivpu/ivpu_gem.c
+++ b/drivers/accel/ivpu/ivpu_gem.c
@@ -222,6 +222,12 @@ static int ivpu_bo_open(struct drm_gem_object *obj, struct drm_file *file)
 	struct ivpu_bo *bo = to_ivpu_bo(obj);
 	struct ivpu_addr_range *range;
 
+	if (bo->ctx) {
+		ivpu_warn(vdev, "Can't add BO (vpu_addr 0x%llx) to ctx %u: already in ctx %u\n",
+			  bo->vpu_addr, file_priv->ctx.id, bo->ctx->id);
+		return -EALREADY;
+	}
+
 	if (bo->flags & DRM_IVPU_BO_SHAVE_MEM)
 		range = &vdev->hw->ranges.shave;
 	else if (bo->flags & DRM_IVPU_BO_DMA_MEM)
@@ -252,47 +258,9 @@ static void ivpu_bo_free(struct drm_gem_object *obj)
 	drm_gem_shmem_free(&bo->base);
 }
 
-static const struct dma_buf_ops ivpu_bo_dmabuf_ops =  {
-	.cache_sgt_mapping = true,
-	.attach = drm_gem_map_attach,
-	.detach = drm_gem_map_detach,
-	.map_dma_buf = drm_gem_map_dma_buf,
-	.unmap_dma_buf = drm_gem_unmap_dma_buf,
-	.release = drm_gem_dmabuf_release,
-	.mmap = drm_gem_dmabuf_mmap,
-	.vmap = drm_gem_dmabuf_vmap,
-	.vunmap = drm_gem_dmabuf_vunmap,
-};
-
-static struct dma_buf *ivpu_bo_export(struct drm_gem_object *obj, int flags)
-{
-	struct drm_device *dev = obj->dev;
-	struct dma_buf_export_info exp_info = {
-		.exp_name = KBUILD_MODNAME,
-		.owner = dev->driver->fops->owner,
-		.ops = &ivpu_bo_dmabuf_ops,
-		.size = obj->size,
-		.flags = flags,
-		.priv = obj,
-		.resv = obj->resv,
-	};
-	void *sgt;
-
-	/*
-	 * Make sure that pages are allocated and dma-mapped before exporting the bo.
-	 * DMA-mapping is required if the bo will be imported to the same device.
-	 */
-	sgt = drm_gem_shmem_get_pages_sgt(to_drm_gem_shmem_obj(obj));
-	if (IS_ERR(sgt))
-		return sgt;
-
-	return drm_gem_dmabuf_export(dev, &exp_info);
-}
-
 static const struct drm_gem_object_funcs ivpu_gem_funcs = {
 	.free = ivpu_bo_free,
 	.open = ivpu_bo_open,
-	.export = ivpu_bo_export,
 	.print_info = drm_gem_shmem_object_print_info,
 	.pin = drm_gem_shmem_object_pin,
 	.unpin = drm_gem_shmem_object_unpin,
-- 
2.43.0

Re: [PATCH 08/10] accel/ivpu: Disable buffer sharing among VPU contexts

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> This was not supported properly. A buffer was imported to another VPU
> context as a separate buffer object with duplicated sgt.
> Both exported and imported buffers could be DMA mapped causing a double
> mapping on the same device.
> 
> Imported buffer from another VPU context will now have just reference
> increased and there will be a single sgt fixing above problem but
> buffers still can't be shared among VPU contexts because each context
> have its own MMU mapping and ivpu_bo supports only single MMU mapping.
> 
> The solution would be to use a mapping list as in panfrost or etnaviv
> drivers and it will be implemented in future if required.
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_gem.c | 44 +++++------------------------------
>   1 file changed, 6 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
> index 4de454bfbf91..8cb4d337552e 100644
> --- a/drivers/accel/ivpu/ivpu_gem.c
> +++ b/drivers/accel/ivpu/ivpu_gem.c
> @@ -222,6 +222,12 @@ static int ivpu_bo_open(struct drm_gem_object *obj, struct drm_file *file)
>   	struct ivpu_bo *bo = to_ivpu_bo(obj);
>   	struct ivpu_addr_range *range;
>   
> +	if (bo->ctx) {
> +		ivpu_warn(vdev, "Can't add BO (vpu_addr 0x%llx) to ctx %u: already in ctx %u\n",
> +			  bo->vpu_addr, file_priv->ctx.id, bo->ctx->id);

Looks like the vpu_addr is being used as a unique identifier for the BO. 
  Is that really the best value to use?  Seems like if I want to attack 
another context, knowing the device address of something that context 
owns would be useful information.

Re: [PATCH 08/10] accel/ivpu: Disable buffer sharing among VPU contexts

[Jacek Lawrynowicz]

On 05.01.2024 17:46, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> This was not supported properly. A buffer was imported to another VPU
>> context as a separate buffer object with duplicated sgt.
>> Both exported and imported buffers could be DMA mapped causing a double
>> mapping on the same device.
>>
>> Imported buffer from another VPU context will now have just reference
>> increased and there will be a single sgt fixing above problem but
>> buffers still can't be shared among VPU contexts because each context
>> have its own MMU mapping and ivpu_bo supports only single MMU mapping.
>>
>> The solution would be to use a mapping list as in panfrost or etnaviv
>> drivers and it will be implemented in future if required.
>>
>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
>> ---
>>   drivers/accel/ivpu/ivpu_gem.c | 44 +++++------------------------------
>>   1 file changed, 6 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
>> index 4de454bfbf91..8cb4d337552e 100644
>> --- a/drivers/accel/ivpu/ivpu_gem.c
>> +++ b/drivers/accel/ivpu/ivpu_gem.c
>> @@ -222,6 +222,12 @@ static int ivpu_bo_open(struct drm_gem_object *obj, struct drm_file *file)
>>       struct ivpu_bo *bo = to_ivpu_bo(obj);
>>       struct ivpu_addr_range *range;
>>   +    if (bo->ctx) {
>> +        ivpu_warn(vdev, "Can't add BO (vpu_addr 0x%llx) to ctx %u: already in ctx %u\n",
>> +              bo->vpu_addr, file_priv->ctx.id, bo->ctx->id);
> 
> Looks like the vpu_addr is being used as a unique identifier for the BO.  Is that really the best value to use?  Seems like if I want to attack another context, knowing the device address of something that context owns would be useful information.

OK, I'll remove vpu_addr form the message.

Re: [PATCH 08/10] accel/ivpu: Disable buffer sharing among VPU contexts

[Carl Vanderlip]

On 1/5/2024 3:22 AM, Jacek Lawrynowicz wrote:
> Imported buffer from another VPU context will now have just reference
> increased and there will be a single sgt fixing above problem but
> buffers still can't be shared among VPU contexts because each context
> have its own MMU mapping and ivpu_bo supports only single MMU mapping.

This paragraph/sentence needs rewrite. Here's my take...

"
Buffers imported from another VPU context will now just increase 
reference count, leaving only a single sgt, fixing the problem above.
Buffers still can't be shared among VPU contexts because each has its 
own MMU mapping and ivpu_bo only supports single MMU mappings.
"

-Carl V.

Re: [PATCH 08/10] accel/ivpu: Disable buffer sharing among VPU contexts

[Jacek Lawrynowicz]

On 05.01.2024 23:34, Carl Vanderlip wrote:
> 
> On 1/5/2024 3:22 AM, Jacek Lawrynowicz wrote:
>> Imported buffer from another VPU context will now have just reference
>> increased and there will be a single sgt fixing above problem but
>> buffers still can't be shared among VPU contexts because each context
>> have its own MMU mapping and ivpu_bo supports only single MMU mapping.
> 
> This paragraph/sentence needs rewrite. Here's my take...
> 
> "
> Buffers imported from another VPU context will now just increase reference count, leaving only a single sgt, fixing the problem above.
> Buffers still can't be shared among VPU contexts because each has its own MMU mapping and ivpu_bo only supports single MMU mappings.

Perfect, thanks!

[PATCH 09/10] accel/ivpu: Improve buffer object debug logs

[Jacek Lawrynowicz]

Make debug logs more readable and consistent:
  - don't print handle as it is not always available for all buffers
  - use hashed ivpu_bo ptr as main buffer identifier
  - remove unused fields from ivpu_bo_print_info()

Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_gem.c | 72 +++++++++++------------------------
 drivers/accel/ivpu/ivpu_gem.h |  1 -
 2 files changed, 23 insertions(+), 50 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
index 8cb4d337552e..dd327d7eda0d 100644
--- a/drivers/accel/ivpu/ivpu_gem.c
+++ b/drivers/accel/ivpu/ivpu_gem.c
@@ -24,14 +24,11 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs;
 
 static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action)
 {
-	if (bo->ctx)
-		ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n",
-			 action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt,
-			 bo->handle, bo->ctx->id, bo->vpu_addr, bo->mmu_mapped);
-	else
-		ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n",
-			 action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt,
-			 bo->handle);
+	ivpu_dbg(vdev, BO,
+		 "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n",
+		 action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx ? bo->ctx->id : 0,
+		 (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc,
+		 (bool)bo->base.base.import_attach);
 }
 
 /*
@@ -49,12 +46,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo)
 	mutex_lock(&bo->lock);
 
 	ivpu_dbg_bo(vdev, bo, "pin");
-
-	if (!bo->ctx) {
-		ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle);
-		ret = -EINVAL;
-		goto unlock;
-	}
+	drm_WARN_ON(&vdev->drm, !bo->ctx);
 
 	if (!bo->mmu_mapped) {
 		struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base);
@@ -108,9 +100,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
 {
 	struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);
 
-	lockdep_assert_held(&bo->lock);
-
-	ivpu_dbg_bo(vdev, bo, "unbind");
+	lockdep_assert(lockdep_is_held(&bo->lock) || !kref_read(&bo->base.base.refcount));
 
 	if (bo->mmu_mapped) {
 		drm_WARN_ON(&vdev->drm, !bo->ctx);
@@ -122,7 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
 
 	if (bo->ctx) {
 		ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node);
-		bo->vpu_addr = 0;
 		bo->ctx = NULL;
 	}
 
@@ -139,13 +128,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
 	dma_resv_unlock(bo->base.base.resv);
 }
 
-static void ivpu_bo_unbind(struct ivpu_bo *bo)
-{
-	mutex_lock(&bo->lock);
-	ivpu_bo_unbind_locked(bo);
-	mutex_unlock(&bo->lock);
-}
-
 void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx)
 {
 	struct ivpu_bo *bo;
@@ -156,8 +138,10 @@ void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m
 	mutex_lock(&vdev->bo_list_lock);
 	list_for_each_entry(bo, &vdev->bo_list, bo_list_node) {
 		mutex_lock(&bo->lock);
-		if (bo->ctx == ctx)
+		if (bo->ctx == ctx) {
+			ivpu_dbg_bo(vdev, bo, "unbind");
 			ivpu_bo_unbind_locked(bo);
+		}
 		mutex_unlock(&bo->lock);
 	}
 	mutex_unlock(&vdev->bo_list_lock);
@@ -209,9 +193,6 @@ ivpu_bo_create(struct ivpu_device *vdev, u64 size, u32 flags)
 	list_add_tail(&bo->bo_list_node, &vdev->bo_list);
 	mutex_unlock(&vdev->bo_list_lock);
 
-	ivpu_dbg(vdev, BO, "create: vpu_addr 0x%llx size %zu flags 0x%x\n",
-		 bo->vpu_addr, bo->base.base.size, flags);
-
 	return bo;
 }
 
@@ -243,15 +224,15 @@ static void ivpu_bo_free(struct drm_gem_object *obj)
 	struct ivpu_device *vdev = to_ivpu_device(obj->dev);
 	struct ivpu_bo *bo = to_ivpu_bo(obj);
 
+	ivpu_dbg_bo(vdev, bo, "free");
+
 	mutex_lock(&vdev->bo_list_lock);
 	list_del(&bo->bo_list_node);
 	mutex_unlock(&vdev->bo_list_lock);
 
 	drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ));
 
-	ivpu_dbg_bo(vdev, bo, "free");
-
-	ivpu_bo_unbind(bo);
+	ivpu_bo_unbind_locked(bo);
 	mutex_destroy(&bo->lock);
 
 	drm_WARN_ON(obj->dev, bo->base.pages_use_count > 1);
@@ -293,11 +274,9 @@ int ivpu_bo_create_ioctl(struct drm_device *dev, void *data, struct drm_file *fi
 		return PTR_ERR(bo);
 	}
 
-	ret = drm_gem_handle_create(file, &bo->base.base, &bo->handle);
-	if (!ret) {
+	ret = drm_gem_handle_create(file, &bo->base.base, &args->handle);
+	if (!ret)
 		args->vpu_addr = bo->vpu_addr;
-		args->handle = bo->handle;
-	}
 
 	drm_gem_object_put(&bo->base.base);
 
@@ -415,19 +394,11 @@ int ivpu_bo_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file
 
 static void ivpu_bo_print_info(struct ivpu_bo *bo, struct drm_printer *p)
 {
-	unsigned long dma_refcount = 0;
-
 	mutex_lock(&bo->lock);
 
-	if (bo->base.base.dma_buf && bo->base.base.dma_buf->file)
-		dma_refcount = atomic_long_read(&bo->base.base.dma_buf->file->f_count);
-
-	drm_printf(p, "%-3u %-6d 0x%-12llx %-10lu 0x%-8x %-4u %-8lu",
-		   bo->ctx->id, bo->handle, bo->vpu_addr, bo->base.base.size,
-		   bo->flags, kref_read(&bo->base.base.refcount), dma_refcount);
-
-	if (bo->base.base.import_attach)
-		drm_printf(p, " imported");
+	drm_printf(p, "%-9p %-3u 0x%-12llx %-10lu 0x%-8x %-4u",
+		   bo, bo->ctx->id, bo->vpu_addr, bo->base.base.size,
+		   bo->flags, kref_read(&bo->base.base.refcount));
 
 	if (bo->base.pages)
 		drm_printf(p, " has_pages");
@@ -435,6 +406,9 @@ static void ivpu_bo_print_info(struct ivpu_bo *bo, struct drm_printer *p)
 	if (bo->mmu_mapped)
 		drm_printf(p, " mmu_mapped");
 
+	if (bo->base.base.import_attach)
+		drm_printf(p, " imported");
+
 	drm_printf(p, "\n");
 
 	mutex_unlock(&bo->lock);
@@ -445,8 +419,8 @@ void ivpu_bo_list(struct drm_device *dev, struct drm_printer *p)
 	struct ivpu_device *vdev = to_ivpu_device(dev);
 	struct ivpu_bo *bo;
 
-	drm_printf(p, "%-3s %-6s %-14s %-10s %-10s %-4s %-8s %s\n",
-		   "ctx", "handle", "vpu_addr", "size", "flags", "refs", "dma_refs", "attribs");
+	drm_printf(p, "%-9s %-3s %-14s %-10s %-10s %-4s %s\n",
+		   "bo", "ctx", "vpu_addr", "size", "flags", "refs", "attribs");
 
 	mutex_lock(&vdev->bo_list_lock);
 	list_for_each_entry(bo, &vdev->bo_list, bo_list_node)
diff --git a/drivers/accel/ivpu/ivpu_gem.h b/drivers/accel/ivpu/ivpu_gem.h
index d75cad0d3c74..5cb1dda3e58e 100644
--- a/drivers/accel/ivpu/ivpu_gem.h
+++ b/drivers/accel/ivpu/ivpu_gem.h
@@ -19,7 +19,6 @@ struct ivpu_bo {
 
 	struct mutex lock; /* Protects: ctx, mmu_mapped, vpu_addr */
 	u64 vpu_addr;
-	u32 handle;
 	u32 flags;
 	u32 job_status; /* Valid only for command buffer */
 	bool mmu_mapped;
-- 
2.43.0

Re: [PATCH 09/10] accel/ivpu: Improve buffer object debug logs

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> Make debug logs more readable and consistent:
>    - don't print handle as it is not always available for all buffers
>    - use hashed ivpu_bo ptr as main buffer identifier
>    - remove unused fields from ivpu_bo_print_info()
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_gem.c | 72 +++++++++++------------------------
>   drivers/accel/ivpu/ivpu_gem.h |  1 -
>   2 files changed, 23 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
> index 8cb4d337552e..dd327d7eda0d 100644
> --- a/drivers/accel/ivpu/ivpu_gem.c
> +++ b/drivers/accel/ivpu/ivpu_gem.c
> @@ -24,14 +24,11 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs;
>   
>   static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action)
>   {
> -	if (bo->ctx)
> -		ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n",
> -			 action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt,
> -			 bo->handle, bo->ctx->id, bo->vpu_addr, bo->mmu_mapped);
> -	else
> -		ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n",
> -			 action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt,
> -			 bo->handle);
> +	ivpu_dbg(vdev, BO,
> +		 "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n",
> +		 action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx ? bo->ctx->id : 0,
> +		 (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc,
> +		 (bool)bo->base.base.import_attach);
>   }
>   
>   /*
> @@ -49,12 +46,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo)
>   	mutex_lock(&bo->lock);
>   
>   	ivpu_dbg_bo(vdev, bo, "pin");
> -
> -	if (!bo->ctx) {
> -		ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle);
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> +	drm_WARN_ON(&vdev->drm, !bo->ctx);
>   
>   	if (!bo->mmu_mapped) {
>   		struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base);
> @@ -108,9 +100,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
>   {
>   	struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);
>   
> -	lockdep_assert_held(&bo->lock);
> -
> -	ivpu_dbg_bo(vdev, bo, "unbind");
> +	lockdep_assert(lockdep_is_held(&bo->lock) || !kref_read(&bo->base.base.refcount));
>   
>   	if (bo->mmu_mapped) {
>   		drm_WARN_ON(&vdev->drm, !bo->ctx);
> @@ -122,7 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
>   
>   	if (bo->ctx) {
>   		ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node);
> -		bo->vpu_addr = 0;
>   		bo->ctx = NULL;
>   	}
>   
> @@ -139,13 +128,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
>   	dma_resv_unlock(bo->base.base.resv);
>   }
>   
> -static void ivpu_bo_unbind(struct ivpu_bo *bo)
> -{
> -	mutex_lock(&bo->lock);
> -	ivpu_bo_unbind_locked(bo);
> -	mutex_unlock(&bo->lock);
> -}

This does not seem to be related to $SUBJECT

> -
>   void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx)
>   {
>   	struct ivpu_bo *bo;
> @@ -156,8 +138,10 @@ void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m
>   	mutex_lock(&vdev->bo_list_lock);
>   	list_for_each_entry(bo, &vdev->bo_list, bo_list_node) {
>   		mutex_lock(&bo->lock);
> -		if (bo->ctx == ctx)
> +		if (bo->ctx == ctx) {
> +			ivpu_dbg_bo(vdev, bo, "unbind");
>   			ivpu_bo_unbind_locked(bo);
> +		}
>   		mutex_unlock(&bo->lock);
>   	}
>   	mutex_unlock(&vdev->bo_list_lock);
> @@ -209,9 +193,6 @@ ivpu_bo_create(struct ivpu_device *vdev, u64 size, u32 flags)
>   	list_add_tail(&bo->bo_list_node, &vdev->bo_list);
>   	mutex_unlock(&vdev->bo_list_lock);
>   
> -	ivpu_dbg(vdev, BO, "create: vpu_addr 0x%llx size %zu flags 0x%x\n",
> -		 bo->vpu_addr, bo->base.base.size, flags);
> -
>   	return bo;
>   }
>   
> @@ -243,15 +224,15 @@ static void ivpu_bo_free(struct drm_gem_object *obj)
>   	struct ivpu_device *vdev = to_ivpu_device(obj->dev);
>   	struct ivpu_bo *bo = to_ivpu_bo(obj);
>   
> +	ivpu_dbg_bo(vdev, bo, "free");
> +
>   	mutex_lock(&vdev->bo_list_lock);
>   	list_del(&bo->bo_list_node);
>   	mutex_unlock(&vdev->bo_list_lock);
>   
>   	drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ));
>   
> -	ivpu_dbg_bo(vdev, bo, "free");
> -
> -	ivpu_bo_unbind(bo);
> +	ivpu_bo_unbind_locked(bo);

This does not seem to be related to $SUBJECT

Re: [PATCH 09/10] accel/ivpu: Improve buffer object debug logs

[Jacek Lawrynowicz]

On 05.01.2024 18:03, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> Make debug logs more readable and consistent:
>>    - don't print handle as it is not always available for all buffers
>>    - use hashed ivpu_bo ptr as main buffer identifier
>>    - remove unused fields from ivpu_bo_print_info()
>>
>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
>> ---
>>   drivers/accel/ivpu/ivpu_gem.c | 72 +++++++++++------------------------
>>   drivers/accel/ivpu/ivpu_gem.h |  1 -
>>   2 files changed, 23 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c
>> index 8cb4d337552e..dd327d7eda0d 100644
>> --- a/drivers/accel/ivpu/ivpu_gem.c
>> +++ b/drivers/accel/ivpu/ivpu_gem.c
>> @@ -24,14 +24,11 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs;
>>     static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action)
>>   {
>> -    if (bo->ctx)
>> -        ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u ctx %d vpu_addr 0x%llx mmu_mapped %d\n",
>> -             action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt,
>> -             bo->handle, bo->ctx->id, bo->vpu_addr, bo->mmu_mapped);
>> -    else
>> -        ivpu_dbg(vdev, BO, "%6s: size %zu has_pages %d dma_mapped %d handle %u (not added to context)\n",
>> -             action, ivpu_bo_size(bo), (bool)bo->base.pages, (bool)bo->base.sgt,
>> -             bo->handle);
>> +    ivpu_dbg(vdev, BO,
>> +         "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n",
>> +         action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx ? bo->ctx->id : 0,
>> +         (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc,
>> +         (bool)bo->base.base.import_attach);
>>   }
>>     /*
>> @@ -49,12 +46,7 @@ int __must_check ivpu_bo_pin(struct ivpu_bo *bo)
>>       mutex_lock(&bo->lock);
>>         ivpu_dbg_bo(vdev, bo, "pin");
>> -
>> -    if (!bo->ctx) {
>> -        ivpu_err(vdev, "vpu_addr not allocated for BO %d\n", bo->handle);
>> -        ret = -EINVAL;
>> -        goto unlock;
>> -    }
>> +    drm_WARN_ON(&vdev->drm, !bo->ctx);
>>         if (!bo->mmu_mapped) {
>>           struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(&bo->base);
>> @@ -108,9 +100,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
>>   {
>>       struct ivpu_device *vdev = ivpu_bo_to_vdev(bo);
>>   -    lockdep_assert_held(&bo->lock);
>> -
>> -    ivpu_dbg_bo(vdev, bo, "unbind");
>> +    lockdep_assert(lockdep_is_held(&bo->lock) || !kref_read(&bo->base.base.refcount));
>>         if (bo->mmu_mapped) {
>>           drm_WARN_ON(&vdev->drm, !bo->ctx);
>> @@ -122,7 +112,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
>>         if (bo->ctx) {
>>           ivpu_mmu_context_remove_node(bo->ctx, &bo->mm_node);
>> -        bo->vpu_addr = 0;
>>           bo->ctx = NULL;
>>       }
>>   @@ -139,13 +128,6 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo)
>>       dma_resv_unlock(bo->base.base.resv);
>>   }
>>   -static void ivpu_bo_unbind(struct ivpu_bo *bo)
>> -{
>> -    mutex_lock(&bo->lock);
>> -    ivpu_bo_unbind_locked(bo);
>> -    mutex_unlock(&bo->lock);
>> -}
> 
> This does not seem to be related to $SUBJECT

OK, I'll drop this.

>> -
>>   void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx)
>>   {
>>       struct ivpu_bo *bo;
>> @@ -156,8 +138,10 @@ void ivpu_bo_remove_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m
>>       mutex_lock(&vdev->bo_list_lock);
>>       list_for_each_entry(bo, &vdev->bo_list, bo_list_node) {
>>           mutex_lock(&bo->lock);
>> -        if (bo->ctx == ctx)
>> +        if (bo->ctx == ctx) {
>> +            ivpu_dbg_bo(vdev, bo, "unbind");
>>               ivpu_bo_unbind_locked(bo);
>> +        }
>>           mutex_unlock(&bo->lock);
>>       }
>>       mutex_unlock(&vdev->bo_list_lock);
>> @@ -209,9 +193,6 @@ ivpu_bo_create(struct ivpu_device *vdev, u64 size, u32 flags)
>>       list_add_tail(&bo->bo_list_node, &vdev->bo_list);
>>       mutex_unlock(&vdev->bo_list_lock);
>>   -    ivpu_dbg(vdev, BO, "create: vpu_addr 0x%llx size %zu flags 0x%x\n",
>> -         bo->vpu_addr, bo->base.base.size, flags);
>> -
>>       return bo;
>>   }
>>   @@ -243,15 +224,15 @@ static void ivpu_bo_free(struct drm_gem_object *obj)
>>       struct ivpu_device *vdev = to_ivpu_device(obj->dev);
>>       struct ivpu_bo *bo = to_ivpu_bo(obj);
>>   +    ivpu_dbg_bo(vdev, bo, "free");
>> +
>>       mutex_lock(&vdev->bo_list_lock);
>>       list_del(&bo->bo_list_node);
>>       mutex_unlock(&vdev->bo_list_lock);
>>         drm_WARN_ON(&vdev->drm, !dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_READ));
>>   -    ivpu_dbg_bo(vdev, bo, "free");
>> -
>> -    ivpu_bo_unbind(bo);
>> +    ivpu_bo_unbind_locked(bo);
> 
> This does not seem to be related to $SUBJECT

OK, I'll drop this.

[PATCH 10/10] accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY

[Jacek Lawrynowicz]

From: "Wachowski, Karol" <karol.wachowski@intel.com>

DRM_IVPU_PARAM_CONTEXT_PRIORITY has been deprecated because it
has been replaced with DRM_IVPU_JOB_PRIORITY levels set with
submit IOCTL and was unused anyway.

Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
---
 drivers/accel/ivpu/ivpu_drv.c | 11 -----------
 drivers/accel/ivpu/ivpu_drv.h |  1 -
 drivers/accel/ivpu/ivpu_job.c |  3 +++
 include/uapi/drm/ivpu_accel.h | 21 ++++++++++++++++++++-
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index ec66c2c39877..546c0899bb9e 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -177,9 +177,6 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
 	case DRM_IVPU_PARAM_CONTEXT_BASE_ADDRESS:
 		args->value = vdev->hw->ranges.user.start;
 		break;
-	case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
-		args->value = file_priv->priority;
-		break;
 	case DRM_IVPU_PARAM_CONTEXT_ID:
 		args->value = file_priv->ctx.id;
 		break;
@@ -219,17 +216,10 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
 
 static int ivpu_set_param_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 {
-	struct ivpu_file_priv *file_priv = file->driver_priv;
 	struct drm_ivpu_param *args = data;
 	int ret = 0;
 
 	switch (args->param) {
-	case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
-		if (args->value <= DRM_IVPU_CONTEXT_PRIORITY_REALTIME)
-			file_priv->priority = args->value;
-		else
-			ret = -EINVAL;
-		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -258,7 +248,6 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
 	}
 
 	file_priv->vdev = vdev;
-	file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
 	kref_init(&file_priv->ref);
 	mutex_init(&file_priv->lock);
 
diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
index 9b6e336626e3..7a6bc1918780 100644
--- a/drivers/accel/ivpu/ivpu_drv.h
+++ b/drivers/accel/ivpu/ivpu_drv.h
@@ -146,7 +146,6 @@ struct ivpu_file_priv {
 	struct mutex lock; /* Protects cmdq */
 	struct ivpu_cmdq *cmdq[IVPU_NUM_ENGINES];
 	struct ivpu_mmu_context ctx;
-	u32 priority;
 	bool has_mmu_faults;
 };
 
diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
index 7206cf9cdb4a..82e40bb4803c 100644
--- a/drivers/accel/ivpu/ivpu_job.c
+++ b/drivers/accel/ivpu/ivpu_job.c
@@ -488,6 +488,9 @@ int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	if (params->engine > DRM_IVPU_ENGINE_COPY)
 		return -EINVAL;
 
+	if (params->priority > DRM_IVPU_JOB_PRIORITY_REALTIME)
+		return -EINVAL;
+
 	if (params->buffer_count == 0 || params->buffer_count > JOB_MAX_BUFFER_COUNT)
 		return -EINVAL;
 
diff --git a/include/uapi/drm/ivpu_accel.h b/include/uapi/drm/ivpu_accel.h
index de1944e42c65..cc9a0504ee2f 100644
--- a/include/uapi/drm/ivpu_accel.h
+++ b/include/uapi/drm/ivpu_accel.h
@@ -13,7 +13,7 @@ extern "C" {
 #endif
 
 #define DRM_IVPU_DRIVER_MAJOR 1
-#define DRM_IVPU_DRIVER_MINOR 0
+#define DRM_IVPU_DRIVER_MINOR 1
 
 #define DRM_IVPU_GET_PARAM		  0x00
 #define DRM_IVPU_SET_PARAM		  0x01
@@ -64,11 +64,18 @@ extern "C" {
 
 #define DRM_IVPU_PLATFORM_TYPE_SILICON	    0
 
+/* Deprecated - to be removed */
 #define DRM_IVPU_CONTEXT_PRIORITY_IDLE	    0
 #define DRM_IVPU_CONTEXT_PRIORITY_NORMAL    1
 #define DRM_IVPU_CONTEXT_PRIORITY_FOCUS	    2
 #define DRM_IVPU_CONTEXT_PRIORITY_REALTIME  3
 
+#define DRM_IVPU_JOB_PRIORITY_DEFAULT  0
+#define DRM_IVPU_JOB_PRIORITY_IDLE     1
+#define DRM_IVPU_JOB_PRIORITY_NORMAL   2
+#define DRM_IVPU_JOB_PRIORITY_FOCUS    3
+#define DRM_IVPU_JOB_PRIORITY_REALTIME 4
+
 /**
  * DRM_IVPU_CAP_METRIC_STREAMER
  *
@@ -286,6 +293,18 @@ struct drm_ivpu_submit {
 	 * to be executed. The offset has to be 8-byte aligned.
 	 */
 	__u32 commands_offset;
+
+	/**
+	 * @priority:
+	 *
+	 * Priority to be set for related job command queue, can be one of the following:
+	 * %DRM_IVPU_JOB_PRIORITY_DEFAULT
+	 * %DRM_IVPU_JOB_PRIORITY_IDLE
+	 * %DRM_IVPU_JOB_PRIORITY_NORMAL
+	 * %DRM_IVPU_JOB_PRIORITY_FOCUS
+	 * %DRM_IVPU_JOB_PRIORITY_REALTIME
+	 */
+	__u32 priority;
 };
 
 /* drm_ivpu_bo_wait job status codes */
-- 
2.43.0

Re: [PATCH 10/10] accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY

[Jeffrey Hugo]

On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <karol.wachowski@intel.com>
> 
> DRM_IVPU_PARAM_CONTEXT_PRIORITY has been deprecated because it
> has been replaced with DRM_IVPU_JOB_PRIORITY levels set with
> submit IOCTL and was unused anyway.
> 
> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_drv.c | 11 -----------
>   drivers/accel/ivpu/ivpu_drv.h |  1 -
>   drivers/accel/ivpu/ivpu_job.c |  3 +++
>   include/uapi/drm/ivpu_accel.h | 21 ++++++++++++++++++++-
>   4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index ec66c2c39877..546c0899bb9e 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -177,9 +177,6 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>   	case DRM_IVPU_PARAM_CONTEXT_BASE_ADDRESS:
>   		args->value = vdev->hw->ranges.user.start;
>   		break;
> -	case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
> -		args->value = file_priv->priority;
> -		break;
>   	case DRM_IVPU_PARAM_CONTEXT_ID:
>   		args->value = file_priv->ctx.id;
>   		break;
> @@ -219,17 +216,10 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>   
>   static int ivpu_set_param_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   {
> -	struct ivpu_file_priv *file_priv = file->driver_priv;
>   	struct drm_ivpu_param *args = data;
>   	int ret = 0;
>   
>   	switch (args->param) {
> -	case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
> -		if (args->value <= DRM_IVPU_CONTEXT_PRIORITY_REALTIME)
> -			file_priv->priority = args->value;
> -		else
> -			ret = -EINVAL;
> -		break;
>   	default:
>   		ret = -EINVAL;
>   	}
> @@ -258,7 +248,6 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
>   	}
>   
>   	file_priv->vdev = vdev;
> -	file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
>   	kref_init(&file_priv->ref);
>   	mutex_init(&file_priv->lock);
>   
> diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> index 9b6e336626e3..7a6bc1918780 100644
> --- a/drivers/accel/ivpu/ivpu_drv.h
> +++ b/drivers/accel/ivpu/ivpu_drv.h
> @@ -146,7 +146,6 @@ struct ivpu_file_priv {
>   	struct mutex lock; /* Protects cmdq */
>   	struct ivpu_cmdq *cmdq[IVPU_NUM_ENGINES];
>   	struct ivpu_mmu_context ctx;
> -	u32 priority;
>   	bool has_mmu_faults;
>   };
>   
> diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
> index 7206cf9cdb4a..82e40bb4803c 100644
> --- a/drivers/accel/ivpu/ivpu_job.c
> +++ b/drivers/accel/ivpu/ivpu_job.c
> @@ -488,6 +488,9 @@ int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   	if (params->engine > DRM_IVPU_ENGINE_COPY)
>   		return -EINVAL;
>   
> +	if (params->priority > DRM_IVPU_JOB_PRIORITY_REALTIME)
> +		return -EINVAL;
> +
>   	if (params->buffer_count == 0 || params->buffer_count > JOB_MAX_BUFFER_COUNT)
>   		return -EINVAL;
>   
> diff --git a/include/uapi/drm/ivpu_accel.h b/include/uapi/drm/ivpu_accel.h
> index de1944e42c65..cc9a0504ee2f 100644
> --- a/include/uapi/drm/ivpu_accel.h
> +++ b/include/uapi/drm/ivpu_accel.h
> @@ -13,7 +13,7 @@ extern "C" {
>   #endif
>   
>   #define DRM_IVPU_DRIVER_MAJOR 1
> -#define DRM_IVPU_DRIVER_MINOR 0
> +#define DRM_IVPU_DRIVER_MINOR 1

I remember when this driver was going through initial review before 
acceptance, Oded mentioned that the DRM driver version mechanism was 
deprecated and to not use it.  Based on that, it seems like you should 
not be incrementing the minor number.

>   
>   #define DRM_IVPU_GET_PARAM		  0x00
>   #define DRM_IVPU_SET_PARAM		  0x01
> @@ -64,11 +64,18 @@ extern "C" {
>   
>   #define DRM_IVPU_PLATFORM_TYPE_SILICON	    0
>   
> +/* Deprecated - to be removed */
>   #define DRM_IVPU_CONTEXT_PRIORITY_IDLE	    0
>   #define DRM_IVPU_CONTEXT_PRIORITY_NORMAL    1
>   #define DRM_IVPU_CONTEXT_PRIORITY_FOCUS	    2
>   #define DRM_IVPU_CONTEXT_PRIORITY_REALTIME  3

$SUBJECT suggests these are being removed, not just deprecated.  Also, 
shouldn't DRM_IVPU_PARAM_CONTEXT_PRIORITY which is a few lines above 
this be deprecated/removed/something?

>   
> +#define DRM_IVPU_JOB_PRIORITY_DEFAULT  0
> +#define DRM_IVPU_JOB_PRIORITY_IDLE     1
> +#define DRM_IVPU_JOB_PRIORITY_NORMAL   2
> +#define DRM_IVPU_JOB_PRIORITY_FOCUS    3
> +#define DRM_IVPU_JOB_PRIORITY_REALTIME 4
> +
>   /**
>    * DRM_IVPU_CAP_METRIC_STREAMER
>    *
> @@ -286,6 +293,18 @@ struct drm_ivpu_submit {
>   	 * to be executed. The offset has to be 8-byte aligned.
>   	 */
>   	__u32 commands_offset;
> +
> +	/**
> +	 * @priority:
> +	 *
> +	 * Priority to be set for related job command queue, can be one of the following:
> +	 * %DRM_IVPU_JOB_PRIORITY_DEFAULT
> +	 * %DRM_IVPU_JOB_PRIORITY_IDLE
> +	 * %DRM_IVPU_JOB_PRIORITY_NORMAL
> +	 * %DRM_IVPU_JOB_PRIORITY_FOCUS
> +	 * %DRM_IVPU_JOB_PRIORITY_REALTIME
> +	 */
> +	__u32 priority;

I think this breaks the uapi (which makes me think you are using the 
driver minor version above to detect).  This struct is passed to DRM_IOW 
which uses the struct size to calculate the ioctl number.  By changing 
the size of this struct, you change the ioctl number, and make it so 
that old userspace (with the old number) cannot work with newer kernels.

I beleive last time I brought up a uapi breakage, I was told that your 
userspace han't been offically released yet.  Is that still the case?

Seems odd though, because you are incrementing the driver minor number 
above which makes me think you need to communicate this change to 
userspace, which seems to suggest you might have old userspace out in 
the wild...

Re: [PATCH 10/10] accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY

[Jacek Lawrynowicz]

On 05.01.2024 18:29, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> From: "Wachowski, Karol" <karol.wachowski@intel.com>
>>
>> DRM_IVPU_PARAM_CONTEXT_PRIORITY has been deprecated because it
>> has been replaced with DRM_IVPU_JOB_PRIORITY levels set with
>> submit IOCTL and was unused anyway.
>>
>> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
>> ---
>>   drivers/accel/ivpu/ivpu_drv.c | 11 -----------
>>   drivers/accel/ivpu/ivpu_drv.h |  1 -
>>   drivers/accel/ivpu/ivpu_job.c |  3 +++
>>   include/uapi/drm/ivpu_accel.h | 21 ++++++++++++++++++++-
>>   4 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
>> index ec66c2c39877..546c0899bb9e 100644
>> --- a/drivers/accel/ivpu/ivpu_drv.c
>> +++ b/drivers/accel/ivpu/ivpu_drv.c
>> @@ -177,9 +177,6 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>>       case DRM_IVPU_PARAM_CONTEXT_BASE_ADDRESS:
>>           args->value = vdev->hw->ranges.user.start;
>>           break;
>> -    case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>> -        args->value = file_priv->priority;
>> -        break;
>>       case DRM_IVPU_PARAM_CONTEXT_ID:
>>           args->value = file_priv->ctx.id;
>>           break;
>> @@ -219,17 +216,10 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>>     static int ivpu_set_param_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>>   {
>> -    struct ivpu_file_priv *file_priv = file->driver_priv;
>>       struct drm_ivpu_param *args = data;
>>       int ret = 0;
>>         switch (args->param) {
>> -    case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>> -        if (args->value <= DRM_IVPU_CONTEXT_PRIORITY_REALTIME)
>> -            file_priv->priority = args->value;
>> -        else
>> -            ret = -EINVAL;
>> -        break;
>>       default:
>>           ret = -EINVAL;
>>       }
>> @@ -258,7 +248,6 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
>>       }
>>         file_priv->vdev = vdev;
>> -    file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
>>       kref_init(&file_priv->ref);
>>       mutex_init(&file_priv->lock);
>>   diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
>> index 9b6e336626e3..7a6bc1918780 100644
>> --- a/drivers/accel/ivpu/ivpu_drv.h
>> +++ b/drivers/accel/ivpu/ivpu_drv.h
>> @@ -146,7 +146,6 @@ struct ivpu_file_priv {
>>       struct mutex lock; /* Protects cmdq */
>>       struct ivpu_cmdq *cmdq[IVPU_NUM_ENGINES];
>>       struct ivpu_mmu_context ctx;
>> -    u32 priority;
>>       bool has_mmu_faults;
>>   };
>>   diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
>> index 7206cf9cdb4a..82e40bb4803c 100644
>> --- a/drivers/accel/ivpu/ivpu_job.c
>> +++ b/drivers/accel/ivpu/ivpu_job.c
>> @@ -488,6 +488,9 @@ int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>>       if (params->engine > DRM_IVPU_ENGINE_COPY)
>>           return -EINVAL;
>>   +    if (params->priority > DRM_IVPU_JOB_PRIORITY_REALTIME)
>> +        return -EINVAL;
>> +
>>       if (params->buffer_count == 0 || params->buffer_count > JOB_MAX_BUFFER_COUNT)
>>           return -EINVAL;
>>   diff --git a/include/uapi/drm/ivpu_accel.h b/include/uapi/drm/ivpu_accel.h
>> index de1944e42c65..cc9a0504ee2f 100644
>> --- a/include/uapi/drm/ivpu_accel.h
>> +++ b/include/uapi/drm/ivpu_accel.h
>> @@ -13,7 +13,7 @@ extern "C" {
>>   #endif
>>     #define DRM_IVPU_DRIVER_MAJOR 1
>> -#define DRM_IVPU_DRIVER_MINOR 0
>> +#define DRM_IVPU_DRIVER_MINOR 1
> 
> I remember when this driver was going through initial review before acceptance, Oded mentioned that the DRM driver version mechanism was deprecated and to not use it.  Based on that, it seems like you should not be incrementing the minor number.

I wanted to use minor version in tests to verify the UAPI but this is not very important. I can leave this as is.

>>     #define DRM_IVPU_GET_PARAM          0x00
>>   #define DRM_IVPU_SET_PARAM          0x01
>> @@ -64,11 +64,18 @@ extern "C" {
>>     #define DRM_IVPU_PLATFORM_TYPE_SILICON        0
>>   +/* Deprecated - to be removed */
>>   #define DRM_IVPU_CONTEXT_PRIORITY_IDLE        0
>>   #define DRM_IVPU_CONTEXT_PRIORITY_NORMAL    1
>>   #define DRM_IVPU_CONTEXT_PRIORITY_FOCUS        2
>>   #define DRM_IVPU_CONTEXT_PRIORITY_REALTIME  3
> 
> $SUBJECT suggests these are being removed, not just deprecated.  Also, shouldn't DRM_IVPU_PARAM_CONTEXT_PRIORITY which is a few lines above this be deprecated/removed/something?

OK, I'll correct the subject and add "deprecated" comment to DRM_IVPU_PARAM_CONTEXT_PRIORITY.

>>   +#define DRM_IVPU_JOB_PRIORITY_DEFAULT  0
>> +#define DRM_IVPU_JOB_PRIORITY_IDLE     1
>> +#define DRM_IVPU_JOB_PRIORITY_NORMAL   2
>> +#define DRM_IVPU_JOB_PRIORITY_FOCUS    3
>> +#define DRM_IVPU_JOB_PRIORITY_REALTIME 4
>> +
>>   /**
>>    * DRM_IVPU_CAP_METRIC_STREAMER
>>    *
>> @@ -286,6 +293,18 @@ struct drm_ivpu_submit {
>>        * to be executed. The offset has to be 8-byte aligned.
>>        */
>>       __u32 commands_offset;
>> +
>> +    /**
>> +     * @priority:
>> +     *
>> +     * Priority to be set for related job command queue, can be one of the following:
>> +     * %DRM_IVPU_JOB_PRIORITY_DEFAULT
>> +     * %DRM_IVPU_JOB_PRIORITY_IDLE
>> +     * %DRM_IVPU_JOB_PRIORITY_NORMAL
>> +     * %DRM_IVPU_JOB_PRIORITY_FOCUS
>> +     * %DRM_IVPU_JOB_PRIORITY_REALTIME
>> +     */
>> +    __u32 priority;
> 
> I think this breaks the uapi (which makes me think you are using the driver minor version above to detect).  This struct is passed to DRM_IOW which uses the struct size to calculate the ioctl number.  By changing the size of this struct, you change the ioctl number, and make it so that old userspace (with the old number) cannot work with newer kernels.
> 
> I beleive last time I brought up a uapi breakage, I was told that your userspace han't been offically released yet.  Is that still the case?
> 
> Seems odd though, because you are incrementing the driver minor number above which makes me think you need to communicate this change to userspace, which seems to suggest you might have old userspace out in the wild...

The user-space part of the driver was already released but it have never used DRM_IVPU_PARAM_CONTEXT_PRIORITY.
I've tested the new kmd with old umd and ioctls work fine. drm_ioctl() handles the difference in user vs driver arg size.
I think it is perfectly safe to extend the ioctl arg. The ioctl number is passed directly to DRM_IOW(), I can't see where it would be calculated based on arg size.

Regards,
Jacek

Re: [PATCH 10/10] accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY

[Jeffrey Hugo]

On 1/10/2024 7:33 AM, Jacek Lawrynowicz wrote:
> On 05.01.2024 18:29, Jeffrey Hugo wrote:
>> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>>> From: "Wachowski, Karol" <karol.wachowski@intel.com>
>>>
>>> DRM_IVPU_PARAM_CONTEXT_PRIORITY has been deprecated because it
>>> has been replaced with DRM_IVPU_JOB_PRIORITY levels set with
>>> submit IOCTL and was unused anyway.
>>>
>>> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
>>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
>>> ---
>>>    drivers/accel/ivpu/ivpu_drv.c | 11 -----------
>>>    drivers/accel/ivpu/ivpu_drv.h |  1 -
>>>    drivers/accel/ivpu/ivpu_job.c |  3 +++
>>>    include/uapi/drm/ivpu_accel.h | 21 ++++++++++++++++++++-
>>>    4 files changed, 23 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
>>> index ec66c2c39877..546c0899bb9e 100644
>>> --- a/drivers/accel/ivpu/ivpu_drv.c
>>> +++ b/drivers/accel/ivpu/ivpu_drv.c
>>> @@ -177,9 +177,6 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>>>        case DRM_IVPU_PARAM_CONTEXT_BASE_ADDRESS:
>>>            args->value = vdev->hw->ranges.user.start;
>>>            break;
>>> -    case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>>> -        args->value = file_priv->priority;
>>> -        break;
>>>        case DRM_IVPU_PARAM_CONTEXT_ID:
>>>            args->value = file_priv->ctx.id;
>>>            break;
>>> @@ -219,17 +216,10 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>>>      static int ivpu_set_param_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>>>    {
>>> -    struct ivpu_file_priv *file_priv = file->driver_priv;
>>>        struct drm_ivpu_param *args = data;
>>>        int ret = 0;
>>>          switch (args->param) {
>>> -    case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>>> -        if (args->value <= DRM_IVPU_CONTEXT_PRIORITY_REALTIME)
>>> -            file_priv->priority = args->value;
>>> -        else
>>> -            ret = -EINVAL;
>>> -        break;
>>>        default:
>>>            ret = -EINVAL;
>>>        }
>>> @@ -258,7 +248,6 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
>>>        }
>>>          file_priv->vdev = vdev;
>>> -    file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
>>>        kref_init(&file_priv->ref);
>>>        mutex_init(&file_priv->lock);
>>>    diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
>>> index 9b6e336626e3..7a6bc1918780 100644
>>> --- a/drivers/accel/ivpu/ivpu_drv.h
>>> +++ b/drivers/accel/ivpu/ivpu_drv.h
>>> @@ -146,7 +146,6 @@ struct ivpu_file_priv {
>>>        struct mutex lock; /* Protects cmdq */
>>>        struct ivpu_cmdq *cmdq[IVPU_NUM_ENGINES];
>>>        struct ivpu_mmu_context ctx;
>>> -    u32 priority;
>>>        bool has_mmu_faults;
>>>    };
>>>    diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
>>> index 7206cf9cdb4a..82e40bb4803c 100644
>>> --- a/drivers/accel/ivpu/ivpu_job.c
>>> +++ b/drivers/accel/ivpu/ivpu_job.c
>>> @@ -488,6 +488,9 @@ int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>>>        if (params->engine > DRM_IVPU_ENGINE_COPY)
>>>            return -EINVAL;
>>>    +    if (params->priority > DRM_IVPU_JOB_PRIORITY_REALTIME)
>>> +        return -EINVAL;
>>> +
>>>        if (params->buffer_count == 0 || params->buffer_count > JOB_MAX_BUFFER_COUNT)
>>>            return -EINVAL;
>>>    diff --git a/include/uapi/drm/ivpu_accel.h b/include/uapi/drm/ivpu_accel.h
>>> index de1944e42c65..cc9a0504ee2f 100644
>>> --- a/include/uapi/drm/ivpu_accel.h
>>> +++ b/include/uapi/drm/ivpu_accel.h
>>> @@ -13,7 +13,7 @@ extern "C" {
>>>    #endif
>>>      #define DRM_IVPU_DRIVER_MAJOR 1
>>> -#define DRM_IVPU_DRIVER_MINOR 0
>>> +#define DRM_IVPU_DRIVER_MINOR 1
>>
>> I remember when this driver was going through initial review before acceptance, Oded mentioned that the DRM driver version mechanism was deprecated and to not use it.  Based on that, it seems like you should not be incrementing the minor number.
> 
> I wanted to use minor version in tests to verify the UAPI but this is not very important. I can leave this as is.
> 
>>>      #define DRM_IVPU_GET_PARAM          0x00
>>>    #define DRM_IVPU_SET_PARAM          0x01
>>> @@ -64,11 +64,18 @@ extern "C" {
>>>      #define DRM_IVPU_PLATFORM_TYPE_SILICON        0
>>>    +/* Deprecated - to be removed */
>>>    #define DRM_IVPU_CONTEXT_PRIORITY_IDLE        0
>>>    #define DRM_IVPU_CONTEXT_PRIORITY_NORMAL    1
>>>    #define DRM_IVPU_CONTEXT_PRIORITY_FOCUS        2
>>>    #define DRM_IVPU_CONTEXT_PRIORITY_REALTIME  3
>>
>> $SUBJECT suggests these are being removed, not just deprecated.  Also, shouldn't DRM_IVPU_PARAM_CONTEXT_PRIORITY which is a few lines above this be deprecated/removed/something?
> 
> OK, I'll correct the subject and add "deprecated" comment to DRM_IVPU_PARAM_CONTEXT_PRIORITY.
> 
>>>    +#define DRM_IVPU_JOB_PRIORITY_DEFAULT  0
>>> +#define DRM_IVPU_JOB_PRIORITY_IDLE     1
>>> +#define DRM_IVPU_JOB_PRIORITY_NORMAL   2
>>> +#define DRM_IVPU_JOB_PRIORITY_FOCUS    3
>>> +#define DRM_IVPU_JOB_PRIORITY_REALTIME 4
>>> +
>>>    /**
>>>     * DRM_IVPU_CAP_METRIC_STREAMER
>>>     *
>>> @@ -286,6 +293,18 @@ struct drm_ivpu_submit {
>>>         * to be executed. The offset has to be 8-byte aligned.
>>>         */
>>>        __u32 commands_offset;
>>> +
>>> +    /**
>>> +     * @priority:
>>> +     *
>>> +     * Priority to be set for related job command queue, can be one of the following:
>>> +     * %DRM_IVPU_JOB_PRIORITY_DEFAULT
>>> +     * %DRM_IVPU_JOB_PRIORITY_IDLE
>>> +     * %DRM_IVPU_JOB_PRIORITY_NORMAL
>>> +     * %DRM_IVPU_JOB_PRIORITY_FOCUS
>>> +     * %DRM_IVPU_JOB_PRIORITY_REALTIME
>>> +     */
>>> +    __u32 priority;
>>
>> I think this breaks the uapi (which makes me think you are using the driver minor version above to detect).  This struct is passed to DRM_IOW which uses the struct size to calculate the ioctl number.  By changing the size of this struct, you change the ioctl number, and make it so that old userspace (with the old number) cannot work with newer kernels.
>>
>> I beleive last time I brought up a uapi breakage, I was told that your userspace han't been offically released yet.  Is that still the case?
>>
>> Seems odd though, because you are incrementing the driver minor number above which makes me think you need to communicate this change to userspace, which seems to suggest you might have old userspace out in the wild...
> 
> The user-space part of the driver was already released but it have never used DRM_IVPU_PARAM_CONTEXT_PRIORITY.
> I've tested the new kmd with old umd and ioctls work fine. drm_ioctl() handles the difference in user vs driver arg size.

Intresting.  You are right, drm_ioctl does extra handling to ignore the 
ioctl size.  As long as the struct change is backwards compatible 
everything will work fine.

> I think it is perfectly safe to extend the ioctl arg. The ioctl number is passed directly to DRM_IOW(), I can't see where it would be calculated based on arg size.

DRM_IOW is defined as _IOW which is defined as _IOC.  _IOC calls 
_IOC_TYPECHECK which looks to be defined as a sizeof.  The actual _IOC 
macro uses the 4 paramaters in bitwise math to generate the ioctl 
number.  I probably should have clarified eariler that I'm considering 
the ioctl number to be the 32 bit value "returned" by DRM_IOW, and not 
the NR field.

Anyways, as you pointed out, I had forgotten an element of how DRM 
handles ioctls.

-Jeff