Re: [PATCH v4 2/7] accel/ivpu: Add Intel VPU MMU support

2022-12-19 Thread Jacek Lawrynowicz
Hi,

On 18.12.2022 10:13, Oded Gabbay wrote:
> On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz
>  wrote:
>>
>> VPU Memory Management Unit is based on ARM MMU-600.
>> It allows the creation of multiple virtual address spaces for
>> the device and map noncontinuous host memory (there is no dedicated
>> memory on the VPU).
>>
>> Address space is implemented as a struct ivpu_mmu_context, it has an ID,
>> drm_mm allocator for VPU addresses and struct ivpu_mmu_pgtable that
>> holds actual 3-level, 4KB page table.
>> Context with ID 0 (global context) is created upon driver initialization
>> and it's mainly used for mapping memory required to execute
>> the firmware.
>> Contexts with non-zero IDs are user contexts allocated each time
>> the devices is open()-ed and they map command buffers and other
>> workload-related memory.
>> Workloads executing in a given contexts have access only
>> to the memory mapped in this context.
>>
>> This patch is has to main files:
> This patch has two main files:

OK

>>   - ivpu_mmu_context.c handles MMU page tables and memory mapping
>>   - ivpu_mmu.c implements a driver that programs the MMU device
>>
>> Co-developed-by: Karol Wachowski 
>> Signed-off-by: Karol Wachowski 
>> Co-developed-by: Krystian Pradzynski 
>> Signed-off-by: Krystian Pradzynski 
>> Signed-off-by: Jacek Lawrynowicz 
>> ---
>>  drivers/accel/ivpu/Makefile   |   4 +-
>>  drivers/accel/ivpu/ivpu_drv.c |  83 ++-
>>  drivers/accel/ivpu/ivpu_drv.h |   6 +
>>  drivers/accel/ivpu/ivpu_hw_mtl.c  |  10 +
>>  drivers/accel/ivpu/ivpu_mmu.c | 875 ++
>>  drivers/accel/ivpu/ivpu_mmu.h |  50 ++
>>  drivers/accel/ivpu/ivpu_mmu_context.c | 385 
>>  drivers/accel/ivpu/ivpu_mmu_context.h |  49 ++
>>  include/uapi/drm/ivpu_drm.h   |   4 +
>>  9 files changed, 1463 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/accel/ivpu/ivpu_mmu.c
>>  create mode 100644 drivers/accel/ivpu/ivpu_mmu.h
>>  create mode 100644 drivers/accel/ivpu/ivpu_mmu_context.c
>>  create mode 100644 drivers/accel/ivpu/ivpu_mmu_context.h
>>
>> diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
>> index 28330c04e52f..37b8bf1d3247 100644
>> --- a/drivers/accel/ivpu/Makefile
>> +++ b/drivers/accel/ivpu/Makefile
>> @@ -3,6 +3,8 @@
>>
>>  intel_vpu-y := \
>> ivpu_drv.o \
>> -   ivpu_hw_mtl.o
>> +   ivpu_hw_mtl.o \
>> +   ivpu_mmu.o \
>> +   ivpu_mmu_context.o
>>
>>  obj-$(CONFIG_DRM_ACCEL_IVPU) += intel_vpu.o
>> \ No newline at end of file
>> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
>> index 8fbccb8d888b..a22d41ca5a4b 100644
>> --- a/drivers/accel/ivpu/ivpu_drv.c
>> +++ b/drivers/accel/ivpu/ivpu_drv.c
>> @@ -15,6 +15,8 @@
>>
>>  #include "ivpu_drv.h"
>>  #include "ivpu_hw.h"
>> +#include "ivpu_mmu.h"
>> +#include "ivpu_mmu_context.h"
>>
>>  #ifndef DRIVER_VERSION_STR
>>  #define DRIVER_VERSION_STR __stringify(DRM_IVPU_DRIVER_MAJOR) "." \
>> @@ -37,23 +39,38 @@ MODULE_PARM_DESC(pll_max_ratio, "Maximum PLL ratio used 
>> to set VPU frequency");
>>
>>  struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv)
>>  {
>> +   struct ivpu_device *vdev = file_priv->vdev;
>> +
>> kref_get(_priv->ref);
>> +
>> +   ivpu_dbg(vdev, KREF, "file_priv get: ctx %u refcount %u\n",
>> +file_priv->ctx.id, kref_read(_priv->ref));
>> +
>> return file_priv;
>>  }
>>
>>  static void file_priv_release(struct kref *ref)
>>  {
>> struct ivpu_file_priv *file_priv = container_of(ref, struct 
>> ivpu_file_priv, ref);
>> +   struct ivpu_device *vdev = file_priv->vdev;
>>
>> +   ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n", 
>> file_priv->ctx.id);
>> +
>> +   ivpu_mmu_user_context_fini(vdev, _priv->ctx);
>> +   WARN_ON(xa_erase_irq(>context_xa, file_priv->ctx.id) != 
>> file_priv);
>> kfree(file_priv);
>>  }
>>
>>  void ivpu_file_priv_put(struct ivpu_file_priv **link)
>>  {
>> struct ivpu_file_priv *file_priv = *link;
>> +   struct ivpu_device *vdev = file_priv->vdev;
>>
>> WARN_ON(!file_priv);
>>
>> +   ivpu_dbg(vdev, KREF, "file_priv put: ctx %u refcount %u\n",
>> +file_priv->ctx.id, kref_read(_priv->ref));
>> +
>> *link = NULL;
>> kref_put(_priv->ref, file_priv_release);
>>  }
>> @@ -88,6 +105,9 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, 
>> void *data, struct drm_f
>> case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>> args->value = file_priv->priority;
>> break;
>> +   case DRM_IVPU_PARAM_CONTEXT_ID:
>> +   args->value = file_priv->ctx.id;
> Why is this needed ? Why does the user need to know its context ID ?

