[PATCH 10/10] accel/ivpu: Remove deprecated DRM_IVPU_PARAM_CONTEXT_PRIORITY
Jeffrey Hugo
quic_jhugo at quicinc.com
Thu Jan 11 21:03:58 UTC 2024
On 1/10/2024 7:33 AM, Jacek Lawrynowicz wrote:
> On 05.01.2024 18:29, Jeffrey Hugo wrote:
>> 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.
>
> 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.
Intresting. You are right, drm_ioctl does extra handling to ignore the
ioctl size. As long as the struct change is backwards compatible
everything will work fine.
> 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.
DRM_IOW is defined as _IOW which is defined as _IOC. _IOC calls
_IOC_TYPECHECK which looks to be defined as a sizeof. The actual _IOC
macro uses the 4 paramaters in bitwise math to generate the ioctl
number. I probably should have clarified eariler that I'm considering
the ioctl number to be the 32 bit value "returned" by DRM_IOW, and not
the NR field.
Anyways, as you pointed out, I had forgotten an element of how DRM
handles ioctls.
-Jeff
More information about the dri-devel
mailing list