On 10/25/2022 4:08 PM, Jeffrey Hugo wrote:
> On 10/25/2022 5:42 AM, Jacek Lawrynowicz wrote:
>> Hi, thanks for detailed review. My responses inline.
>>
>> On 10/25/2022 1:00 AM, Jeffrey Hugo wrote:
>>> On 9/24/2022 9:11 AM, Jacek Lawrynowicz wrote:
>>>> VPU stands for Versatile Processing Unit and it's a CPU-integrated
>>>> inference accelerator for Computer Vision and Deep Learning
>>>> applications.
>>>>
>>>> The VPU device consist of following componensts:
>
> Just noticed this. "components"
Fixed.
>>>> - Buttress - provides CPU to VPU integration, interrupt, frequency and
>>>> power management.
>>>> - Memory Management Unit (based on ARM MMU-600) - translates VPU to
>>>> host DMA addresses, isolates user workloads.
>>>> - RISC based microcontroller - executes firmware that provides job
>>>> execution API for the kernel-mode driver
>>>> - Neural Compute Subsystem (NCS) - does the actual work, provides
>>>> Compute and Copy engines.
>>>> - Network on Chip (NoC) - network fabric connecting all the components
>>>>
>>>> This driver supports VPU IP v2.7 integrated into Intel Meteor Lake
>>>> client CPUs (14th generation).
>>>>
>>>> Module sources are at drivers/gpu/drm/ivpu and module name is
>>>> "intel_vpu.ko".
>>>>
>>>> This patch includes only very besic functionality:
>>>> - module, PCI device and IRQ initialization
>>>> - register definitions and low level register manipulation functions
>>>> - SET/GET_PARAM ioctls
>>>> - power up without firmware
>>>>
>>>> Signed-off-by: Krystian Pradzynski <krystian.pradzyn...@linux.intel.com>
>>>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynow...@linux.intel.com>
>>>> ---
>>>> MAINTAINERS | 8 +
>>>> drivers/gpu/drm/Kconfig | 2 +
>>>> drivers/gpu/drm/Makefile | 1 +
>>>> drivers/gpu/drm/ivpu/Kconfig | 12 +
>>>> drivers/gpu/drm/ivpu/Makefile | 8 +
>>>> drivers/gpu/drm/ivpu/TODO | 7 +
>>>> drivers/gpu/drm/ivpu/ivpu_drv.c | 392 +++++++++
>>>> drivers/gpu/drm/ivpu/ivpu_drv.h | 153 ++++
>>>> drivers/gpu/drm/ivpu/ivpu_hw.h | 169 ++++
>>>> drivers/gpu/drm/ivpu/ivpu_hw_mtl.c | 1021 ++++++++++++++++++++++++
>>>> drivers/gpu/drm/ivpu/ivpu_hw_mtl_reg.h | 468 +++++++++++
>>>> drivers/gpu/drm/ivpu/ivpu_hw_reg_io.h | 115 +++
>>>> include/uapi/drm/ivpu_drm.h | 95 +++
>>>> 13 files changed, 2451 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/ivpu/Kconfig
>>>> create mode 100644 drivers/gpu/drm/ivpu/Makefile
>>>> create mode 100644 drivers/gpu/drm/ivpu/TODO
>>>> create mode 100644 drivers/gpu/drm/ivpu/ivpu_drv.c
>>>> create mode 100644 drivers/gpu/drm/ivpu/ivpu_drv.h
>>>> create mode 100644 drivers/gpu/drm/ivpu/ivpu_hw.h
>>>> create mode 100644 drivers/gpu/drm/ivpu/ivpu_hw_mtl.c
>>>> create mode 100644 drivers/gpu/drm/ivpu/ivpu_hw_mtl_reg.h
>>>> create mode 100644 drivers/gpu/drm/ivpu/ivpu_hw_reg_io.h
>>>> create mode 100644 include/uapi/drm/ivpu_drm.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 9475aa701a3f..d72ceef107e6 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -7046,6 +7046,14 @@ F:
>>>> Documentation/devicetree/bindings/gpu/vivante,gc.yaml
>>>> F: drivers/gpu/drm/etnaviv/
>>>> F: include/uapi/drm/etnaviv_drm.h
>>>> +DRM DRIVERS FOR VPU
>>>> +M: Jacek Lawrynowicz <jacek.lawrynow...@linux.intel.com>
>>>> +M: Stanislaw Gruszka <stanislaw.grus...@linux.intel.com>
>>>> +S: Supported
>>>> +T: git git://anongit.freedesktop.org/drm/drm-misc
>>>> +F: drivers/gpu/drm/ivpu/
>>>> +F: include/uapi/drm/ivpu_drm.h
>>>
>>> No mail list?
>>
>> OK, I will add a link to dri-devel.
>>
>>>> +
>>>> DRM DRIVERS FOR XEN
>>>> M: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>> L: dri-devel@lists.freedesktop.org
>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>>> index 198ba846d34b..0aaac0e5366f 100644
>>>> --- a/drivers/gpu/drm/Kconfig
>>>> +++ b/drivers/gpu/drm/Kconfig
>>>> @@ -364,6 +364,8 @@ source "drivers/gpu/drm/v3d/Kconfig"
>>>> source "drivers/gpu/drm/vc4/Kconfig"
>>>> +source "drivers/gpu/drm/ivpu/Kconfig"
>>>> +
>>>
>>> Why here of all places? Just randomly in the middle of the list of sourced
>>> Kconfigs?
>>
>> I'll move it to the end.
>>
>>>> source "drivers/gpu/drm/etnaviv/Kconfig"
>>>> source "drivers/gpu/drm/hisilicon/Kconfig"
>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>> index 25d0ba310509..1bfd7415c2d0 100644
>>>> --- a/drivers/gpu/drm/Makefile
>>>> +++ b/drivers/gpu/drm/Makefile
>>>> @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_KMB_DISPLAY) += kmb/
>>>> obj-$(CONFIG_DRM_MGAG200) += mgag200/
>>>> obj-$(CONFIG_DRM_V3D) += v3d/
>>>> obj-$(CONFIG_DRM_VC4) += vc4/
>>>> +obj-$(CONFIG_DRM_IVPU) += ivpu/
>>>
>>> Again, why here?
>>
>> I'll move it to the end.
>>
>>>> diff --git a/drivers/gpu/drm/ivpu/Makefile b/drivers/gpu/drm/ivpu/Makefile
>>>> new file mode 100644
>>>> index 000000000000..e59dc65abe6a
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/ivpu/Makefile
>>>> @@ -0,0 +1,8 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>> +# Copyright © 2022 Intel Corporation
>>>
>>> I'm pretty sure (C) is preferred. Looks like you do this in multiple
>>> places. I'm only going to mention it here.
>>
>> OK, I'll use (C) everywhere.
>>
>>>> +int ivpu_dbg_mask;
>>>> +module_param_named(dbg_mask, ivpu_dbg_mask, int, 0644);
>>>> +MODULE_PARM_DESC(dbg_mask, "Driver debug mask. See IVPU_DBG_* macros.");
>>>
>>> Shouldn't this be unnecessary with the DRM_DEBUG levels, or making the
>>> things tracepoints?
>>
>> drm logging doesn't provide the granuality we need.
>> We plan to add tracepoints in future patches.
>>
>>>> +char *ivpu_platform_to_str(u32 platform)
>>>> +{
>>>> + switch (platform) {
>>>> + case IVPU_PLATFORM_SILICON:
>>>> + return "IVPU_PLATFORM_SILICON";
>>>> + case IVPU_PLATFORM_SIMICS:
>>>> + return "IVPU_PLATFORM_SIMICS";
>>>> + case IVPU_PLATFORM_FPGA:
>>>> + return "IVPU_PLATFORM_FPGA";
>>>> + default:
>>>> + return "Invalid platform";
>>>> + }
>>>> +}
>>>
>>> In this entire series, this is only used in this patch, and only in one
>>> file. Seems pointless to define it here, and have it in the header. Why
>>> shouldn't this be moved to the file it is used in, and made static?
>>
>> OK, I'll move it.
>>
>>>> +static int ivpu_get_param_ioctl(struct drm_device *dev, void *data,
>>>> struct drm_file *file)
>>>> +{
>>>> + struct ivpu_file_priv *file_priv = file->driver_priv;
>>>> + 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;
>>>> + switch (args->param) {
>>>> + case DRM_IVPU_PARAM_DEVICE_ID:
>>>> + args->value = pdev->device;
>>>> + break;
>>>> + case DRM_IVPU_PARAM_DEVICE_REVISION:
>>>> + args->value = pdev->revision;
>>>> + break;
>>>> + case DRM_IVPU_PARAM_PLATFORM_TYPE:
>>>> + args->value = vdev->platform;
>>>> + break;
>>>> + case DRM_IVPU_PARAM_CORE_CLOCK_RATE:
>>>> + args->value = ivpu_hw_reg_pll_freq_get(vdev);
>>>> + break;
>>>> + case DRM_IVPU_PARAM_NUM_CONTEXTS:
>>>> + args->value = ivpu_get_context_count(vdev);
>>>> + break;
>>>> + case DRM_IVPU_PARAM_CONTEXT_BASE_ADDRESS:
>>>> + args->value = vdev->hw->ranges.user_low.start;
>>>> + break;
>>>> + case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
>>>> + args->value = file_priv->priority;
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
>>>
>>> This doesn't cause a switch case fallthrough warning?
>>
>> No, but I will add break for consistency.
>>
>>>> +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;
>>>> +
>>>> + file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
>>>> + if (!file_priv)
>>>> + return -ENOMEM;
>>>> +
>>>> + file_priv->vdev = vdev;
>>>> + file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
>>>> +
>>>> + kref_init(&file_priv->ref);
>>>
>>> VFS is going to maintain a refcount on the fd. This looks like you are
>>> duplicating that ref count, which seems pointless.
>>>
>>> Later on you use this for jobs, as each job takes a ref, but why would it
>>> be valid for jobs to hang around after the fd is closed?
>>
>> This allows user space to close fd immediately without blocking the process
>> in case the job is still being processed by the HW.
>
> Eh. Ok. Maybe add a comment to that effect?
OK.
Regards,
Jacek