This is not really needed by the user space, we only use this as a debug 
feature.

>> +   break;
>> default:
>> ret = -EINVAL;
>> break;
>> @@ 

Re: [PATCH v4 2/7] accel/ivpu: Add Intel VPU MMU support

2022-12-18 Thread Oded Gabbay
On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz
 wrote:
>
> VPU Memory Management Unit is based on ARM MMU-600.
> It allows the creation of multiple virtual address spaces for
> the device and map noncontinuous host memory (there is no dedicated
> memory on the VPU).
>
> Address space is implemented as a struct ivpu_mmu_context, it has an ID,
> drm_mm allocator for VPU addresses and struct ivpu_mmu_pgtable that
> holds actual 3-level, 4KB page table.
> Context with ID 0 (global context) is created upon driver initialization
> and it's mainly used for mapping memory required to execute
> the firmware.
> Contexts with non-zero IDs are user contexts allocated each time
> the devices is open()-ed and they map command buffers and other
> workload-related memory.
> Workloads executing in a given contexts have access only
> to the memory mapped in this context.
>
> This patch is has to main files:
This patch has two main files:

>   - ivpu_mmu_context.c handles MMU page tables and memory mapping
>   - ivpu_mmu.c implements a driver that programs the MMU device
>
> Co-developed-by: Karol Wachowski 
> Signed-off-by: Karol Wachowski 
> Co-developed-by: Krystian Pradzynski 
> Signed-off-by: Krystian Pradzynski 
> Signed-off-by: Jacek Lawrynowicz 
> ---
>  drivers/accel/ivpu/Makefile   |   4 +-
>  drivers/accel/ivpu/ivpu_drv.c |  83 ++-
>  drivers/accel/ivpu/ivpu_drv.h |   6 +
>  drivers/accel/ivpu/ivpu_hw_mtl.c  |  10 +
>  drivers/accel/ivpu/ivpu_mmu.c | 875 ++
>  drivers/accel/ivpu/ivpu_mmu.h |  50 ++
>  drivers/accel/ivpu/ivpu_mmu_context.c | 385 
>  drivers/accel/ivpu/ivpu_mmu_context.h |  49 ++
>  include/uapi/drm/ivpu_drm.h   |   4 +
>  9 files changed, 1463 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/accel/ivpu/ivpu_mmu.c
>  create mode 100644 drivers/accel/ivpu/ivpu_mmu.h
>  create mode 100644 drivers/accel/ivpu/ivpu_mmu_context.c
>  create mode 100644 drivers/accel/ivpu/ivpu_mmu_context.h
>
> diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
> index 28330c04e52f..37b8bf1d3247 100644
> --- a/drivers/accel/ivpu/Makefile
> +++ b/drivers/accel/ivpu/Makefile
> @@ -3,6 +3,8 @@
>
>  intel_vpu-y := \
> ivpu_drv.o \
> -   ivpu_hw_mtl.o
> +   ivpu_hw_mtl.o \
> +   ivpu_mmu.o \
> +   ivpu_mmu_context.o
>
>  obj-$(CONFIG_DRM_ACCEL_IVPU) += intel_vpu.o
> \ No newline at end of file
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index 8fbccb8d888b..a22d41ca5a4b 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -15,6 +15,8 @@
>
>  #include "ivpu_drv.h"
>  #include "ivpu_hw.h"
> +#include "ivpu_mmu.h"
> +#include "ivpu_mmu_context.h"
>
>  #ifndef DRIVER_VERSION_STR
>  #define DRIVER_VERSION_STR __stringify(DRM_IVPU_DRIVER_MAJOR) "." \
> @@ -37,23 +39,38 @@ MODULE_PARM_DESC(pll_max_ratio, "Maximum PLL ratio used 
> to set VPU frequency");
>
>  struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv)
>  {
> +   struct ivpu_device *vdev = file_priv->vdev;
> +
> kref_get(_priv->ref);
> +
> +   ivpu_dbg(vdev, KREF, "file_priv get: ctx %u refcount %u\n",
> +file_priv->ctx.id, kref_read(_priv->ref));
> +
> return file_priv;
>  }
>
>  static void file_priv_release(struct kref *ref)
>  {
> struct ivpu_file_priv *file_priv = container_of(ref, struct 
> ivpu_file_priv, ref);
> +   struct ivpu_device *vdev = file_priv->vdev;
>
> +   ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n", 
> file_priv->ctx.id);
> +
> +   ivpu_mmu_user_context_fini(vdev, _priv->ctx);
> +   WARN_ON(xa_erase_irq(>context_xa, file_priv->ctx.id) != 
> file_priv);
> kfree(file_priv);
>  }
>
>  void ivpu_file_priv_put(struct ivpu_file_priv **link)
>  {
> struct ivpu_file_priv *file_priv = *link;
> +   struct ivpu_device *vdev = file_priv->vdev;
>
> WARN_ON(!file_priv);
>
> +   ivpu_dbg(vdev, KREF, "file_priv put: ctx %u refcount %u\n",
> +file_priv->ctx.id, kref_read(_priv->ref));
> +
> *link = NULL;
> kref_put(_priv->ref, file_priv_release);
>  }
> @@ -88,6 +105,9 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, 
> void *data, struct drm_f
> case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
> args->value = file_priv->priority;
> break;
> +   case DRM_IVPU_PARAM_CONTEXT_ID:
> +   args->value = file_priv->ctx.id;
Why is this needed ? Why does the user need to know its context ID ?

