Skip to content

Commit

Permalink
drm/msm: UAPI error reporting
Browse files Browse the repository at this point in the history 
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@chromium.org>
Reviewed-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
Patchwork: https://patchwork.freedesktop.org/patch/630578/
  • Loading branch information
@robclark
robclark committed Jan 3, 2025
1 parent 19dbdbf commit 866e43b
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 49 deletions.
21 changes: 11 additions & 10 deletions drivers/gpu/drm/msm/adreno/adreno_gpu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -389,26 +390,27 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
*value = adreno_gpu->uche_trap_base;
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:
/* kstrdup_quotable_cmdline() limits to PAGE_SIZE, so
* 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) {
Expand Down Expand Up @@ -437,11 +439,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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/msm/msm_drv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions drivers/gpu/drm/msm/msm_drv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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__)

Expand Down
64 changes: 27 additions & 37 deletions drivers/gpu/drm/msm/msm_gem_submit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)
Expand All @@ -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..
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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: %x", syncobj_desc.flags);
break;
}

Expand All @@ -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: %u", i);
break;
}
}
Expand Down Expand Up @@ -588,14 +579,14 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev,
post_deps[i].point = syncobj_desc.point;

if (syncobj_desc.flags) {
ret = -EINVAL;
ret = UERR(EINVAL, dev, "invalid syncobj flags");
break;
}

if (syncobj_desc.point) {
if (!drm_core_check_feature(dev,
DRIVER_SYNCOBJ_TIMELINE)) {
ret = -EOPNOTSUPP;
ret = UERR(EOPNOTSUPP, dev, "syncobj timeline unsupported");
break;
}

Expand All @@ -609,7 +600,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev,
post_deps[i].syncobj =
drm_syncobj_find(file, syncobj_desc.handle);
if (!post_deps[i].syncobj) {
ret = -EINVAL;
ret = UERR(EINVAL, dev, "invalid syncobj handle");
break;
}
}
Expand Down Expand Up @@ -677,10 +668,10 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
* be more clever to dispatch to appropriate gpu module:
*/
if (MSM_PIPE_ID(args->flags) != MSM_PIPE_3D0)
return -EINVAL;
return UERR(EINVAL, dev, "invalid pipe");

if (MSM_PIPE_FLAGS(args->flags) & ~MSM_SUBMIT_FLAGS)
return -EINVAL;
return UERR(EINVAL, dev, "invalid flags");

if (args->flags & MSM_SUBMIT_SUDO) {
if (!IS_ENABLED(CONFIG_DRM_MSM_GPU_SUDO) ||
Expand Down Expand Up @@ -724,7 +715,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
in_fence = sync_file_get_fence(args->fence_fd);

if (!in_fence) {
ret = -EINVAL;
ret = UERR(EINVAL, dev, "invalid in-fence");
goto out_unlock;
}

Expand Down Expand Up @@ -788,8 +779,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,

if (!submit->cmd[i].size ||
(size_add(submit->cmd[i].size, submit->cmd[i].offset) > obj->size / 4)) {
SUBMIT_ERROR(submit, "invalid cmdstream size: %u\n", submit->cmd[i].size * 4);
ret = -EINVAL;
ret = UERR(EINVAL, dev, "invalid cmdstream size: %u\n",
submit->cmd[i].size * 4);
goto out;
}

Expand All @@ -799,8 +790,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
continue;

if (!gpu->allow_relocs) {
SUBMIT_ERROR(submit, "relocs not allowed\n");
ret = -EINVAL;
ret = UERR(EINVAL, dev, "relocs not allowed\n");
goto out;
}

Expand All @@ -826,7 +816,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
(!args->fence || idr_find(&queue->fence_idr, args->fence))) {
spin_unlock(&queue->idr_lock);
idr_preload_end();
ret = -EINVAL;
ret = UERR(EINVAL, dev, "invalid in-fence-sn");
goto out;
}

Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/msm/msm_submitqueue.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ int msm_file_private_set_sysprof(struct msm_file_private *ctx,

switch (sysprof) {
default:
return -EINVAL;
return UERR(EINVAL, gpu->dev, "Invalid sysprof: %d", sysprof);
case 2:
pm_runtime_get_sync(&gpu->pdev->dev);
fallthrough;
Expand Down

0 comments on commit 866e43b

Please sign in to comment.