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

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Wed, Dec 14, 2022 at 03:57:54PM +0200, 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 ?

It allows to extend the calls for code that are common between individual
implementations.

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

Yes, it's not optimal to identify features using major/minor version and
we do not relay heavily on this. For now we expect that some features are
present and working since particular major/minor version and mark feature
tests failed or skipped accordingly.

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

Cool.

Regards
Stanislaw