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

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

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).

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 
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.

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.