Re: [PATCH v3 2/7] drm/ivpu: Add Intel VPU MMU support

[Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>]

Hi,

On 11/1/2022 10:00 AM, Thomas Zimmermann wrote:
> Hi
> 
> Am 24.09.22 um 17:11 schrieb Jacek Lawrynowicz:
>> VPU Memory Management Unit is based on ARM MMU-600.
>> It allows to create 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
>>
>> Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
>> Signed-off-by: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>
>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
>> ---
>>   drivers/gpu/drm/ivpu/Makefile           |   4 +-
>>   drivers/gpu/drm/ivpu/ivpu_drv.c         |  59 +-
>>   drivers/gpu/drm/ivpu/ivpu_drv.h         |   7 +
>>   drivers/gpu/drm/ivpu/ivpu_hw_mtl.c      |  10 +
>>   drivers/gpu/drm/ivpu/ivpu_mmu.c         | 883 ++++++++++++++++++++++++
>>   drivers/gpu/drm/ivpu/ivpu_mmu.h         |  53 ++
>>   drivers/gpu/drm/ivpu/ivpu_mmu_context.c | 419 +++++++++++
>>   drivers/gpu/drm/ivpu/ivpu_mmu_context.h |  49 ++
>>   include/uapi/drm/ivpu_drm.h             |   4 +
>>   9 files changed, 1485 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/gpu/drm/ivpu/ivpu_mmu.c
>>   create mode 100644 drivers/gpu/drm/ivpu/ivpu_mmu.h
>>   create mode 100644 drivers/gpu/drm/ivpu/ivpu_mmu_context.c
>>   create mode 100644 drivers/gpu/drm/ivpu/ivpu_mmu_context.h
>>
>> diff --git a/drivers/gpu/drm/ivpu/Makefile b/drivers/gpu/drm/ivpu/Makefile
>> index e59dc65abe6a..95bb04f26296 100644
>> --- a/drivers/gpu/drm/ivpu/Makefile
>> +++ b/drivers/gpu/drm/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_IVPU) += intel_vpu.o
>> diff --git a/drivers/gpu/drm/ivpu/ivpu_drv.c b/drivers/gpu/drm/ivpu/ivpu_drv.c
>> index a01c7244f6e5..cbeb9a801a31 100644
>> --- a/drivers/gpu/drm/ivpu/ivpu_drv.c
>> +++ b/drivers/gpu/drm/ivpu/ivpu_drv.c
>> @@ -14,6 +14,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) "." \
>> @@ -50,6 +52,11 @@ char *ivpu_platform_to_str(u32 platform)
>>     void ivpu_file_priv_get(struct ivpu_file_priv *file_priv, struct ivpu_file_priv **link)
>>   {
>> +    struct ivpu_device *vdev = file_priv->vdev;
>> +
>> +    ivpu_dbg(KREF, "file_priv get: ctx %u refcount %u\n",
>> +         file_priv->ctx.id, kref_read(&file_priv->ref));
>> +
>>       kref_get(&file_priv->ref);
>>       *link = file_priv;
>>   }
>> @@ -57,6 +64,12 @@ void ivpu_file_priv_get(struct ivpu_file_priv *file_priv, struct ivpu_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(FILE, "file_priv release: ctx %u\n", file_priv->ctx.id);
>> +
>> +    if (file_priv->ctx.id)
>> +        ivpu_mmu_user_context_fini(file_priv);
>>         kfree(file_priv);
>>   }
>> @@ -64,6 +77,10 @@ static void file_priv_release(struct kref *ref)
>>   void ivpu_file_priv_put(struct ivpu_file_priv **link)
>>   {
>>       struct ivpu_file_priv *file_priv = *link;
>> +    struct ivpu_device *vdev = file_priv->vdev;
>> +
>> +    ivpu_dbg(KREF, "file_priv put: ctx %u refcount %u\n",
>> +         file_priv->ctx.id, kref_read(&file_priv->ref));
>>         *link = NULL;
>>       kref_put(&file_priv->ref, file_priv_release);
>> @@ -75,7 +92,11 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>>       struct ivpu_device *vdev = file_priv->vdev;
>>       struct pci_dev *pdev = to_pci_dev(vdev->drm.dev);
>>       struct drm_ivpu_param *args = data;
>> -    int ret = 0;
>> +    int ret;
>> +
>> +    ret = ivpu_mmu_user_context_init(file_priv);
>> +    if (ret)
>> +        return ret;
>>         switch (args->param) {
>>       case DRM_IVPU_PARAM_DEVICE_ID:
>> @@ -99,6 +120,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;
>>       }
>> @@ -110,7 +134,11 @@ static int ivpu_set_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>>   {
>>       struct ivpu_file_priv *file_priv = file->driver_priv;
>>       struct drm_ivpu_param *args = data;
>> -    int ret = 0;
>> +    int ret;
>> +
>> +    ret = ivpu_mmu_user_context_init(file_priv);
>> +    if (ret)
>> +        return ret;
>>         switch (args->param) {
>>       case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>> @@ -139,9 +167,13 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
>>       file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
>>         kref_init(&file_priv->ref);
>> +    mutex_init(&file_priv->lock);
>>         file->driver_priv = file_priv;
>>   +    ivpu_dbg(FILE, "file_priv alloc: process %s pid %d\n",
>> +         current->comm, task_pid_nr(current));
>> +
>>       return 0;
>>   }
>>   @@ -164,6 +196,7 @@ int ivpu_shutdown(struct ivpu_device *vdev)
>>       int ret;
>>         ivpu_hw_irq_disable(vdev);
>> +    ivpu_mmu_disable(vdev);
>>         ret = ivpu_hw_power_down(vdev);
>>       if (ret)
>> @@ -272,6 +305,10 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
>>       if (!vdev->hw)
>>           return -ENOMEM;
>>   +    vdev->mmu = devm_kzalloc(vdev->drm.dev, sizeof(*vdev->mmu), GFP_KERNEL);
>> +    if (!vdev->mmu)
>> +        return -ENOMEM;
>> +
>>       vdev->hw->ops = &ivpu_hw_mtl_ops;
>>       vdev->platform = IVPU_PLATFORM_INVALID;
>>   @@ -303,8 +340,24 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
>>           goto err_irq_fini;
>>       }
>>   +    ret = ivpu_mmu_global_context_init(vdev);
>> +    if (ret) {
>> +        ivpu_err(vdev, "Failed to initialize global MMU context: %d\n", ret);
>> +        goto err_power_down;
>> +    }
>> +
>> +    ret = ivpu_mmu_init(vdev);
>> +    if (ret) {
>> +        ivpu_err(vdev, "Failed to initialize MMU device: %d\n", ret);
>> +        goto err_mmu_gctx_fini;
>> +    }
>> +
>>       return 0;
>>   +err_mmu_gctx_fini:
>> +    ivpu_mmu_global_context_fini(vdev);
>> +err_power_down:
>> +    ivpu_hw_power_down(vdev);
>>   err_irq_fini:
>>       ivpu_irq_fini(vdev);
>>   err_pci_fini:
>> @@ -316,6 +369,8 @@ static void ivpu_dev_fini(struct ivpu_device *vdev)
>>   {
>>       ivpu_shutdown(vdev);
>>   +    ivpu_mmu_fini(vdev);
>> +    ivpu_mmu_global_context_fini(vdev);
>>       ivpu_irq_fini(vdev);
>>       ivpu_pci_fini(vdev);
>>   diff --git a/drivers/gpu/drm/ivpu/ivpu_drv.h b/drivers/gpu/drm/ivpu/ivpu_drv.h
>> index 43dfa78544c6..6eec3eb76c2f 100644
>> --- a/drivers/gpu/drm/ivpu/ivpu_drv.h
>> +++ b/drivers/gpu/drm/ivpu/ivpu_drv.h
>> @@ -14,6 +14,8 @@
>>   #include <linux/xarray.h>
>>   #include <uapi/drm/ivpu_drm.h>
>>   +#include "ivpu_mmu_context.h"
>> +
>>   #define DRIVER_NAME "intel_vpu"
>>   #define DRIVER_DESC "Driver for Intel Versatile Processing Unit (VPU)"
>>   #define DRIVER_DATE "20220913"
>> @@ -70,6 +72,7 @@ struct ivpu_wa_table {
>>   };
>>     struct ivpu_hw_info;
>> +struct ivpu_mmu_info;
>>     struct ivpu_device {
>>       struct drm_device drm; /* Must be first */
>> @@ -80,7 +83,9 @@ struct ivpu_device {
>>         struct ivpu_wa_table wa;
>>       struct ivpu_hw_info *hw;
>> +    struct ivpu_mmu_info *mmu;
>>   +    struct ivpu_mmu_context gctx;
>>       struct xarray context_xa;
>>       struct xa_limit context_xa_limit;
>>   @@ -95,6 +100,8 @@ struct ivpu_device {
>>   struct ivpu_file_priv {
>>       struct kref ref;
>>       struct ivpu_device *vdev;
>> +    struct mutex lock;
> 
> There was another lock in the mmu struct, but what's this lock for?

It is used for protecting lazy context init and cmdq added in patch 6.
The comment for this mutex is also in patch 6.
I've removed lazy context init so it will be now used only for cmdq.
The comment will also be in the correct patch.


Regards,
Jacek