> +   break;
> default:
> ret = -EINVAL;
> break;
> @@ -120,22 +140,59 @@ static int ivpu_open(struct drm_device *dev, struct 
> drm_file *file)
>  {
> struct ivpu_device *vdev = to_ivpu_device(dev);
> struct ivpu_file_priv *file_priv;
> +   u32 ctx_id;
> +   void *old;
> +   int ret;
> +
> + 

[PATCH v4 2/7] accel/ivpu: Add Intel VPU MMU support

2022-12-08 Thread Jacek Lawrynowicz
VPU Memory Management Unit is based on ARM MMU-600.
It allows the creation of multiple virtual address spaces for
the device and map noncontinuous host memory (there is no dedicated
memory on the VPU).

Address space is implemented as a struct ivpu_mmu_context, it has an ID,
drm_mm allocator for VPU addresses and struct ivpu_mmu_pgtable that
holds actual 3-level, 4KB page table.
Context with ID 0 (global context) is created upon driver initialization
and it's mainly used for mapping memory required to execute
the firmware.
Contexts with non-zero IDs are user contexts allocated each time
the devices is open()-ed and they map command buffers and other
workload-related memory.
Workloads executing in a given contexts have access only
to the memory mapped in this context.

This patch is has to main files:
  - ivpu_mmu_context.c handles MMU page tables and memory mapping
  - ivpu_mmu.c implements a driver that programs the MMU device

Co-developed-by: Karol Wachowski 
Signed-off-by: Karol Wachowski 
Co-developed-by: Krystian Pradzynski 
Signed-off-by: Krystian Pradzynski 
Signed-off-by: Jacek Lawrynowicz 
---
 drivers/accel/ivpu/Makefile   |   4 +-
 drivers/accel/ivpu/ivpu_drv.c |  83 ++-
 drivers/accel/ivpu/ivpu_drv.h |   6 +
 drivers/accel/ivpu/ivpu_hw_mtl.c  |  10 +
 drivers/accel/ivpu/ivpu_mmu.c | 875 ++
 drivers/accel/ivpu/ivpu_mmu.h |  50 ++
 drivers/accel/ivpu/ivpu_mmu_context.c | 385 
 drivers/accel/ivpu/ivpu_mmu_context.h |  49 ++
 include/uapi/drm/ivpu_drm.h   |   4 +
 9 files changed, 1463 insertions(+), 3 deletions(-)
 create mode 100644 drivers/accel/ivpu/ivpu_mmu.c
 create mode 100644 drivers/accel/ivpu/ivpu_mmu.h
 create mode 100644 drivers/accel/ivpu/ivpu_mmu_context.c
 create mode 100644 drivers/accel/ivpu/ivpu_mmu_context.h

diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
index 28330c04e52f..37b8bf1d3247 100644
--- a/drivers/accel/ivpu/Makefile
+++ b/drivers/accel/ivpu/Makefile
@@ -3,6 +3,8 @@
 
 intel_vpu-y := \
ivpu_drv.o \
-   ivpu_hw_mtl.o
+   ivpu_hw_mtl.o \
+   ivpu_mmu.o \
+   ivpu_mmu_context.o
 
 obj-$(CONFIG_DRM_ACCEL_IVPU) += intel_vpu.o
\ No newline at end of file
diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index 8fbccb8d888b..a22d41ca5a4b 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -15,6 +15,8 @@
 
 #include "ivpu_drv.h"
 #include "ivpu_hw.h"
+#include "ivpu_mmu.h"
+#include "ivpu_mmu_context.h"
 
 #ifndef DRIVER_VERSION_STR
 #define DRIVER_VERSION_STR __stringify(DRM_IVPU_DRIVER_MAJOR) "." \
@@ -37,23 +39,38 @@ MODULE_PARM_DESC(pll_max_ratio, "Maximum PLL ratio used to 
set VPU frequency");
 
 struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv)
 {
+   struct ivpu_device *vdev = file_priv->vdev;
+
kref_get(_priv->ref);
+
+   ivpu_dbg(vdev, KREF, "file_priv get: ctx %u refcount %u\n",
+file_priv->ctx.id, kref_read(_priv->ref));
+
return file_priv;
 }
 
 static void file_priv_release(struct kref *ref)
 {
struct ivpu_file_priv *file_priv = container_of(ref, struct 
ivpu_file_priv, ref);
+   struct ivpu_device *vdev = file_priv->vdev;
 
+   ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n", file_priv->ctx.id);
+
+   ivpu_mmu_user_context_fini(vdev, _priv->ctx);
+   WARN_ON(xa_erase_irq(>context_xa, file_priv->ctx.id) != 
file_priv);
kfree(file_priv);
 }
 
 void ivpu_file_priv_put(struct ivpu_file_priv **link)
 {
struct ivpu_file_priv *file_priv = *link;
+   struct ivpu_device *vdev = file_priv->vdev;
 
WARN_ON(!file_priv);
 
+   ivpu_dbg(vdev, KREF, "file_priv put: ctx %u refcount %u\n",
+file_priv->ctx.id, kref_read(_priv->ref));
+
*link = NULL;
kref_put(_priv->ref, file_priv_release);
 }
@@ -88,6 +105,9 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void 
*data, struct drm_f
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;
default:
ret = -EINVAL;
break;
@@ -120,22 +140,59 @@ static int ivpu_open(struct drm_device *dev, struct 
drm_file *file)
 {
struct ivpu_device *vdev = to_ivpu_device(dev);
struct ivpu_file_priv *file_priv;
+   u32 ctx_id;
+   void *old;
+   int ret;
+
+   ret = xa_alloc_irq(>context_xa, _id, NULL, 
vdev->context_xa_limit, GFP_KERNEL);
+   if (ret) {
+   ivpu_err(vdev, "Failed to allocate context id: %d\n", ret);
+   return ret;
+   }
 
file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
-   if (!file_priv)
-   return -ENOMEM;
+   if (!file_priv) {
+   ret = -ENOMEM;
+