| Message ID | 20241213232933.1545388-6-lizhi.hou@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD NPU driver improvements | expand |
On 12/13/2024 17:29, Lizhi Hou wrote: > For input structures, it is better to check if the pad is zero. > Thus, the pad bytes might be usable in the future. > IIRC you should pick up: Suggested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> > --- > drivers/accel/amdxdna/aie2_ctx.c | 3 +++ > drivers/accel/amdxdna/aie2_message.c | 3 +++ > drivers/accel/amdxdna/amdxdna_ctx.c | 6 ++++++ > drivers/accel/amdxdna/amdxdna_gem.c | 2 +- > include/uapi/drm/amdxdna_accel.h | 11 +++++------ > 5 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c > index cdeead75c6f5..9facf45818f9 100644 > --- a/drivers/accel/amdxdna/aie2_ctx.c > +++ b/drivers/accel/amdxdna/aie2_ctx.c > @@ -690,6 +690,9 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size > int ret; > > XDNA_DBG(xdna, "Config %d CU to %s", config->num_cus, hwctx->name); > + if (XDNA_MBZ_DBG(xdna, config->pad, sizeof(config->pad))) > + return -EINVAL; > + > if (hwctx->status != HWCTX_STAT_INIT) { > XDNA_ERR(xdna, "Not support re-config CU"); > return -EINVAL; > diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c > index b2ca78cfd0a7..9e2c9a44f76a 100644 > --- a/drivers/accel/amdxdna/aie2_message.c > +++ b/drivers/accel/amdxdna/aie2_message.c > @@ -395,6 +395,9 @@ int aie2_config_cu(struct amdxdna_hwctx *hwctx) > for (i = 0; i < hwctx->cus->num_cus; i++) { > struct amdxdna_cu_config *cu = &hwctx->cus->cu_configs[i]; > > + if (XDNA_MBZ_DBG(xdna, cu->pad, sizeof(cu->pad))) > + return -EINVAL; > + > gobj = drm_gem_object_lookup(hwctx->client->filp, cu->cu_bo); > if (!gobj) { > XDNA_ERR(xdna, "Lookup GEM object failed"); > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c > index 324f35c43f6c..d11b1c83d9c3 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c > @@ -243,6 +243,9 @@ int amdxdna_drm_destroy_hwctx_ioctl(struct drm_device *dev, void *data, struct d > struct amdxdna_hwctx *hwctx; > int ret = 0, idx; > > + if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad))) > + return -EINVAL; > + > if (!drm_dev_enter(dev, &idx)) > return -ENODEV; > > @@ -277,6 +280,9 @@ int amdxdna_drm_config_hwctx_ioctl(struct drm_device *dev, void *data, struct dr > void *buf; > u64 val; > > + if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad))) > + return -EINVAL; > + > if (!xdna->dev_info->ops->hwctx_config) > return -EOPNOTSUPP; > > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c > index 4dfeca306d98..606433d73236 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -552,7 +552,7 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm > struct drm_gem_object *gobj; > int ret = 0; > > - if (args->ext || args->ext_flags) > + if (args->ext || args->ext_flags || args->pad) > return -EINVAL; > > gobj = drm_gem_object_lookup(filp, args->handle); > diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h > index e4edb52bc27b..a706ead39082 100644 > --- a/include/uapi/drm/amdxdna_accel.h > +++ b/include/uapi/drm/amdxdna_accel.h > @@ -87,7 +87,7 @@ struct amdxdna_drm_create_hwctx { > /** > * struct amdxdna_drm_destroy_hwctx - Destroy hardware context. > * @handle: Hardware context handle. > - * @pad: Structure padding. > + * @pad: MBZ. > */ > struct amdxdna_drm_destroy_hwctx { > __u32 handle; > @@ -98,7 +98,7 @@ struct amdxdna_drm_destroy_hwctx { > * struct amdxdna_cu_config - configuration for one CU > * @cu_bo: CU configuration buffer bo handle. > * @cu_func: Function of a CU. > - * @pad: Structure padding. > + * @pad: MBZ. > */ > struct amdxdna_cu_config { > __u32 cu_bo; > @@ -109,7 +109,7 @@ struct amdxdna_cu_config { > /** > * struct amdxdna_hwctx_param_config_cu - configuration for CUs in hardware context > * @num_cus: Number of CUs to configure. > - * @pad: Structure padding. > + * @pad: MBZ. > * @cu_configs: Array of CU configurations of struct amdxdna_cu_config. > */ > struct amdxdna_hwctx_param_config_cu { > @@ -122,7 +122,6 @@ enum amdxdna_drm_config_hwctx_param { > DRM_AMDXDNA_HWCTX_CONFIG_CU, > DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF, > DRM_AMDXDNA_HWCTX_REMOVE_DBG_BUF, > - DRM_AMDXDNA_HWCTX_CONFIG_NUM This change actually meant here? It seems like it should be it's own change. > }; > > /** > @@ -133,7 +132,7 @@ enum amdxdna_drm_config_hwctx_param { > * @param_val: A structure specified by the param_type struct member. > * @param_val_size: Size of the parameter buffer pointed to by the param_val. > * If param_val is not a pointer, driver can ignore this. > - * @pad: Structure padding. > + * @pad: MBZ. > * > * Note: if the param_val is a pointer pointing to a buffer, the maximum size > * of the buffer is 4KiB(PAGE_SIZE). > @@ -175,7 +174,7 @@ struct amdxdna_drm_create_bo { > * @ext: MBZ. > * @ext_flags: MBZ. > * @handle: DRM buffer object handle. > - * @pad: Structure padding. > + * @pad: MBZ. > * @map_offset: Returned DRM fake offset for mmap(). > * @vaddr: Returned user VA of buffer. 0 in case user needs mmap(). > * @xdna_addr: Returned XDNA device virtual address.
On 12/16/24 12:38, Mario Limonciello wrote: > On 12/13/2024 17:29, Lizhi Hou wrote: >> For input structures, it is better to check if the pad is zero. >> Thus, the pad bytes might be usable in the future. >> > > IIRC you should pick up: > Suggested-by: Jeffrey Hugo <quic_jhugo@quicinc.com> Sure. Will add it. > >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> >> --- >> drivers/accel/amdxdna/aie2_ctx.c | 3 +++ >> drivers/accel/amdxdna/aie2_message.c | 3 +++ >> drivers/accel/amdxdna/amdxdna_ctx.c | 6 ++++++ >> drivers/accel/amdxdna/amdxdna_gem.c | 2 +- >> include/uapi/drm/amdxdna_accel.h | 11 +++++------ >> 5 files changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/accel/amdxdna/aie2_ctx.c >> b/drivers/accel/amdxdna/aie2_ctx.c >> index cdeead75c6f5..9facf45818f9 100644 >> --- a/drivers/accel/amdxdna/aie2_ctx.c >> +++ b/drivers/accel/amdxdna/aie2_ctx.c >> @@ -690,6 +690,9 @@ static int aie2_hwctx_cu_config(struct >> amdxdna_hwctx *hwctx, void *buf, u32 size >> int ret; >> XDNA_DBG(xdna, "Config %d CU to %s", config->num_cus, >> hwctx->name); >> + if (XDNA_MBZ_DBG(xdna, config->pad, sizeof(config->pad))) >> + return -EINVAL; >> + >> if (hwctx->status != HWCTX_STAT_INIT) { >> XDNA_ERR(xdna, "Not support re-config CU"); >> return -EINVAL; >> diff --git a/drivers/accel/amdxdna/aie2_message.c >> b/drivers/accel/amdxdna/aie2_message.c >> index b2ca78cfd0a7..9e2c9a44f76a 100644 >> --- a/drivers/accel/amdxdna/aie2_message.c >> +++ b/drivers/accel/amdxdna/aie2_message.c >> @@ -395,6 +395,9 @@ int aie2_config_cu(struct amdxdna_hwctx *hwctx) >> for (i = 0; i < hwctx->cus->num_cus; i++) { >> struct amdxdna_cu_config *cu = &hwctx->cus->cu_configs[i]; >> + if (XDNA_MBZ_DBG(xdna, cu->pad, sizeof(cu->pad))) >> + return -EINVAL; >> + >> gobj = drm_gem_object_lookup(hwctx->client->filp, cu->cu_bo); >> if (!gobj) { >> XDNA_ERR(xdna, "Lookup GEM object failed"); >> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c >> b/drivers/accel/amdxdna/amdxdna_ctx.c >> index 324f35c43f6c..d11b1c83d9c3 100644 >> --- a/drivers/accel/amdxdna/amdxdna_ctx.c >> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c >> @@ -243,6 +243,9 @@ int amdxdna_drm_destroy_hwctx_ioctl(struct >> drm_device *dev, void *data, struct d >> struct amdxdna_hwctx *hwctx; >> int ret = 0, idx; >> + if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad))) >> + return -EINVAL; >> + >> if (!drm_dev_enter(dev, &idx)) >> return -ENODEV; >> @@ -277,6 +280,9 @@ int amdxdna_drm_config_hwctx_ioctl(struct >> drm_device *dev, void *data, struct dr >> void *buf; >> u64 val; >> + if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad))) >> + return -EINVAL; >> + >> if (!xdna->dev_info->ops->hwctx_config) >> return -EOPNOTSUPP; >> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c >> b/drivers/accel/amdxdna/amdxdna_gem.c >> index 4dfeca306d98..606433d73236 100644 >> --- a/drivers/accel/amdxdna/amdxdna_gem.c >> +++ b/drivers/accel/amdxdna/amdxdna_gem.c >> @@ -552,7 +552,7 @@ int amdxdna_drm_get_bo_info_ioctl(struct >> drm_device *dev, void *data, struct drm >> struct drm_gem_object *gobj; >> int ret = 0; >> - if (args->ext || args->ext_flags) >> + if (args->ext || args->ext_flags || args->pad) >> return -EINVAL; >> gobj = drm_gem_object_lookup(filp, args->handle); >> diff --git a/include/uapi/drm/amdxdna_accel.h >> b/include/uapi/drm/amdxdna_accel.h >> index e4edb52bc27b..a706ead39082 100644 >> --- a/include/uapi/drm/amdxdna_accel.h >> +++ b/include/uapi/drm/amdxdna_accel.h >> @@ -87,7 +87,7 @@ struct amdxdna_drm_create_hwctx { >> /** >> * struct amdxdna_drm_destroy_hwctx - Destroy hardware context. >> * @handle: Hardware context handle. >> - * @pad: Structure padding. >> + * @pad: MBZ. >> */ >> struct amdxdna_drm_destroy_hwctx { >> __u32 handle; >> @@ -98,7 +98,7 @@ struct amdxdna_drm_destroy_hwctx { >> * struct amdxdna_cu_config - configuration for one CU >> * @cu_bo: CU configuration buffer bo handle. >> * @cu_func: Function of a CU. >> - * @pad: Structure padding. >> + * @pad: MBZ. >> */ >> struct amdxdna_cu_config { >> __u32 cu_bo; >> @@ -109,7 +109,7 @@ struct amdxdna_cu_config { >> /** >> * struct amdxdna_hwctx_param_config_cu - configuration for CUs in >> hardware context >> * @num_cus: Number of CUs to configure. >> - * @pad: Structure padding. >> + * @pad: MBZ. >> * @cu_configs: Array of CU configurations of struct >> amdxdna_cu_config. >> */ >> struct amdxdna_hwctx_param_config_cu { >> @@ -122,7 +122,6 @@ enum amdxdna_drm_config_hwctx_param { >> DRM_AMDXDNA_HWCTX_CONFIG_CU, >> DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF, >> DRM_AMDXDNA_HWCTX_REMOVE_DBG_BUF, >> - DRM_AMDXDNA_HWCTX_CONFIG_NUM > > This change actually meant here? It seems like it should be it's own > change. Agree. Hopefully, the patch 1 - 4 are good to go and I do not need to resend them. :) Thanks, Lizhi > >> }; >> /** >> @@ -133,7 +132,7 @@ enum amdxdna_drm_config_hwctx_param { >> * @param_val: A structure specified by the param_type struct member. >> * @param_val_size: Size of the parameter buffer pointed to by the >> param_val. >> * If param_val is not a pointer, driver can ignore this. >> - * @pad: Structure padding. >> + * @pad: MBZ. >> * >> * Note: if the param_val is a pointer pointing to a buffer, the >> maximum size >> * of the buffer is 4KiB(PAGE_SIZE). >> @@ -175,7 +174,7 @@ struct amdxdna_drm_create_bo { >> * @ext: MBZ. >> * @ext_flags: MBZ. >> * @handle: DRM buffer object handle. >> - * @pad: Structure padding. >> + * @pad: MBZ. >> * @map_offset: Returned DRM fake offset for mmap(). >> * @vaddr: Returned user VA of buffer. 0 in case user needs mmap(). >> * @xdna_addr: Returned XDNA device virtual address. >
>> This change actually meant here? It seems like it should be it's own >> change. > > Agree. Hopefully, the patch 1 - 4 are good to go and I do not need to > resend them. :) > > Yeah those are fine at this point. They've gotten good feedback, no complaints from LKP robot. I think you can just send the fixed up patch 5 separately. I've just added 1-4 to drm-misc-next. Here are the hashes. b1dcfe620574b accel/amdxdna: Read firmware interface version from registers f4d7b8a6bc8c9 accel/amdxdna: Enhance power management settings a37d78470bcc8 accel/amdxdna: Replace idr api with xarray 273b5176ac178 accel/amdxdna: Add RyzenAI-npu6 support
diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c index cdeead75c6f5..9facf45818f9 100644 --- a/drivers/accel/amdxdna/aie2_ctx.c +++ b/drivers/accel/amdxdna/aie2_ctx.c @@ -690,6 +690,9 @@ static int aie2_hwctx_cu_config(struct amdxdna_hwctx *hwctx, void *buf, u32 size int ret; XDNA_DBG(xdna, "Config %d CU to %s", config->num_cus, hwctx->name); + if (XDNA_MBZ_DBG(xdna, config->pad, sizeof(config->pad))) + return -EINVAL; + if (hwctx->status != HWCTX_STAT_INIT) { XDNA_ERR(xdna, "Not support re-config CU"); return -EINVAL; diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c index b2ca78cfd0a7..9e2c9a44f76a 100644 --- a/drivers/accel/amdxdna/aie2_message.c +++ b/drivers/accel/amdxdna/aie2_message.c @@ -395,6 +395,9 @@ int aie2_config_cu(struct amdxdna_hwctx *hwctx) for (i = 0; i < hwctx->cus->num_cus; i++) { struct amdxdna_cu_config *cu = &hwctx->cus->cu_configs[i]; + if (XDNA_MBZ_DBG(xdna, cu->pad, sizeof(cu->pad))) + return -EINVAL; + gobj = drm_gem_object_lookup(hwctx->client->filp, cu->cu_bo); if (!gobj) { XDNA_ERR(xdna, "Lookup GEM object failed"); diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c index 324f35c43f6c..d11b1c83d9c3 100644 --- a/drivers/accel/amdxdna/amdxdna_ctx.c +++ b/drivers/accel/amdxdna/amdxdna_ctx.c @@ -243,6 +243,9 @@ int amdxdna_drm_destroy_hwctx_ioctl(struct drm_device *dev, void *data, struct d struct amdxdna_hwctx *hwctx; int ret = 0, idx; + if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad))) + return -EINVAL; + if (!drm_dev_enter(dev, &idx)) return -ENODEV; @@ -277,6 +280,9 @@ int amdxdna_drm_config_hwctx_ioctl(struct drm_device *dev, void *data, struct dr void *buf; u64 val; + if (XDNA_MBZ_DBG(xdna, &args->pad, sizeof(args->pad))) + return -EINVAL; + if (!xdna->dev_info->ops->hwctx_config) return -EOPNOTSUPP; diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c index 4dfeca306d98..606433d73236 100644 --- a/drivers/accel/amdxdna/amdxdna_gem.c +++ b/drivers/accel/amdxdna/amdxdna_gem.c @@ -552,7 +552,7 @@ int amdxdna_drm_get_bo_info_ioctl(struct drm_device *dev, void *data, struct drm struct drm_gem_object *gobj; int ret = 0; - if (args->ext || args->ext_flags) + if (args->ext || args->ext_flags || args->pad) return -EINVAL; gobj = drm_gem_object_lookup(filp, args->handle); diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h index e4edb52bc27b..a706ead39082 100644 --- a/include/uapi/drm/amdxdna_accel.h +++ b/include/uapi/drm/amdxdna_accel.h @@ -87,7 +87,7 @@ struct amdxdna_drm_create_hwctx { /** * struct amdxdna_drm_destroy_hwctx - Destroy hardware context. * @handle: Hardware context handle. - * @pad: Structure padding. + * @pad: MBZ. */ struct amdxdna_drm_destroy_hwctx { __u32 handle; @@ -98,7 +98,7 @@ struct amdxdna_drm_destroy_hwctx { * struct amdxdna_cu_config - configuration for one CU * @cu_bo: CU configuration buffer bo handle. * @cu_func: Function of a CU. - * @pad: Structure padding. + * @pad: MBZ. */ struct amdxdna_cu_config { __u32 cu_bo; @@ -109,7 +109,7 @@ struct amdxdna_cu_config { /** * struct amdxdna_hwctx_param_config_cu - configuration for CUs in hardware context * @num_cus: Number of CUs to configure. - * @pad: Structure padding. + * @pad: MBZ. * @cu_configs: Array of CU configurations of struct amdxdna_cu_config. */ struct amdxdna_hwctx_param_config_cu { @@ -122,7 +122,6 @@ enum amdxdna_drm_config_hwctx_param { DRM_AMDXDNA_HWCTX_CONFIG_CU, DRM_AMDXDNA_HWCTX_ASSIGN_DBG_BUF, DRM_AMDXDNA_HWCTX_REMOVE_DBG_BUF, - DRM_AMDXDNA_HWCTX_CONFIG_NUM }; /** @@ -133,7 +132,7 @@ enum amdxdna_drm_config_hwctx_param { * @param_val: A structure specified by the param_type struct member. * @param_val_size: Size of the parameter buffer pointed to by the param_val. * If param_val is not a pointer, driver can ignore this. - * @pad: Structure padding. + * @pad: MBZ. * * Note: if the param_val is a pointer pointing to a buffer, the maximum size * of the buffer is 4KiB(PAGE_SIZE). @@ -175,7 +174,7 @@ struct amdxdna_drm_create_bo { * @ext: MBZ. * @ext_flags: MBZ. * @handle: DRM buffer object handle. - * @pad: Structure padding. + * @pad: MBZ. * @map_offset: Returned DRM fake offset for mmap(). * @vaddr: Returned user VA of buffer. 0 in case user needs mmap(). * @xdna_addr: Returned XDNA device virtual address.
For input structures, it is better to check if the pad is zero. Thus, the pad bytes might be usable in the future. Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> --- drivers/accel/amdxdna/aie2_ctx.c | 3 +++ drivers/accel/amdxdna/aie2_message.c | 3 +++ drivers/accel/amdxdna/amdxdna_ctx.c | 6 ++++++ drivers/accel/amdxdna/amdxdna_gem.c | 2 +- include/uapi/drm/amdxdna_accel.h | 11 +++++------ 5 files changed, 18 insertions(+), 7 deletions(-)