[PATCH 10/10] accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY
Jeffrey Hugo
quic_jhugo at quicinc.com
Fri Jan 5 17:29:21 UTC 2024
On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <karol.wachowski at intel.com>
>
> DRM_IVPU_PARAM_CONTEXT_PRIORITY has been deprecated because it
> has been replaced with DRM_IVPU_JOB_PRIORITY levels set with
> submit IOCTL and was unused anyway.
>
> Signed-off-by: Wachowski, Karol <karol.wachowski at intel.com>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz at linux.intel.com>
> ---
> drivers/accel/ivpu/ivpu_drv.c | 11 -----------
> drivers/accel/ivpu/ivpu_drv.h | 1 -
> drivers/accel/ivpu/ivpu_job.c | 3 +++
> include/uapi/drm/ivpu_accel.h | 21 ++++++++++++++++++++-
> 4 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index ec66c2c39877..546c0899bb9e 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -177,9 +177,6 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
> case DRM_IVPU_PARAM_CONTEXT_BASE_ADDRESS:
> args->value = vdev->hw->ranges.user.start;
> break;
> - case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
> - args->value = file_priv->priority;
> - break;
> case DRM_IVPU_PARAM_CONTEXT_ID:
> args->value = file_priv->ctx.id;
> break;
> @@ -219,17 +216,10 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
>
> static int ivpu_set_param_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> {
> - struct ivpu_file_priv *file_priv = file->driver_priv;
> struct drm_ivpu_param *args = data;
> int ret = 0;
>
> switch (args->param) {
> - case DRM_IVPU_PARAM_CONTEXT_PRIORITY:
> - if (args->value <= DRM_IVPU_CONTEXT_PRIORITY_REALTIME)
> - file_priv->priority = args->value;
> - else
> - ret = -EINVAL;
> - break;
> default:
> ret = -EINVAL;
> }
> @@ -258,7 +248,6 @@ static int ivpu_open(struct drm_device *dev, struct drm_file *file)
> }
>
> file_priv->vdev = vdev;
> - file_priv->priority = DRM_IVPU_CONTEXT_PRIORITY_NORMAL;
> kref_init(&file_priv->ref);
> mutex_init(&file_priv->lock);
>
> diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> index 9b6e336626e3..7a6bc1918780 100644
> --- a/drivers/accel/ivpu/ivpu_drv.h
> +++ b/drivers/accel/ivpu/ivpu_drv.h
> @@ -146,7 +146,6 @@ struct ivpu_file_priv {
> struct mutex lock; /* Protects cmdq */
> struct ivpu_cmdq *cmdq[IVPU_NUM_ENGINES];
> struct ivpu_mmu_context ctx;
> - u32 priority;
> bool has_mmu_faults;
> };
>
> diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
> index 7206cf9cdb4a..82e40bb4803c 100644
> --- a/drivers/accel/ivpu/ivpu_job.c
> +++ b/drivers/accel/ivpu/ivpu_job.c
> @@ -488,6 +488,9 @@ int ivpu_submit_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> if (params->engine > DRM_IVPU_ENGINE_COPY)
> return -EINVAL;
>
> + if (params->priority > DRM_IVPU_JOB_PRIORITY_REALTIME)
> + return -EINVAL;
> +
> if (params->buffer_count == 0 || params->buffer_count > JOB_MAX_BUFFER_COUNT)
> return -EINVAL;
>
> diff --git a/include/uapi/drm/ivpu_accel.h b/include/uapi/drm/ivpu_accel.h
> index de1944e42c65..cc9a0504ee2f 100644
> --- a/include/uapi/drm/ivpu_accel.h
> +++ b/include/uapi/drm/ivpu_accel.h
> @@ -13,7 +13,7 @@ extern "C" {
> #endif
>
> #define DRM_IVPU_DRIVER_MAJOR 1
> -#define DRM_IVPU_DRIVER_MINOR 0
> +#define DRM_IVPU_DRIVER_MINOR 1
I remember when this driver was going through initial review before
acceptance, Oded mentioned that the DRM driver version mechanism was
deprecated and to not use it. Based on that, it seems like you should
not be incrementing the minor number.
>
> #define DRM_IVPU_GET_PARAM 0x00
> #define DRM_IVPU_SET_PARAM 0x01
> @@ -64,11 +64,18 @@ extern "C" {
>
> #define DRM_IVPU_PLATFORM_TYPE_SILICON 0
>
> +/* Deprecated - to be removed */
> #define DRM_IVPU_CONTEXT_PRIORITY_IDLE 0
> #define DRM_IVPU_CONTEXT_PRIORITY_NORMAL 1
> #define DRM_IVPU_CONTEXT_PRIORITY_FOCUS 2
> #define DRM_IVPU_CONTEXT_PRIORITY_REALTIME 3
$SUBJECT suggests these are being removed, not just deprecated. Also,
shouldn't DRM_IVPU_PARAM_CONTEXT_PRIORITY which is a few lines above
this be deprecated/removed/something?
>
> +#define DRM_IVPU_JOB_PRIORITY_DEFAULT 0
> +#define DRM_IVPU_JOB_PRIORITY_IDLE 1
> +#define DRM_IVPU_JOB_PRIORITY_NORMAL 2
> +#define DRM_IVPU_JOB_PRIORITY_FOCUS 3
> +#define DRM_IVPU_JOB_PRIORITY_REALTIME 4
> +
> /**
> * DRM_IVPU_CAP_METRIC_STREAMER
> *
> @@ -286,6 +293,18 @@ struct drm_ivpu_submit {
> * to be executed. The offset has to be 8-byte aligned.
> */
> __u32 commands_offset;
> +
> + /**
> + * @priority:
> + *
> + * Priority to be set for related job command queue, can be one of the following:
> + * %DRM_IVPU_JOB_PRIORITY_DEFAULT
> + * %DRM_IVPU_JOB_PRIORITY_IDLE
> + * %DRM_IVPU_JOB_PRIORITY_NORMAL
> + * %DRM_IVPU_JOB_PRIORITY_FOCUS
> + * %DRM_IVPU_JOB_PRIORITY_REALTIME
> + */
> + __u32 priority;
I think this breaks the uapi (which makes me think you are using the
driver minor version above to detect). This struct is passed to DRM_IOW
which uses the struct size to calculate the ioctl number. By changing
the size of this struct, you change the ioctl number, and make it so
that old userspace (with the old number) cannot work with newer kernels.
I beleive last time I brought up a uapi breakage, I was told that your
userspace han't been offically released yet. Is that still the case?
Seems odd though, because you are incrementing the driver minor number
above which makes me think you need to communicate this change to
userspace, which seems to suggest you might have old userspace out in
the wild...
More information about the dri-devel
mailing list