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

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

On Wed, Dec 14, 2022 at 5:07 PM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 12/14/2022 6:57 AM, Oded Gabbay wrote:
> > 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.
>
> This seems to assume that the user is running whole kernels and not a
> backport of the driver (similar to the backports project that is popular
> with net).
That's what the kernel upstream community usually assumes ;)
>
> Lets assume this gets merged into 6.3.  End user is on say Ubuntu 20.04
> which GA'd with a 5.4 kernel and is maintaining that.  Intel here
> backports the 6.3 driver to 5.4 to support this end user and tries to do
> the "right thing" by using the upstream code instead of some downstream
> fork.  Now the kernel version is meaningless because 5.4 never had this
If you use some kind of get_cap ioctl in your driver, your userspace
can inquire about supported features, without need for driver version.
This can also allow you to take-out certain features from specific versions.

> driver.  Of course if the user reports an issue to Intel, it would be
> handy to know exactly what version of the driver the user has.
Assuming you install your driver as out-of-tree, you could keep
numbering in the dkms package.

>
> The src version hash that modinfo provides is meaningless because it
> changes based on the entirety of the build, which includes things like
> the compiler used.
>
> If there is an alternative solution for this, I'd love to hear it.
What I said above.
Having said that, I'm not going to insist on this, it's bike-shedding