Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU

[Oded Gabbay <oded.gabbay@gmail.com>]

On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz
<jacek.lawrynowicz@linux.intel.com> wrote:
> +
> +static inline int ivpu_hw_info_init(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->info_init(vdev);
> +};
> +
> +static inline int ivpu_hw_power_up(struct ivpu_device *vdev)
> +{
> +       ivpu_dbg(vdev, PM, "HW power up\n");
> +
> +       return vdev->hw->ops->power_up(vdev);
> +};
> +
> +static inline int ivpu_hw_boot_fw(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->boot_fw(vdev);
> +};
> +
> +static inline bool ivpu_hw_is_idle(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->is_idle(vdev);
> +};
> +
> +static inline int ivpu_hw_power_down(struct ivpu_device *vdev)
> +{
> +       ivpu_dbg(vdev, PM, "HW power down\n");
> +
> +       return vdev->hw->ops->power_down(vdev);
> +};
> +
> +static inline void ivpu_hw_wdt_disable(struct ivpu_device *vdev)
> +{
> +       vdev->hw->ops->wdt_disable(vdev);
> +};
> +
> +/* Register indirect accesses */
> +static inline u32 ivpu_hw_reg_pll_freq_get(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->reg_pll_freq_get(vdev);
> +};
> +
> +static inline u32 ivpu_hw_reg_telemetry_offset_get(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->reg_telemetry_offset_get(vdev);
> +};
> +
> +static inline u32 ivpu_hw_reg_telemetry_size_get(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->reg_telemetry_size_get(vdev);
> +};
> +
> +static inline u32 ivpu_hw_reg_telemetry_enable_get(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->reg_telemetry_enable_get(vdev);
> +};
> +
> +static inline void ivpu_hw_reg_db_set(struct ivpu_device *vdev, u32 db_id)
> +{
> +       vdev->hw->ops->reg_db_set(vdev, db_id);
> +};
> +
> +static inline u32 ivpu_hw_reg_ipc_rx_addr_get(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->reg_ipc_rx_addr_get(vdev);
> +};
> +
> +static inline u32 ivpu_hw_reg_ipc_rx_count_get(struct ivpu_device *vdev)
> +{
> +       return vdev->hw->ops->reg_ipc_rx_count_get(vdev);
> +};
> +
> +static inline void ivpu_hw_reg_ipc_tx_set(struct ivpu_device *vdev, u32 vpu_addr)
> +{
> +       vdev->hw->ops->reg_ipc_tx_set(vdev, vpu_addr);
> +};
> +
> +static inline void ivpu_hw_irq_clear(struct ivpu_device *vdev)
> +{
> +       vdev->hw->ops->irq_clear(vdev);
> +};
> +
> +static inline void ivpu_hw_irq_enable(struct ivpu_device *vdev)
> +{
> +       vdev->hw->ops->irq_enable(vdev);
> +};
> +
> +static inline void ivpu_hw_irq_disable(struct ivpu_device *vdev)
> +{
> +       vdev->hw->ops->irq_disable(vdev);
> +};
Why hide all the calls to the hw ops functions inside inline functions ?
I mean, it just makes it one step longer to read the code...
Why not directly call vdev->hw->ops->operation ?

> diff --git a/include/uapi/drm/ivpu_drm.h b/include/uapi/drm/ivpu_drm.h
> new file mode 100644
> index 000000000000..922cbf30ce34
> --- /dev/null
> +++ b/include/uapi/drm/ivpu_drm.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +/*
> + * Copyright (C) 2020-2022 Intel Corporation
> + */
> +
> +#ifndef __UAPI_IVPU_DRM_H__
> +#define __UAPI_IVPU_DRM_H__
> +
> +#include "drm.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +#define DRM_IVPU_DRIVER_MAJOR 1
> +#define DRM_IVPU_DRIVER_MINOR 0
I would prefer not to define versions for the driver. Don't get in
this trap of trying to keep a version
number updated over time.  It breaks down the moment you get the code
merged into the kernel tree as the driver is what is in the kernel, not
separate from it.

Also think of the stable kernels, you will backport fixes to those for
the driver as part of the development process.  So what "version" are
they now?  They are some mis-match of the old one with new fixes, and as
such, the version number now means nothing.

Just stick to the version number of the kernel itself, that's the only
sane way.

btw, I talked to Daniel about this and he told me this
major/minor/patchlevel is legacy in drm and should be deprecated, so
I'm going to send a patch to document that it is deprecated and how to
use getcap ioctl to find out the features the driver support.

Thanks,
Oded