From: Oded Gabbay <oded.gabbay@gmail.com>
To: Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>,
tzimmermann@suse.de, dri-devel@lists.freedesktop.org,
stanislaw.gruszka@linux.intel.com,
Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>,
andrzej.kacprowski@linux.intel.com
Subject: Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
Date: Wed, 14 Dec 2022 20:21:57 +0200 [thread overview]
Message-ID: <CAFCwf12owW9r6ePPEcTxAt_W1H3UW35gpM-r0KqGrMSDUQs9uw@mail.gmail.com> (raw)
In-Reply-To: <d7ab7c6b-cbeb-b140-ac7a-0209fa627308@quicinc.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
next prev parent reply other threads:[~2022-12-14 18:22 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-08 11:07 [PATCH v4 0/7] New DRM accel driver for Intel VPU Jacek Lawrynowicz
2022-12-08 11:07 ` [PATCH v4 1/7] accel/ivpu: Introduce a new DRM " Jacek Lawrynowicz
2022-12-14 13:57 ` Oded Gabbay
2022-12-14 15:07 ` Jeffrey Hugo
2022-12-14 18:21 ` Oded Gabbay [this message]
2022-12-14 15:39 ` Stanislaw Gruszka
2022-12-20 20:17 ` Oded Gabbay
2022-12-21 8:25 ` Jacek Lawrynowicz
2023-01-05 12:57 ` Daniel Vetter
2023-01-05 16:25 ` Jeffrey Hugo
2023-01-05 17:38 ` Oded Gabbay
2023-01-06 9:28 ` Daniel Vetter
2023-01-06 9:56 ` Stanislaw Gruszka
2023-01-06 10:44 ` Daniel Vetter
2023-01-06 11:43 ` Oded Gabbay
2023-01-11 4:35 ` Dave Airlie
2023-01-06 12:43 ` Stanislaw Gruszka
2022-12-08 11:07 ` [PATCH v4 2/7] accel/ivpu: Add Intel VPU MMU support Jacek Lawrynowicz
2022-12-12 11:19 ` kernel test robot
2022-12-18 9:13 ` Oded Gabbay
2022-12-19 13:17 ` Jacek Lawrynowicz
2022-12-08 11:07 ` [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management Jacek Lawrynowicz
2022-12-12 15:31 ` kernel test robot
2022-12-18 10:23 ` Oded Gabbay
2022-12-19 8:08 ` Jacek Lawrynowicz
2023-01-05 18:46 ` Andrew Davis
2023-01-06 13:29 ` Stanislaw Gruszka
2023-01-09 11:47 ` Jacek Lawrynowicz
2023-01-09 18:09 ` Andrew Davis
2023-01-06 10:50 ` Daniel Vetter
2023-01-06 13:22 ` Stanislaw Gruszka
2023-01-06 18:25 ` Daniel Vetter
2023-01-09 12:06 ` Jacek Lawrynowicz
2022-12-08 11:07 ` [PATCH v4 4/7] accel/ivpu: Add IPC driver and JSM messages Jacek Lawrynowicz
2022-12-27 15:34 ` Oded Gabbay
2023-01-03 10:54 ` Jacek Lawrynowicz
2022-12-08 11:07 ` [PATCH v4 5/7] accel/ivpu: Implement firmware parsing and booting Jacek Lawrynowicz
2022-12-08 11:07 ` [PATCH v4 6/7] accel/ivpu: Add command buffer submission logic Jacek Lawrynowicz
2022-12-08 11:07 ` [PATCH v4 7/7] accel/ivpu: Add PM support Jacek Lawrynowicz
2022-12-13 11:36 ` [PATCH v4 8/7] accel/ivpu: Add depend on !UML to Kconfig Stanislaw Gruszka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFCwf12owW9r6ePPEcTxAt_W1H3UW35gpM-r0KqGrMSDUQs9uw@mail.gmail.com \
--to=oded.gabbay@gmail.com \
--cc=andrzej.kacprowski@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=jacek.lawrynowicz@linux.intel.com \
--cc=krystian.pradzynski@linux.intel.com \
--cc=quic_jhugo@quicinc.com \
--cc=stanislaw.gruszka@linux.intel.com \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).