On 05.01.2024 18:29, Jeffrey Hugo wrote:
> On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
>> From: "Wachowski, Karol" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Jacek Lawrynowicz <[email protected]>
>> ---
>> 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.
I wanted to use minor version in tests to verify the UAPI but this is not very
important. I can leave this as is.
>> #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?
OK, I'll correct the subject and add "deprecated" comment to
DRM_IVPU_PARAM_CONTEXT_PRIORITY.
>> +#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...
The user-space part of the driver was already released but it have never used
DRM_IVPU_PARAM_CONTEXT_PRIORITY.
I've tested the new kmd with old umd and ioctls work fine. drm_ioctl() handles
the difference in user vs driver arg size.
I think it is perfectly safe to extend the ioctl arg. The ioctl number is
passed directly to DRM_IOW(), I can't see where it would be calculated based on
arg size.
Regards,
Jacek
