On 12/30/2024 9:30 PM, Rob Clark wrote:
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
>
> Debugging incorrect UAPI usage tends to be a bit painful, so add a
> helper macro to make it easier to add debug logging which can be enabled
> at runtime via drm.debug.
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 21 ++++----
> drivers/gpu/drm/msm/msm_drv.c | 2 +-
> drivers/gpu/drm/msm/msm_drv.h | 7 +++
> drivers/gpu/drm/msm/msm_gem_submit.c | 64 +++++++++++--------------
> drivers/gpu/drm/msm/msm_submitqueue.c | 2 +-
> 5 files changed, 47 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 076be0473eb5..9649c0bd0a38 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -310,10 +310,11 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> uint32_t param, uint64_t *value, uint32_t *len)
> {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> + struct drm_device *drm = gpu->dev;
>
> /* No pointer params yet */
> if (*len != 0)
> - return -EINVAL;
> + return UERR(EINVAL, drm, "invalid len");
>
> switch (param) {
> case MSM_PARAM_GPU_ID:
> @@ -365,12 +366,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> return 0;
> case MSM_PARAM_VA_START:
> if (ctx->aspace == gpu->aspace)
> - return -EINVAL;
> + return UERR(EINVAL, drm, "requires per-process pgtables");
> *value = ctx->aspace->va_start;
> return 0;
> case MSM_PARAM_VA_SIZE:
> if (ctx->aspace == gpu->aspace)
> - return -EINVAL;
> + return UERR(EINVAL, drm, "requires per-process pgtables");
> *value = ctx->aspace->va_size;
> return 0;
> case MSM_PARAM_HIGHEST_BANK_BIT:
> @@ -386,14 +387,15 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> *value = adreno_gpu->ubwc_config.macrotile_mode;
> return 0;
> default:
> - DBG("%s: invalid param: %u", gpu->name, param);
> - return -EINVAL;
> + return UERR(EINVAL, drm, "%s: invalid param: %u", gpu->name, param);
> }
> }
>
> int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> uint32_t param, uint64_t value, uint32_t len)
> {
> + struct drm_device *drm = gpu->dev;
> +
> switch (param) {
> case MSM_PARAM_COMM:
> case MSM_PARAM_CMDLINE:
> @@ -401,11 +403,11 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> * that should be a reasonable upper bound
> */
> if (len > PAGE_SIZE)
> - return -EINVAL;
> + return UERR(EINVAL, drm, "invalid len");
> break;
> default:
> if (len != 0)
> - return -EINVAL;
> + return UERR(EINVAL, drm, "invalid len");
> }
>
> switch (param) {
> @@ -434,11 +436,10 @@ int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
> }
> case MSM_PARAM_SYSPROF:
> if (!capable(CAP_SYS_ADMIN))
> - return -EPERM;
> + return UERR(EPERM, drm, "invalid permissions");
> return msm_file_private_set_sysprof(ctx, gpu, value);
> default:
> - DBG("%s: invalid param: %u", gpu->name, param);
> - return -EINVAL;
> + return UERR(EINVAL, drm, "%s: invalid param: %u", gpu->name, param);
> }
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c2dd8ef6d6dc..2aefb8becda0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -538,7 +538,7 @@ static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
>
> /* Only supported if per-process address space is supported: */
> if (priv->gpu->aspace == ctx->aspace)
> - return -EOPNOTSUPP;
> + return UERR(EOPNOTSUPP, dev, "requires per-process pgtables");
>
> if (should_fail(&fail_gem_iova, obj->size))
> return -ENOMEM;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index d8c9a1b19263..fee31680a6d5 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -28,6 +28,7 @@
>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_print.h>
> #include <drm/drm_probe_helper.h>
> #include <drm/display/drm_dsc.h>
> #include <drm/msm_drm.h>
> @@ -506,6 +507,12 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
> clockid_t clock_id,
> enum hrtimer_mode mode);
>
> +/* Helper for returning a UABI error with optional logging which can make
> + * it easier for userspace to understand what it is doing wrong.
> + */
> +#define UERR(err, drm, fmt, ...) \
> + ({ DRM_DEV_DEBUG_DRIVER((drm)->dev, fmt, ##__VA_ARGS__); -(err); })
> +
> #define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
> #define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index fba78193127d..be6e793f34bd 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -20,8 +20,8 @@
> /* For userspace errors, use DRM_UT_DRIVER.. so that userspace can enable
> * error msgs for debugging, but we don't spam dmesg by default
> */
> -#define SUBMIT_ERROR(submit, fmt, ...) \
> - DRM_DEV_DEBUG_DRIVER((submit)->dev->dev, fmt, ##__VA_ARGS__)
> +#define SUBMIT_ERROR(err, submit, fmt, ...) \
> + UERR(err, (submit)->dev, fmt, ##__VA_ARGS__)
>
> /*
> * Cmdstream submission:
> @@ -142,8 +142,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
>
> if ((submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) ||
> !(submit_bo.flags & MANDATORY_FLAGS)) {
> - SUBMIT_ERROR(submit, "invalid flags: %x\n", submit_bo.flags);
> - ret = -EINVAL;
> + ret = SUBMIT_ERROR(EINVAL, submit, "invalid flags: %x\n", submit_bo.flags);
> i = 0;
> goto out;
> }
> @@ -162,8 +161,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
> */
> obj = idr_find(&file->object_idr, submit->bos[i].handle);
> if (!obj) {
> - SUBMIT_ERROR(submit, "invalid handle %u at index %u\n", submit->bos[i].handle, i);
> - ret = -EINVAL;
> + ret = SUBMIT_ERROR(EINVAL, submit, "invalid handle %u at index %u\n", submit->bos[i].handle, i);
> goto out_unlock;
> }
>
> @@ -206,14 +204,12 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> break;
> default:
> - SUBMIT_ERROR(submit, "invalid type: %08x\n", submit_cmd.type);
> - return -EINVAL;
> + return SUBMIT_ERROR(EINVAL, submit, "invalid type: %08x\n", submit_cmd.type);
> }
>
> if (submit_cmd.size % 4) {
> - SUBMIT_ERROR(submit, "non-aligned cmdstream buffer size: %u\n",
> - submit_cmd.size);
> - ret = -EINVAL;
> + ret = SUBMIT_ERROR(EINVAL, submit, "non-aligned cmdstream buffer size: %u\n",
> + submit_cmd.size);
> goto out;
> }
>
> @@ -371,9 +367,8 @@ static int submit_bo(struct msm_gem_submit *submit, uint32_t idx,
> struct drm_gem_object **obj, uint64_t *iova)
> {
> if (idx >= submit->nr_bos) {
> - SUBMIT_ERROR(submit, "invalid buffer index: %u (out of %u)\n",
> - idx, submit->nr_bos);
> - return -EINVAL;
> + return SUBMIT_ERROR(EINVAL, submit, "invalid buffer index: %u (out of %u)\n",
> + idx, submit->nr_bos);
> }
>
> if (obj)
> @@ -392,10 +387,8 @@ static int submit_reloc(struct msm_gem_submit *submit, struct drm_gem_object *ob
> uint32_t *ptr;
> int ret = 0;
>
> - if (offset % 4) {
> - SUBMIT_ERROR(submit, "non-aligned cmdstream buffer: %u\n", offset);
> - return -EINVAL;
> - }
> + if (offset % 4)
> + return SUBMIT_ERROR(EINVAL, submit, "non-aligned cmdstream buffer: %u\n", offset);
>
> /* For now, just map the entire thing. Eventually we probably
> * to do it page-by-page, w/ kmap() if not vmap()d..
> @@ -414,9 +407,8 @@ static int submit_reloc(struct msm_gem_submit *submit, struct drm_gem_object *ob
> uint64_t iova;
>
> if (submit_reloc.submit_offset % 4) {
> - SUBMIT_ERROR(submit, "non-aligned reloc offset: %u\n",
> - submit_reloc.submit_offset);
> - ret = -EINVAL;
> + ret = SUBMIT_ERROR(EINVAL, submit, "non-aligned reloc offset: %u\n",
> + submit_reloc.submit_offset);
> goto out;
> }
>
> @@ -425,8 +417,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct drm_gem_object *ob
>
> if ((off >= (obj->size / 4)) ||
> (off < last_offset)) {
> - SUBMIT_ERROR(submit, "invalid offset %u at reloc %u\n", off, i);
> - ret = -EINVAL;
> + ret = SUBMIT_ERROR(EINVAL, submit, "invalid offset %u at reloc %u\n", off, i);
> goto out;
> }
>
> @@ -513,12 +504,12 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit,
>
> if (syncobj_desc.point &&
> !drm_core_check_feature(submit->dev, DRIVER_SYNCOBJ_TIMELINE)) {
> - ret = -EOPNOTSUPP;
> + ret = SUBMIT_ERROR(EOPNOTSUPP, submit, "syncobj timeline unsupported");
> break;
> }
>
> if (syncobj_desc.flags & ~MSM_SUBMIT_SYNCOBJ_FLAGS) {
> - ret = -EINVAL;
> + ret = -SUBMIT_ERROR(EINVAL, submit, "invalid syncobj flags");
> break;
> }
>
> @@ -531,7 +522,7 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit,
> syncobjs[i] =
> drm_syncobj_find(file, syncobj_desc.handle);
> if (!syncobjs[i]) {
> - ret = -EINVAL;
> + ret = SUBMIT_ERROR(EINVAL, submit, "invalid syncobj handle");
Just to be more useful, probably we can print the index or the handle
here. Anyway
Reviewed-by: Akhil P Oommen <quic_akhilpo@xxxxxxxxxxx>
-Akhil.
