Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

13 participants
3240 discussions

Re: [PATCH] Replace nested max() with single max3()

by Christian König

Am 15.03.25 um 05:40 schrieb feng.wei8(a)zte.com.cn: > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. Please add a "dma-buf: heaps: " prefix to the subject line, apart from that looks good to me. Regards, Christian. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> > --- > drivers/dma-buf/dma-heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 3cbe87d4a464..96cb9ab5731a 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -147,7 +147,7 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > in_size = 0; > if ((ucmd & kcmd & IOC_OUT) == 0) > out_size = 0; > - ksize = max(max(in_size, out_size), drv_size); > + ksize = max3(in_size, out_size, drv_size); > > /* If necessary, allocate buffer for ioctl argument */ > if (ksize > sizeof(stack_kdata)) {

9 months

Re: [PATCH v4] dma-buf: heap: Replace nested max() with single max3()

by Krzysztof Kozlowski

On 17/03/2025 03:37, feng.wei8(a)zte.com.cn wrote: > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> > --- > v3 -> v4 > fix the format of this patch. > drivers/dma-buf/dma-heap.c | 2 +- You sent five versions per day of the same patch. Look what was in v3: v2 -> v3 fix the format of this patch So you are doing the same over and over and sending it to us? Srsly, ZTE, slow down and be sure you follow the process BEFORE you send flood of patches like that and learn on the go. Best regards, Krzysztof

9 months

Re: [PATCH v6 03/10] optee: account for direction while converting parameters

by Jens Wiklander

Hi Sumit, On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Wed, Mar 05, 2025 at 02:04:09PM +0100, Jens Wiklander wrote: > > The OP-TEE backend driver has two internal function pointers to convert > > between the subsystem type struct tee_param and the OP-TEE type struct > > optee_msg_param. > > > > The conversion is done from one of the types to the other, which is then > > involved in some operation and finally converted back to the original > > type. When converting to prepare the parameters for the operation, all > > fields must be taken into account, but then converting back, it's enough > > to update only out-values and out-sizes. So, an update_out parameter is > > added to the conversion functions to tell if all or only some fields > > must be copied. > > > > This is needed in a later patch where it might get confusing when > > converting back in from_msg_param() callback since an allocated > > restricted SHM can be using the sec_world_id of the used restricted > > memory pool and that doesn't translate back well. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/call.c | 10 ++-- > > drivers/tee/optee/ffa_abi.c | 43 +++++++++++++---- > > drivers/tee/optee/optee_private.h | 42 +++++++++++------ > > drivers/tee/optee/rpc.c | 31 +++++++++---- > > drivers/tee/optee/smc_abi.c | 76 +++++++++++++++++++++++-------- > > 5 files changed, 144 insertions(+), 58 deletions(-) > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > index 16eb953e14bb..f1533b894726 100644 > > --- a/drivers/tee/optee/call.c > > +++ b/drivers/tee/optee/call.c > > @@ -400,7 +400,8 @@ int optee_open_session(struct tee_context *ctx, > > export_uuid(msg_arg->params[1].u.octets, &client_uuid); > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params + 2, > > - arg->num_params, param); > > + arg->num_params, param, > > + false /*!update_out*/); > > if (rc) > > goto out; > > > > @@ -427,7 +428,8 @@ int optee_open_session(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > - msg_arg->params + 2)) { > > + msg_arg->params + 2, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_COMMUNICATION; > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > /* Close session again to avoid leakage */ > > @@ -541,7 +543,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > msg_arg->cancel_id = arg->cancel_id; > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params, > > - param); > > + param, false /*!update_out*/); > > if (rc) > > goto out; > > > > @@ -551,7 +553,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > } > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > - msg_arg->params)) { > > + msg_arg->params, true /*update_out*/)) { > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > } > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c > > index 4ca1d5161b82..e4b08cd195f3 100644 > > --- a/drivers/tee/optee/ffa_abi.c > > +++ b/drivers/tee/optee/ffa_abi.c > > @@ -122,15 +122,21 @@ static int optee_shm_rem_ffa_handle(struct optee *optee, u64 global_id) > > */ > > > > static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > - u32 attr, const struct optee_msg_param *mp) > > + u32 attr, const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm = NULL; > > u64 offs_high = 0; > > u64 offs_low = 0; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_FMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_FMEM_INPUT; > > - p->u.memref.size = mp->u.fmem.size; > > > > if (mp->u.fmem.global_id != OPTEE_MSG_FMEM_INVALID_GLOBAL_ID) > > shm = optee_shm_from_ffa_handle(optee, mp->u.fmem.global_id); > > @@ -141,6 +147,8 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > offs_high = mp->u.fmem.offs_high; > > } > > p->u.memref.shm_offs = offs_low | offs_high << 32; > > +out: > > + p->u.memref.size = mp->u.fmem.size; > > } > > > > /** > > @@ -150,12 +158,14 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > * @params: subsystem internal parameter representation > > * @num_params: number of elements in the parameter arrays > > * @msg_params: OPTEE_MSG parameters > > + * @update_out: update parameter for output only > > * > > * Returns 0 on success or <0 on failure > > */ > > static int optee_ffa_from_msg_param(struct optee *optee, > > struct tee_param *params, size_t num_params, > > - const struct optee_msg_param *msg_params) > > + const struct optee_msg_param *msg_params, > > + bool update_out) > > { > > size_t n; > > > > @@ -166,18 +176,20 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > switch (attr) { > > case OPTEE_MSG_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&p->u, 0, sizeof(p->u)); > > break; > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > - optee_from_msg_param_value(p, attr, mp); > > + optee_from_msg_param_value(p, attr, mp, update_out); > > break; > > case OPTEE_MSG_ATTR_TYPE_FMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_FMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_FMEM_INOUT: > > - from_msg_param_ffa_mem(optee, p, attr, mp); > > + from_msg_param_ffa_mem(optee, p, attr, mp, update_out); > > break; > > default: > > return -EINVAL; > > @@ -188,10 +200,16 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > } > > > > static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > struct tee_shm *shm = p->u.memref.shm; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_FMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > @@ -211,6 +229,7 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > memset(&mp->u, 0, sizeof(mp->u)); > > mp->u.fmem.global_id = OPTEE_MSG_FMEM_INVALID_GLOBAL_ID; > > } > > +out: > > mp->u.fmem.size = p->u.memref.size; > > > > return 0; > > @@ -222,13 +241,15 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > * @optee: main service struct > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > - * @params: subsystem itnernal parameter representation > > + * @params: subsystem internal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_ffa_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > size_t num_params, > > - const struct tee_param *params) > > + const struct tee_param *params, > > + bool update_out) > > { > > size_t n; > > > > @@ -238,18 +259,20 @@ static int optee_ffa_to_msg_param(struct optee *optee, > > > > switch (p->attr) { > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&mp->u, 0, sizeof(mp->u)); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > - optee_to_msg_param_value(mp, p); > > + optee_to_msg_param_value(mp, p, update_out); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > - if (to_msg_param_ffa_mem(mp, p)) > > + if (to_msg_param_ffa_mem(mp, p, update_out)) > > return -EINVAL; > > break; > > default: > > Can we rather handle it as follows to improve code readability and > maintainence long term? Ditto for all other places. > > static int optee_ffa_to_msg_param(struct optee *optee, > struct optee_msg_param *msg_params, > size_t num_params, > const struct tee_param *params, > bool update_out) > { > size_t n; > > for (n = 0; n < num_params; n++) { > const struct tee_param *p = params + n; > struct optee_msg_param *mp = msg_params + n; > > if (update_out && (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_NONE || > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT || > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT)) > continue; You're missing updating the length field for memrefs. Cheers, Jens > > switch (p->attr) { > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > memset(&mp->u, 0, sizeof(mp->u)); > break; > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > optee_to_msg_param_value(mp, p); > break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > if (to_msg_param_ffa_mem(mp, p)) > return -EINVAL; > break; > default: > return -EINVAL; > } > } > > return 0; > } > > -Sumit > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > > index dc0f355ef72a..20eda508dbac 100644 > > --- a/drivers/tee/optee/optee_private.h > > +++ b/drivers/tee/optee/optee_private.h > > @@ -185,10 +185,12 @@ struct optee_ops { > > bool system_thread); > > int (*to_msg_param)(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params); > > + size_t num_params, const struct tee_param *params, > > + bool update_out); > > int (*from_msg_param)(struct optee *optee, struct tee_param *params, > > size_t num_params, > > - const struct optee_msg_param *msg_params); > > + const struct optee_msg_param *msg_params, > > + bool update_out); > > }; > > > > /** > > @@ -316,23 +318,35 @@ void optee_release(struct tee_context *ctx); > > void optee_release_supp(struct tee_context *ctx); > > > > static inline void optee_from_msg_param_value(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > - p->u.value.a = mp->u.value.a; > > - p->u.value.b = mp->u.value.b; > > - p->u.value.c = mp->u.value.c; > > + if (!update_out) > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > + > > + if (attr == OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT || > > + attr == OPTEE_MSG_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + p->u.value.a = mp->u.value.a; > > + p->u.value.b = mp->u.value.b; > > + p->u.value.c = mp->u.value.c; > > + } > > } > > > > static inline void optee_to_msg_param_value(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, > > + bool update_out) > > { > > - mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > - TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > - mp->u.value.a = p->u.value.a; > > - mp->u.value.b = p->u.value.b; > > - mp->u.value.c = p->u.value.c; > > + if (!update_out) > > + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > + > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > + p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + mp->u.value.a = p->u.value.a; > > + mp->u.value.b = p->u.value.b; > > + mp->u.value.c = p->u.value.c; > > + } > > } > > > > void optee_cq_init(struct optee_call_queue *cq, int thread_count); > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c > > index ebbbd42b0e3e..580e6b9b0606 100644 > > --- a/drivers/tee/optee/rpc.c > > +++ b/drivers/tee/optee/rpc.c > > @@ -63,7 +63,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) > > + arg->params, false /*!update_out*/)) > > goto bad; > > > > for (i = 0; i < arg->num_params; i++) { > > @@ -107,7 +107,8 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } else { > > params[3].u.value.a = msg.len; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) > > + arg->num_params, params, > > + true /*update_out*/)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > else > > arg->ret = TEEC_SUCCESS; > > @@ -188,6 +189,7 @@ static void handle_rpc_func_cmd_wait(struct optee_msg_arg *arg) > > static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > struct optee_msg_arg *arg) > > { > > + bool update_out = false; > > struct tee_param *params; > > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > @@ -200,15 +202,21 @@ static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) { > > + arg->params, update_out)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > > > arg->ret = optee_supp_thrd_req(ctx, arg->cmd, arg->num_params, params); > > > > + /* > > + * Special treatment for OPTEE_RPC_CMD_SHM_ALLOC since input is a > > + * value type, but the output is a memref type. > > + */ > > + if (arg->cmd != OPTEE_RPC_CMD_SHM_ALLOC) > > + update_out = true; > > if (optee->ops->to_msg_param(optee, arg->params, arg->num_params, > > - params)) > > + params, update_out)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > out: > > kfree(params); > > @@ -270,7 +278,7 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > @@ -280,7 +288,8 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > params[0].u.value.b = 0; > > params[0].u.value.c = 0; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -324,7 +333,7 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -358,7 +367,8 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > params[0].u.value.b = rdev->descr.capacity; > > params[0].u.value.c = rdev->descr.reliable_wr_count; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -384,7 +394,7 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -401,7 +411,8 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > goto out; > > } > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index 165fadd9abc9..cfdae266548b 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -81,20 +81,26 @@ static int optee_cpuhp_disable_pcpu_irq(unsigned int cpu) > > */ > > > > static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > phys_addr_t pa; > > int rc; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_TMEM_INPUT) > > + return 0; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > - p->u.memref.size = mp->u.tmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; > > if (!shm) { > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(shm, 0, &pa); > > @@ -103,18 +109,25 @@ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > > > p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > > p->u.memref.shm = shm; > > - > > +out: > > + p->u.memref.size = mp->u.tmem.size; > > return 0; > > } > > > > static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_RMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > - p->u.memref.size = mp->u.rmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; > > > > if (shm) { > > @@ -124,6 +137,8 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > } > > +out: > > + p->u.memref.size = mp->u.rmem.size; > > } > > > > /** > > @@ -133,11 +148,13 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > * @params: subsystem internal parameter representation > > * @num_params: number of elements in the parameter arrays > > * @msg_params: OPTEE_MSG parameters > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > size_t num_params, > > - const struct optee_msg_param *msg_params) > > + const struct optee_msg_param *msg_params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -149,25 +166,27 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > > > switch (attr) { > > case OPTEE_MSG_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&p->u, 0, sizeof(p->u)); > > break; > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > - optee_from_msg_param_value(p, attr, mp); > > + optee_from_msg_param_value(p, attr, mp, update_out); > > break; > > case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: > > - rc = from_msg_param_tmp_mem(p, attr, mp); > > + rc = from_msg_param_tmp_mem(p, attr, mp, update_out); > > if (rc) > > return rc; > > break; > > case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: > > - from_msg_param_reg_mem(p, attr, mp); > > + from_msg_param_reg_mem(p, attr, mp, update_out); > > break; > > > > default: > > @@ -178,20 +197,25 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > } > > > > static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > int rc; > > phys_addr_t pa; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.tmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.tmem.size = p->u.memref.size; > > > > if (!p->u.memref.shm) { > > mp->u.tmem.buf_ptr = 0; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(p->u.memref.shm, p->u.memref.shm_offs, &pa); > > @@ -201,19 +225,27 @@ static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > mp->u.tmem.buf_ptr = pa; > > mp->attr |= OPTEE_MSG_ATTR_CACHE_PREDEFINED << > > OPTEE_MSG_ATTR_CACHE_SHIFT; > > - > > +out: > > + mp->u.tmem.size = p->u.memref.size; > > return 0; > > } > > > > static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.rmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.rmem.size = p->u.memref.size; > > mp->u.rmem.offs = p->u.memref.shm_offs; > > +out: > > + mp->u.rmem.size = p->u.memref.size; > > return 0; > > } > > > > @@ -223,11 +255,13 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > * @params: subsystem itnernal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params) > > + size_t num_params, const struct tee_param *params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -238,21 +272,23 @@ static int optee_to_msg_param(struct optee *optee, > > > > switch (p->attr) { > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&mp->u, 0, sizeof(mp->u)); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > - optee_to_msg_param_value(mp, p); > > + optee_to_msg_param_value(mp, p, update_out); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > if (tee_shm_is_dynamic(p->u.memref.shm)) > > - rc = to_msg_param_reg_mem(mp, p); > > + rc = to_msg_param_reg_mem(mp, p, update_out); > > else > > - rc = to_msg_param_tmp_mem(mp, p); > > + rc = to_msg_param_tmp_mem(mp, p, update_out); > > if (rc) > > return rc; > > break; > > -- > > 2.43.0 > >

9 months

Re: [PATCH] dma-buf: Replace nested max() with single max3()

by T.J. Mercier

On Fri, Mar 14, 2025 at 10:02 PM <feng.wei8(a)zte.com.cn> wrote: > > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> The commit subject should include "dma-buf: heaps:" as a prefix. With that: Reviewed-by: T.J. Mercier <tjmercier(a)google.com> > --- > drivers/dma-buf/dma-heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 3cbe87d4a464..96cb9ab5731a 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -147,7 +147,7 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > in_size = 0; > if ((ucmd & kcmd & IOC_OUT) == 0) > out_size = 0; > - ksize = max(max(in_size, out_size), drv_size); > + ksize = max3(in_size, out_size, drv_size); > > /* If necessary, allocate buffer for ioctl argument */ > if (ksize > sizeof(stack_kdata)) { > -- > 2.25.1

9 months

Re: [PATCH v5 RESEND 1/2] dmaengine: qcom: gpi: Add GPI Block event interrupt support

by Vinod Koul

On 12-02-25, 17:35, Jyothi Kumar Seerapu wrote: > GSI hardware generates an interrupt for each transfer completion. > For multiple messages within a single transfer, this results in > N interrupts for N messages, leading to significant software > interrupt latency. > > To mitigate this latency, utilize Block Event Interrupt (BEI) mechanism. > Enabling BEI instructs the GSI hardware to prevent interrupt generation > and BEI is disabled when an interrupt is necessary. > > When using BEI, consider splitting a single multi-message transfer into > chunks of 8 messages internally and so interrupts are not expected for > the first 7 message completions, only the last message triggers > an interrupt, indicating the completion of 8 messages. > > This BEI mechanism enhances overall transfer efficiency. That sounds good but I dont like the idea that we add a custom interface for this. Please use DMA_PREP_INTERRUPT instead. Adding this flag should trigger N interrupts, absence of this should lead to Block events only -- ~Vinod

9 months, 1 week

[PATCH v4] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Reviewed-by: T.J. Mercier <tjmercier(a)google.com> Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes from v3: - Add TJ RB - Fix a few typos. Changes from v2: - Remove exhaustive list of names for platforms, and just mention the alternatives. - Add MAINTAINERS entry Changes from v1: - Add the mention that the cma / reserved heap is optional. --- Documentation/userspace-api/dma-buf-heaps.rst | 25 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..535f49047ce6 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,25 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represents a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers. + + - The ``cma`` heap allocates physically contiguous, cacheable, + buffers. Only present if a CMA region is present. Such a region is + usually created either through the kernel commandline through the + `cma` parameter, a memory region Device-Tree node with the + `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or + `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it + might be called ``reserved``, ``linux,cma``, or ``default-pool``. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index b1395d94b3fd..9cbe4390c872 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -42,10 +42,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas diff --git a/MAINTAINERS b/MAINTAINERS index 8e0736dc2ee0..ef0364e65aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6911,10 +6911,11 @@ R: T.J. Mercier <tjmercier(a)google.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Maintained T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/userspace-api/dma-buf-heaps.rst F: drivers/dma-buf/dma-heap.c F: drivers/dma-buf/heaps/* F: include/linux/dma-heap.h F: include/uapi/linux/dma-heap.h F: tools/testing/selftests/dmabuf-heaps/ -- 2.48.1

9 months, 2 weeks

[PATCH v3] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes from v2: - Remove exhaustive list of names for platforms, and just mention the alternatives. - Add MAINTAINERS entry Changes from v1: - Add the mention that the cma / reserved heap is optional. --- Documentation/userspace-api/dma-buf-heaps.rst | 25 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..5b92d69646f6 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,25 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represent a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers + + - The ``cma`` heap allocates physically contiguous, cacheable, + buffers. Only present if a CMA region is present. Such a region is + usually created either through the kernel commandline through the + `cma` parameter, a memory region Device-Tree node with the + `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or + `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it + might be called ``reserved``, ``linux,cma``, or ``default-pool``. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index b1395d94b3fd..9cbe4390c872 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -42,10 +42,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas diff --git a/MAINTAINERS b/MAINTAINERS index 8e0736dc2ee0..ef0364e65aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6911,10 +6911,11 @@ R: T.J. Mercier <tjmercier(a)google.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Maintained T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/userspace-api/dma-buf-heaps.rst F: drivers/dma-buf/dma-heap.c F: drivers/dma-buf/heaps/* F: include/linux/dma-heap.h F: include/uapi/linux/dma-heap.h F: tools/testing/selftests/dmabuf-heaps/ -- 2.48.1

9 months, 2 weeks

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Sumit Garg

On Fri, 14 Feb 2025 at 21:19, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > > On Fri, 14 Feb 2025 18:37:14 +0530 > Sumit Garg <sumit.garg(a)linaro.org> wrote: > > > On Fri, 14 Feb 2025 at 15:37, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > > Hi, > > > > > > On Thu, Feb 13, 2025 at 6:39 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, 13 Feb 2025 at 15:57, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > On Thu, Feb 13, 2025 at 3:05 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > But just because TEE is one good backend implementation, doesn't mean > > > > > > it should be the userspace ABI. Why should userspace care that TEE has > > > > > > mediated the allocation instead of it being a predefined range within > > > > > > DT? > > > > > > > > > > The TEE may very well use a predefined range that part is abstracted > > > > > with the interface. > > > > > > > > Of course. But you can also (and this has been shipped on real > > > > devices) handle this without any per-allocation TEE needs by simply > > > > allocating from a memory range which is predefined within DT. > > > > > > > > From the userspace point of view, why should there be one ABI to > > > > allocate memory from a predefined range which is delivered by DT to > > > > the kernel, and one ABI to allocate memory from a predefined range > > > > which is mediated by TEE? > > > > > > We need some way to specify the protection profile (or use case as > > > I've called it in the ABI) required for the buffer. Whether it's > > > defined in DT seems irrelevant. > > > > > > > > > > > > > What advantage > > > > > > does userspace get from having to have a different codepath to get a > > > > > > different handle to memory? What about x86? > > > > > > > > > > > > I think this proposal is looking at it from the wrong direction. > > > > > > Instead of working upwards from the implementation to userspace, start > > > > > > with userspace and work downwards. The interesting property to focus > > > > > > on is allocating memory, not that EL1 is involved behind the scenes. > > > > > > > > > > From what I've gathered from earlier discussions, it wasn't much of a > > > > > problem for userspace to handle this. If the kernel were to provide it > > > > > via a different ABI, how would it be easier to implement in the > > > > > kernel? I think we need an example to understand your suggestion. > > > > > > > > It is a problem for userspace, because we need to expose acceptable > > > > parameters for allocation through the entire stack. If you look at the > > > > dmabuf documentation in the kernel for how buffers should be allocated > > > > and exchanged, you can see the negotiation flow for modifiers. This > > > > permeates through KMS, EGL, Vulkan, Wayland, GStreamer, and more. > > > > > > What dma-buf properties are you referring to? > > > dma_heap_ioctl_allocate() accepts a few flags for the resulting file > > > descriptor and no flags for the heap itself. > > > > > > > > > > > Standardising on heaps allows us to add those in a similar way. > > > > > > How would you solve this with heaps? Would you use one heap for each > > > protection profile (use case), add heap_flags, or do a bit of both? > > I would say one heap per-profile. > And then it would have a per vendor multiplication factor as each vendor enforces memory restriction in a platform specific manner which won't scale. > > > > Christian gave an historical background here [1] as to why that hasn't > > worked in the past with DMA heaps given the scalability issues. > > > > [1] https://lore.kernel.org/dri-devel/e967e382-6cca-4dee-8333-39892d532f71@gmai… > > Hm, I fail to see where Christian dismiss the dma-heaps solution in > this email. He even says: > > > If the memory is not physically attached to any device, but rather just > memory attached to the CPU or a system wide memory controller then > expose the memory as DMA-heap with specific requirements (e.g. certain > sized pages, contiguous, restricted, encrypted, ...). I am not saying Christian dismissed DMA heaps but rather how scalability is an issue. What we are proposing here is a generic interface via TEE to the firmware/Trusted OS which can perform all the platform specific memory restrictions. This solution will scale across vendors. > > > > > > > > > > If we > > > > have to add different allocation mechanisms, then the complexity > > > > increases, permeating not only into all the different userspace APIs, > > > > but also into the drivers which need to support every different > > > > allocation mechanism even if they have no opinion on it - e.g. Mali > > > > doesn't care in any way whether the allocation comes from a heap or > > > > TEE or ACPI or whatever, it cares only that the memory is protected. > > > > > > > > Does that help? > > > > > > I think you're missing the stage where an unprotected buffer is > > > received and decrypted into a protected buffer. If you use the TEE for > > > decryption or to configure the involved devices for the use case, it > > > makes sense to let the TEE allocate the buffers, too. A TEE doesn't > > > have to be an OS in the secure world, it can be an abstraction to > > > support the use case depending on the design. So the restricted buffer > > > is already allocated before we reach Mali in your example. > > > > > > Allocating restricted buffers from the TEE subsystem saves us from > > > maintaining proxy dma-buf heaps. > > Honestly, when I look at dma-heap implementations, they seem > to be trivial shells around existing (more complex) allocators, and the > boiler plate [1] to expose a dma-heap is relatively small. The dma-buf > implementation, you already have, so we're talking about a hundred > lines of code to maintain, which shouldn't be significantly more than > what you have for the new ioctl() to be honest. It will rather be redundant vendor specific code under DMA heaps calling into firmware/Trusted OS to enforce memory restrictions as you can look into Mediatek example [1]. With TEE subsystem managing that it won't be the case as we will provide a common abstraction for the communication with underlying firmware/Trusted OS. [1] https://lore.kernel.org/linux-arm-kernel/20240515112308.10171-1-yong.wu@med… > And I'll insist on what > Daniel said, it's a small price to pay to have a standard interface to > expose to userspace. If dma-heaps are not used for this kind things, I > honestly wonder what they will be used for... Let's try not to forcefully find a use-case for DMA heaps when there is a better alternative available. I am still failing to see why you don't consider following as a standardised user-space interface: "When user-space has to work with restricted memory, ask TEE device to allocate it" -Sumit

9 months, 2 weeks

Re: [PATCH v2 1/7] dt-bindings: npu: rockchip,rknn: Add bindings

by Rob Herring

On Tue, Feb 25, 2025 at 08:55:47AM +0100, Tomeu Vizoso wrote: > Add the bindings for the Neural Processing Unit IP from Rockchip. > > v2: > - Adapt to new node structure (one node per core, each with its own > IOMMU) > - Several misc. fixes from Sebastian Reichel > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > Signed-off-by: Sebastian Reichel <sebastian.reichel(a)collabora.com> > --- > .../bindings/npu/rockchip,rknn-core.yaml | 152 +++++++++++++++++++++ > 1 file changed, 152 insertions(+) > > diff --git a/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..e8d0afe4a7d1c4f166cf13a9f4aa7c1901362a3f > --- /dev/null > +++ b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml > @@ -0,0 +1,152 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Neural Processing Unit IP from Rockchip > + > +maintainers: > + - Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > + > +description: > + Rockchip IP for accelerating inference of neural networks, based on NVIDIA's > + open source NVDLA IP. > + > +properties: > + $nodename: > + pattern: '^npu-core@[a-f0-9]+$' > + > + compatible: > + oneOf: > + - items: > + - enum: > + - rockchip,rk3588-rknn-core-top > + - const: rockchip,rknn-core-top Drop the fallbacks unless you have some evidence that the IP is the same across a lot of SoCs. If you don't, then rockchip,rk3588-rknn-core-top can be the fallback whenever there are more compatible SoCs. Or if there's version/feature registers that otherwise make it discoverable, then a common compatible is fine. > + - items: > + - enum: > + - rockchip,rk3588-rknn-core > + - const: rockchip,rknn-core I don't understand the difference between core and core-top. That needs to be explained in the top-level description. > + > + reg: > + maxItems: 1 > + > + clocks: > + minItems: 2 > + maxItems: 4 > + > + clock-names: > + items: > + - const: aclk > + - const: hclk > + - const: npu > + - const: pclk > + minItems: 2 > + > + interrupts: > + maxItems: 1 > + > + iommus: > + maxItems: 1 > + > + npu-supply: true > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 2 > + > + reset-names: > + items: > + - const: srst_a > + - const: srst_h > + > + sram-supply: true Group supply properties together > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - iommus > + - npu-supply > + - power-domains > + - resets > + - reset-names > + - sram-supply > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - rockchip,rknn-core-top > + then: > + properties: > + clocks: > + minItems: 4 > + > + clock-names: > + minItems: 4 > + - if: > + properties: > + compatible: > + contains: > + enum: > + - rockchip,rknn-core > + then: > + properties: > + clocks: > + maxItems: 2 > + clock-names: > + maxItems: 2 > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rockchip,rk3588-cru.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/power/rk3588-power.h> > + #include <dt-bindings/reset/rockchip,rk3588-cru.h> > + > + bus { > + #address-cells = <2>; > + #size-cells = <2>; > + > + rknn_core_top: npu-core@fdab0000 { npu@... > + compatible = "rockchip,rk3588-rknn-core-top", "rockchip,rknn-core-top"; > + reg = <0x0 0xfdab0000 0x0 0x9000>; > + assigned-clocks = <&scmi_clk SCMI_CLK_NPU>; > + assigned-clock-rates = <200000000>; > + clocks = <&cru ACLK_NPU0>, <&cru HCLK_NPU0>, > + <&scmi_clk SCMI_CLK_NPU>, <&cru PCLK_NPU_ROOT>; > + clock-names = "aclk", "hclk", "npu", "pclk"; > + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH 0>; > + iommus = <&rknn_mmu_top>; > + npu-supply = <&vdd_npu_s0>; > + power-domains = <&power RK3588_PD_NPUTOP>; > + resets = <&cru SRST_A_RKNN0>, <&cru SRST_H_RKNN0>; > + reset-names = "srst_a", "srst_h"; > + sram-supply = <&vdd_npu_mem_s0>; > + }; > + > + rknn_core_1: npu-core@fdac0000 { > + compatible = "rockchip,rk3588-rknn-core", "rockchip,rknn-core"; > + reg = <0x0 0xfdac0000 0x0 0x9000>; > + clocks = <&cru ACLK_NPU1>, <&cru HCLK_NPU1>; > + clock-names = "aclk", "hclk"; > + interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH 0>; > + iommus = <&rknn_mmu_1>; > + npu-supply = <&vdd_npu_s0>; > + power-domains = <&power RK3588_PD_NPU1>; > + resets = <&cru SRST_A_RKNN1>, <&cru SRST_H_RKNN1>; > + reset-names = "srst_a", "srst_h"; > + sram-supply = <&vdd_npu_mem_s0>; > + }; > + }; > +... > > -- > 2.48.1 >

9 months, 3 weeks

Re: [PATCH v2 6/7] accel/rocket: Add job submission IOCTL

by Thomas Zimmermann

Hi Am 25.02.25 um 08:55 schrieb Tomeu Vizoso: > Using the DRM GPU scheduler infrastructure, with a scheduler for each > core. > > Userspace can decide for a series of tasks to be executed sequentially > in the same core, so SRAM locality can be taken advantage of. > > The job submission code was initially based on Panfrost. > > v2: > - Remove hardcoded number of cores > - Misc. style fixes (Jeffrey Hugo) > - Repack IOCTL struct (Jeffrey Hugo) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > --- > drivers/accel/rocket/Makefile | 3 +- > drivers/accel/rocket/rocket_core.c | 6 + > drivers/accel/rocket/rocket_core.h | 14 + > drivers/accel/rocket/rocket_device.c | 2 + > drivers/accel/rocket/rocket_device.h | 2 + > drivers/accel/rocket/rocket_drv.c | 15 + > drivers/accel/rocket/rocket_drv.h | 4 + > drivers/accel/rocket/rocket_job.c | 710 +++++++++++++++++++++++++++++++++++ > drivers/accel/rocket/rocket_job.h | 50 +++ > include/uapi/drm/rocket_accel.h | 55 +++ > 10 files changed, 860 insertions(+), 1 deletion(-) > > diff --git a/drivers/accel/rocket/Makefile b/drivers/accel/rocket/Makefile > index 875cac2243d902694e0d5d05e60b4ae551a633c4..4d59036af8d9c213d3cac0559eb66e3ebb0320e7 100644 > --- a/drivers/accel/rocket/Makefile > +++ b/drivers/accel/rocket/Makefile > @@ -6,4 +6,5 @@ rocket-y := \ > rocket_core.o \ > rocket_device.o \ > rocket_drv.o \ > - rocket_gem.o > + rocket_gem.o \ > + rocket_job.o > diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c > index 09d966c826b5b1090a18cb24b3aa4aba286a12d4..2b522592693874eed90463e8f85653d5282ae5b8 100644 > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -6,6 +6,7 @@ > #include <linux/pm_runtime.h> > > #include "rocket_core.h" > +#include "rocket_job.h" > #include "rocket_registers.h" > > static int rocket_clk_init(struct rocket_core *core) > @@ -48,6 +49,10 @@ int rocket_core_init(struct rocket_core *core) > if (IS_ERR(core->iomem)) > return PTR_ERR(core->iomem); > > + err = rocket_job_init(core); > + if (err) > + return err; > + > pm_runtime_use_autosuspend(dev); > pm_runtime_set_autosuspend_delay(dev, 50); /* ~3 frames */ > pm_runtime_enable(dev); > @@ -68,4 +73,5 @@ int rocket_core_init(struct rocket_core *core) > void rocket_core_fini(struct rocket_core *core) > { > pm_runtime_disable(core->dev); > + rocket_job_fini(core); > } > diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h > index 2171eba7139ccc63fe24802dc81b4adb7f3abf31..045a46a2010a2ffd6122ed86c379e5fabc70365a 100644 > --- a/drivers/accel/rocket/rocket_core.h > +++ b/drivers/accel/rocket/rocket_core.h > @@ -21,6 +21,20 @@ struct rocket_core { > void __iomem *iomem; > struct clk *a_clk; > struct clk *h_clk; > + > + struct rocket_job *in_flight_job; > + > + spinlock_t job_lock; > + > + struct { > + struct workqueue_struct *wq; > + struct work_struct work; > + atomic_t pending; > + } reset; > + > + struct drm_gpu_scheduler sched; > + u64 fence_context; > + u64 emit_seqno; > }; > > int rocket_core_init(struct rocket_core *core); > diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c > index 9af36357caba7148dcac764c8222699f3b572d60..62c640e1e0200fe25b6834e45d71f6de139ff3ab 100644 > --- a/drivers/accel/rocket/rocket_device.c > +++ b/drivers/accel/rocket/rocket_device.c > @@ -12,6 +12,7 @@ int rocket_device_init(struct rocket_device *rdev) > int err; > > mutex_init(&rdev->iommu_lock); > + mutex_init(&rdev->sched_lock); > > rdev->clk_npu = devm_clk_get(dev, "npu"); > rdev->pclk = devm_clk_get(dev, "pclk"); > @@ -29,5 +30,6 @@ int rocket_device_init(struct rocket_device *rdev) > void rocket_device_fini(struct rocket_device *rdev) > { > rocket_core_fini(&rdev->cores[0]); > + mutex_destroy(&rdev->sched_lock); > mutex_destroy(&rdev->iommu_lock); > } > diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h > index c6152569fdd9e5587c8e8d7b0d7c2e2a77af6000..4168ae8da2d38c2ea114b37c6e053b02611a0232 100644 > --- a/drivers/accel/rocket/rocket_device.h > +++ b/drivers/accel/rocket/rocket_device.h > @@ -11,6 +11,8 @@ > struct rocket_device { > struct drm_device ddev; > > + struct mutex sched_lock; > + > struct clk *clk_npu; > struct clk *pclk; > > diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c > index e5612b52952fa7a0cd0af02aef314984bc483b05..a6b486e2d4f648d7b1d8831590b633bf661c7bc4 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -16,12 +16,14 @@ > > #include "rocket_drv.h" > #include "rocket_gem.h" > +#include "rocket_job.h" > > static int > rocket_open(struct drm_device *dev, struct drm_file *file) > { > struct rocket_device *rdev = to_rocket_device(dev); > struct rocket_file_priv *rocket_priv; > + int ret; > > rocket_priv = kzalloc(sizeof(*rocket_priv), GFP_KERNEL); > if (!rocket_priv) > @@ -30,7 +32,15 @@ rocket_open(struct drm_device *dev, struct drm_file *file) > rocket_priv->rdev = rdev; > file->driver_priv = rocket_priv; > > + ret = rocket_job_open(rocket_priv); > + if (ret) > + goto err_free; > + > return 0; > + > +err_free: > + kfree(rocket_priv); > + return ret; > } > > static void > @@ -38,6 +48,7 @@ rocket_postclose(struct drm_device *dev, struct drm_file *file) > { > struct rocket_file_priv *rocket_priv = file->driver_priv; > > + rocket_job_close(rocket_priv); > kfree(rocket_priv); > } > > @@ -46,6 +57,7 @@ static const struct drm_ioctl_desc rocket_drm_driver_ioctls[] = { > DRM_IOCTL_DEF_DRV(ROCKET_##n, rocket_ioctl_##func, 0) > > ROCKET_IOCTL(CREATE_BO, create_bo), > + ROCKET_IOCTL(SUBMIT, submit), > }; > > DEFINE_DRM_ACCEL_FOPS(rocket_accel_driver_fops); > @@ -245,6 +257,9 @@ static int rocket_device_runtime_suspend(struct device *dev) > if (dev != rdev->cores[core].dev) > continue; > > + if (!rocket_job_is_idle(&rdev->cores[core])) > + return -EBUSY; > + > clk_disable_unprepare(rdev->cores[core].a_clk); > clk_disable_unprepare(rdev->cores[core].h_clk); > > diff --git a/drivers/accel/rocket/rocket_drv.h b/drivers/accel/rocket/rocket_drv.h > index ccdd50c69d4c033eea18cb800407fdcfb3bf2e9b..54e21a61006057aee293496016e54b495a2f6d55 100644 > --- a/drivers/accel/rocket/rocket_drv.h > +++ b/drivers/accel/rocket/rocket_drv.h > @@ -4,10 +4,14 @@ > #ifndef __ROCKET_DRV_H__ > #define __ROCKET_DRV_H__ > > +#include <drm/gpu_scheduler.h> > + > #include "rocket_device.h" > > struct rocket_file_priv { > struct rocket_device *rdev; > + > + struct drm_sched_entity sched_entity; > }; > > #endif > diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c > new file mode 100644 > index 0000000000000000000000000000000000000000..25b31f28e932aaee86173b9a0962932c9c640c03 > --- /dev/null > +++ b/drivers/accel/rocket/rocket_job.c > @@ -0,0 +1,710 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright 2019 Linaro, Ltd, Rob Herring <robh(a)kernel.org> */ > +/* Copyright 2019 Collabora ltd. */ > +/* Copyright 2024 Tomeu Vizoso <tomeu(a)tomeuvizoso.net> */ > + > +#include <drm/drm_file.h> > +#include <drm/drm_gem.h> > +#include <drm/rocket_accel.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > + > +#include "rocket_core.h" > +#include "rocket_device.h" > +#include "rocket_drv.h" > +#include "rocket_job.h" > +#include "rocket_registers.h" > + > +#define JOB_TIMEOUT_MS 500 > + > +#define job_write(dev, reg, data) writel(data, dev->iomem + (reg)) > +#define job_read(dev, reg) readl(dev->iomem + (reg)) > + > +static struct rocket_job * > +to_rocket_job(struct drm_sched_job *sched_job) > +{ > + return container_of(sched_job, struct rocket_job, base); > +} > + > +struct rocket_fence { > + struct dma_fence base; > + struct drm_device *dev; > + /* rocket seqno for signaled() test */ > + u64 seqno; > + int queue; > +}; > + > +static inline struct rocket_fence * > +to_rocket_fence(struct dma_fence *fence) > +{ > + return (struct rocket_fence *)fence; Proper upcast with container_of() please. > +} > + > +static const char *rocket_fence_get_driver_name(struct dma_fence *fence) > +{ > + return "rocket"; > +} > + > +static const char *rocket_fence_get_timeline_name(struct dma_fence *fence) > +{ > + return "rockchip-npu"; > +} > + > +static const struct dma_fence_ops rocket_fence_ops = { > + .get_driver_name = rocket_fence_get_driver_name, > + .get_timeline_name = rocket_fence_get_timeline_name, > +}; > + > +static struct dma_fence *rocket_fence_create(struct rocket_core *core) > +{ > + struct rocket_device *rdev = core->rdev; > + struct rocket_fence *fence; > + > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > + if (!fence) > + return ERR_PTR(-ENOMEM); > + > + fence->dev = &rdev->ddev; > + fence->seqno = ++core->emit_seqno; > + dma_fence_init(&fence->base, &rocket_fence_ops, &core->job_lock, > + core->fence_context, fence->seqno); > + > + return &fence->base; > +} > + > +static int > +rocket_copy_tasks(struct drm_device *dev, > + struct drm_file *file_priv, > + struct drm_rocket_job *job, > + struct rocket_job *rjob) > +{ > + struct drm_rocket_task *tasks; > + int ret = 0; > + int i; > + > + rjob->task_count = job->task_count; > + > + if (!rjob->task_count) > + return 0; > + > + tasks = kvmalloc_array(rjob->task_count, sizeof(*tasks), GFP_KERNEL); > + if (!tasks) { > + ret = -ENOMEM; > + DRM_DEBUG("Failed to allocate incoming tasks\n"); These logging macros are deprecated. Please use drm_dbg() and similar functions instead. Here and everywhere else. > + goto fail; > + } > + > + if (copy_from_user(tasks, > + (void __user *)(uintptr_t)job->tasks, > + rjob->task_count * sizeof(*tasks))) { > + ret = -EFAULT; > + DRM_DEBUG("Failed to copy incoming tasks\n"); > + goto fail; > + } > + > + rjob->tasks = kvmalloc_array(job->task_count, sizeof(*rjob->tasks), GFP_KERNEL); > + if (!rjob->tasks) { > + DRM_DEBUG("Failed to allocate task array\n"); > + ret = -ENOMEM; > + goto fail; > + } > + > + for (i = 0; i < rjob->task_count; i++) { > + if (tasks[i].regcmd_count == 0) { > + ret = -EINVAL; > + goto fail; > + } > + rjob->tasks[i].regcmd = tasks[i].regcmd; > + rjob->tasks[i].regcmd_count = tasks[i].regcmd_count; > + } > + > +fail: > + kvfree(tasks); > + return ret; > +} > + > +static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *job) > +{ > + struct rocket_task *task; > + bool task_pp_en = 1; > + bool task_count = 1; > + > + /* GO ! */ > + > + /* Don't queue the job if a reset is in progress */ > + if (!atomic_read(&core->reset.pending)) { > + > + task = &job->tasks[job->next_task_idx]; > + job->next_task_idx++; /* TODO: Do this only after a successful run? */ > + > + rocket_write(core, REG_PC_BASE_ADDRESS, 0x1); Just 'write' seems a little imprecise. Maybe 'writel' would be better. Best regards Thomas > + > + rocket_write(core, REG_CNA_S_POINTER, 0xe + 0x10000000 * core->index); > + rocket_write(core, REG_CORE_S_POINTER, 0xe + 0x10000000 * core->index); > + > + rocket_write(core, REG_PC_BASE_ADDRESS, task->regcmd); > + rocket_write(core, REG_PC_REGISTER_AMOUNTS, (task->regcmd_count + 1) / 2 - 1); > + > + rocket_write(core, REG_PC_INTERRUPT_MASK, > + PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1); > + rocket_write(core, REG_PC_INTERRUPT_CLEAR, > + PC_INTERRUPT_CLEAR_DPU_0 | PC_INTERRUPT_CLEAR_DPU_1); > + > + rocket_write(core, REG_PC_TASK_CON, ((0x6 | task_pp_en) << 12) | task_count); > + > + rocket_write(core, REG_PC_TASK_DMA_BASE_ADDR, 0x0); > + > + rocket_write(core, REG_PC_OPERATION_ENABLE, 0x1); > + > + dev_dbg(core->dev, > + "Submitted regcmd at 0x%llx to core %d", > + task->regcmd, core->index); > + } > +} > + > +static int rocket_acquire_object_fences(struct drm_gem_object **bos, > + int bo_count, > + struct drm_sched_job *job, > + bool is_write) > +{ > + int i, ret; > + > + for (i = 0; i < bo_count; i++) { > + ret = dma_resv_reserve_fences(bos[i]->resv, 1); > + if (ret) > + return ret; > + > + ret = drm_sched_job_add_implicit_dependencies(job, bos[i], > + is_write); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static void rocket_attach_object_fences(struct drm_gem_object **bos, > + int bo_count, > + struct dma_fence *fence) > +{ > + int i; > + > + for (i = 0; i < bo_count; i++) > + dma_resv_add_fence(bos[i]->resv, fence, DMA_RESV_USAGE_WRITE); > +} > + > +static int rocket_job_push(struct rocket_job *job) > +{ > + struct rocket_device *rdev = job->rdev; > + struct drm_gem_object **bos; > + struct ww_acquire_ctx acquire_ctx; > + int ret = 0; > + > + bos = kvmalloc_array(job->in_bo_count + job->out_bo_count, sizeof(void *), > + GFP_KERNEL); > + memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *)); > + memcpy(&bos[job->in_bo_count], job->out_bos, job->out_bo_count * sizeof(void *)); > + > + ret = drm_gem_lock_reservations(bos, job->in_bo_count + job->out_bo_count, &acquire_ctx); > + if (ret) > + goto err; > + > + mutex_lock(&rdev->sched_lock); > + drm_sched_job_arm(&job->base); > + > + job->inference_done_fence = dma_fence_get(&job->base.s_fence->finished); > + > + ret = rocket_acquire_object_fences(job->in_bos, job->in_bo_count, &job->base, false); > + if (ret) { > + mutex_unlock(&rdev->sched_lock); > + goto err_unlock; > + } > + > + ret = rocket_acquire_object_fences(job->out_bos, job->out_bo_count, &job->base, true); > + if (ret) { > + mutex_unlock(&rdev->sched_lock); > + goto err_unlock; > + } > + > + kref_get(&job->refcount); /* put by scheduler job completion */ > + > + drm_sched_entity_push_job(&job->base); > + > + mutex_unlock(&rdev->sched_lock); > + > + rocket_attach_object_fences(job->out_bos, job->out_bo_count, job->inference_done_fence); > + > +err_unlock: > + drm_gem_unlock_reservations(bos, job->in_bo_count + job->out_bo_count, &acquire_ctx); > +err: > + kfree(bos); > + > + return ret; > +} > + > +static void rocket_job_cleanup(struct kref *ref) > +{ > + struct rocket_job *job = container_of(ref, struct rocket_job, > + refcount); > + unsigned int i; > + > + dma_fence_put(job->done_fence); > + dma_fence_put(job->inference_done_fence); > + > + if (job->in_bos) { > + for (i = 0; i < job->in_bo_count; i++) > + drm_gem_object_put(job->in_bos[i]); > + > + kvfree(job->in_bos); > + } > + > + if (job->out_bos) { > + for (i = 0; i < job->out_bo_count; i++) > + drm_gem_object_put(job->out_bos[i]); > + > + kvfree(job->out_bos); > + } > + > + kfree(job->tasks); > + > + kfree(job); > +} > + > +static void rocket_job_put(struct rocket_job *job) > +{ > + kref_put(&job->refcount, rocket_job_cleanup); > +} > + > +static void rocket_job_free(struct drm_sched_job *sched_job) > +{ > + struct rocket_job *job = to_rocket_job(sched_job); > + > + drm_sched_job_cleanup(sched_job); > + > + rocket_job_put(job); > +} > + > +static struct rocket_core *sched_to_core(struct rocket_device *rdev, > + struct drm_gpu_scheduler *sched) > +{ > + unsigned int core; > + > + for (core = 0; core < rdev->num_cores; core++) { > + if (&rdev->cores[core].sched == sched) > + return &rdev->cores[core]; > + } > + > + return NULL; > +} > + > +static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job) > +{ > + struct rocket_job *job = to_rocket_job(sched_job); > + struct rocket_device *rdev = job->rdev; > + struct rocket_core *core = sched_to_core(rdev, sched_job->sched); > + struct dma_fence *fence = NULL; > + int ret; > + > + if (unlikely(job->base.s_fence->finished.error)) > + return NULL; > + > + /* > + * Nothing to execute: can happen if the job has finished while > + * we were resetting the GPU. > + */ > + if (job->next_task_idx == job->task_count) > + return NULL; > + > + fence = rocket_fence_create(core); > + if (IS_ERR(fence)) > + return fence; > + > + if (job->done_fence) > + dma_fence_put(job->done_fence); > + job->done_fence = dma_fence_get(fence); > + > + ret = pm_runtime_get_sync(core->dev); > + if (ret < 0) > + return fence; > + > + spin_lock(&core->job_lock); > + > + core->in_flight_job = job; > + rocket_job_hw_submit(core, job); > + > + spin_unlock(&core->job_lock); > + > + return fence; > +} > + > +static void rocket_job_handle_done(struct rocket_core *core, > + struct rocket_job *job) > +{ > + if (job->next_task_idx < job->task_count) { > + rocket_job_hw_submit(core, job); > + return; > + } > + > + core->in_flight_job = NULL; > + dma_fence_signal_locked(job->done_fence); > + pm_runtime_put_autosuspend(core->dev); > +} > + > +static void rocket_job_handle_irq(struct rocket_core *core) > +{ > + uint32_t status, raw_status; > + > + pm_runtime_mark_last_busy(core->dev); > + > + status = rocket_read(core, REG_PC_INTERRUPT_STATUS); > + raw_status = rocket_read(core, REG_PC_INTERRUPT_RAW_STATUS); > + > + rocket_write(core, REG_PC_OPERATION_ENABLE, 0x0); > + rocket_write(core, REG_PC_INTERRUPT_CLEAR, 0x1ffff); > + > + spin_lock(&core->job_lock); > + > + if (core->in_flight_job) > + rocket_job_handle_done(core, core->in_flight_job); > + > + spin_unlock(&core->job_lock); > +} > + > +static void > +rocket_reset(struct rocket_core *core, struct drm_sched_job *bad) > +{ > + bool cookie; > + > + if (!atomic_read(&core->reset.pending)) > + return; > + > + /* > + * Stop the scheduler. > + * > + * FIXME: We temporarily get out of the dma_fence_signalling section > + * because the cleanup path generate lockdep splats when taking locks > + * to release job resources. We should rework the code to follow this > + * pattern: > + * > + * try_lock > + * if (locked) > + * release > + * else > + * schedule_work_to_release_later > + */ > + drm_sched_stop(&core->sched, bad); > + > + cookie = dma_fence_begin_signalling(); > + > + if (bad) > + drm_sched_increase_karma(bad); > + > + /* > + * Mask job interrupts and synchronize to make sure we won't be > + * interrupted during our reset. > + */ > + rocket_write(core, REG_PC_INTERRUPT_MASK, 0x0); > + synchronize_irq(core->irq); > + > + /* Handle the remaining interrupts before we reset. */ > + rocket_job_handle_irq(core); > + > + /* > + * Remaining interrupts have been handled, but we might still have > + * stuck jobs. Let's make sure the PM counters stay balanced by > + * manually calling pm_runtime_put_noidle() and > + * rocket_devfreq_record_idle() for each stuck job. > + * Let's also make sure the cycle counting register's refcnt is > + * kept balanced to prevent it from running forever > + */ > + spin_lock(&core->job_lock); > + if (core->in_flight_job) > + pm_runtime_put_noidle(core->dev); > + > + core->in_flight_job = NULL; > + spin_unlock(&core->job_lock); > + > + /* Proceed with reset now. */ > + pm_runtime_force_suspend(core->dev); > + pm_runtime_force_resume(core->dev); > + > + /* GPU has been reset, we can clear the reset pending bit. */ > + atomic_set(&core->reset.pending, 0); > + > + /* > + * Now resubmit jobs that were previously queued but didn't have a > + * chance to finish. > + * FIXME: We temporarily get out of the DMA fence signalling section > + * while resubmitting jobs because the job submission logic will > + * allocate memory with the GFP_KERNEL flag which can trigger memory > + * reclaim and exposes a lock ordering issue. > + */ > + dma_fence_end_signalling(cookie); > + drm_sched_resubmit_jobs(&core->sched); > + cookie = dma_fence_begin_signalling(); > + > + /* Restart the scheduler */ > + drm_sched_start(&core->sched, 0); > + > + dma_fence_end_signalling(cookie); > +} > + > +static enum drm_gpu_sched_stat rocket_job_timedout(struct drm_sched_job *sched_job) > +{ > + struct rocket_job *job = to_rocket_job(sched_job); > + struct rocket_device *rdev = job->rdev; > + struct rocket_core *core = sched_to_core(rdev, sched_job->sched); > + > + /* > + * If the GPU managed to complete this jobs fence, the timeout is > + * spurious. Bail out. > + */ > + if (dma_fence_is_signaled(job->done_fence)) > + return DRM_GPU_SCHED_STAT_NOMINAL; > + > + /* > + * Rocket IRQ handler may take a long time to process an interrupt > + * if there is another IRQ handler hogging the processing. > + * For example, the HDMI encoder driver might be stuck in the IRQ > + * handler for a significant time in a case of bad cable connection. > + * In order to catch such cases and not report spurious rocket > + * job timeouts, synchronize the IRQ handler and re-check the fence > + * status. > + */ > + synchronize_irq(core->irq); > + > + if (dma_fence_is_signaled(job->done_fence)) { > + dev_warn(core->dev, "unexpectedly high interrupt latency\n"); > + return DRM_GPU_SCHED_STAT_NOMINAL; > + } > + > + dev_err(core->dev, "gpu sched timeout"); > + > + atomic_set(&core->reset.pending, 1); > + rocket_reset(core, sched_job); > + > + return DRM_GPU_SCHED_STAT_NOMINAL; > +} > + > +static void rocket_reset_work(struct work_struct *work) > +{ > + struct rocket_core *core; > + > + core = container_of(work, struct rocket_core, reset.work); > + rocket_reset(core, NULL); > +} > + > +static const struct drm_sched_backend_ops rocket_sched_ops = { > + .run_job = rocket_job_run, > + .timedout_job = rocket_job_timedout, > + .free_job = rocket_job_free > +}; > + > +static irqreturn_t rocket_job_irq_handler_thread(int irq, void *data) > +{ > + struct rocket_core *core = data; > + > + rocket_job_handle_irq(core); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t rocket_job_irq_handler(int irq, void *data) > +{ > + struct rocket_core *core = data; > + uint32_t raw_status = rocket_read(core, REG_PC_INTERRUPT_RAW_STATUS); > + > + WARN_ON(raw_status & PC_INTERRUPT_RAW_STATUS_DMA_READ_ERROR); > + WARN_ON(raw_status & PC_INTERRUPT_RAW_STATUS_DMA_READ_ERROR); > + > + if (!(raw_status & PC_INTERRUPT_RAW_STATUS_DPU_0 || > + raw_status & PC_INTERRUPT_RAW_STATUS_DPU_1)) > + return IRQ_NONE; > + > + rocket_write(core, REG_PC_INTERRUPT_MASK, 0x0); > + > + return IRQ_WAKE_THREAD; > +} > + > +int rocket_job_init(struct rocket_core *core) > +{ > + int ret; > + > + INIT_WORK(&core->reset.work, rocket_reset_work); > + spin_lock_init(&core->job_lock); > + > + core->irq = platform_get_irq(to_platform_device(core->dev), 0); > + if (core->irq < 0) > + return core->irq; > + > + ret = devm_request_threaded_irq(core->dev, core->irq, > + rocket_job_irq_handler, > + rocket_job_irq_handler_thread, > + IRQF_SHARED, KBUILD_MODNAME "-job", > + core); > + if (ret) { > + dev_err(core->dev, "failed to request job irq"); > + return ret; > + } > + > + core->reset.wq = alloc_ordered_workqueue("rocket-reset-%d", 0, core->index); > + if (!core->reset.wq) > + return -ENOMEM; > + > + core->fence_context = dma_fence_context_alloc(1); > + > + ret = drm_sched_init(&core->sched, > + &rocket_sched_ops, NULL, > + DRM_SCHED_PRIORITY_COUNT, > + 1, 0, > + msecs_to_jiffies(JOB_TIMEOUT_MS), > + core->reset.wq, > + NULL, "rocket", core->dev); > + if (ret) { > + dev_err(core->dev, "Failed to create scheduler: %d.", ret); > + goto err_sched; > + } > + > + return 0; > + > +err_sched: > + drm_sched_fini(&core->sched); > + > + destroy_workqueue(core->reset.wq); > + return ret; > +} > + > +void rocket_job_fini(struct rocket_core *core) > +{ > + drm_sched_fini(&core->sched); > + > + cancel_work_sync(&core->reset.work); > + destroy_workqueue(core->reset.wq); > +} > + > +int rocket_job_open(struct rocket_file_priv *rocket_priv) > +{ > + struct rocket_device *rdev = rocket_priv->rdev; > + struct drm_gpu_scheduler **scheds = kmalloc_array(rdev->num_cores, sizeof(scheds), > + GFP_KERNEL); > + unsigned int core; > + int ret; > + > + for (core = 0; core < rdev->num_cores; core++) > + scheds[core] = &rdev->cores[core].sched; > + > + ret = drm_sched_entity_init(&rocket_priv->sched_entity, > + DRM_SCHED_PRIORITY_NORMAL, > + scheds, > + rdev->num_cores, NULL); > + if (WARN_ON(ret)) > + return ret; > + > + return 0; > +} > + > +void rocket_job_close(struct rocket_file_priv *rocket_priv) > +{ > + struct drm_sched_entity *entity = &rocket_priv->sched_entity; > + > + kfree(entity->sched_list); > + drm_sched_entity_destroy(entity); > +} > + > +int rocket_job_is_idle(struct rocket_core *core) > +{ > + /* If there are any jobs in this HW queue, we're not idle */ > + if (atomic_read(&core->sched.credit_count)) > + return false; > + > + return true; > +} > + > +static int rocket_ioctl_submit_job(struct drm_device *dev, struct drm_file *file, > + struct drm_rocket_job *job) > +{ > + struct rocket_device *rdev = to_rocket_device(dev); > + struct rocket_file_priv *file_priv = file->driver_priv; > + struct rocket_job *rjob = NULL; > + int ret = 0; > + > + if (job->task_count == 0) > + return -EINVAL; > + > + rjob = kzalloc(sizeof(*rjob), GFP_KERNEL); > + if (!rjob) > + return -ENOMEM; > + > + kref_init(&rjob->refcount); > + > + rjob->rdev = rdev; > + > + ret = drm_sched_job_init(&rjob->base, > + &file_priv->sched_entity, > + 1, NULL); > + if (ret) > + goto out_put_job; > + > + ret = rocket_copy_tasks(dev, file, job, rjob); > + if (ret) > + goto out_cleanup_job; > + > + ret = drm_gem_objects_lookup(file, > + (void __user *)(uintptr_t)job->in_bo_handles, > + job->in_bo_handle_count, &rjob->in_bos); > + if (ret) > + goto out_cleanup_job; > + > + rjob->in_bo_count = job->in_bo_handle_count; > + > + ret = drm_gem_objects_lookup(file, > + (void __user *)(uintptr_t)job->out_bo_handles, > + job->out_bo_handle_count, &rjob->out_bos); > + if (ret) > + goto out_cleanup_job; > + > + rjob->out_bo_count = job->out_bo_handle_count; > + > + ret = rocket_job_push(rjob); > + if (ret) > + goto out_cleanup_job; > + > +out_cleanup_job: > + if (ret) > + drm_sched_job_cleanup(&rjob->base); > +out_put_job: > + rocket_job_put(rjob); > + > + return ret; > +} > + > +int rocket_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file) > +{ > + struct drm_rocket_submit *args = data; > + struct drm_rocket_job *jobs; > + int ret = 0; > + unsigned int i = 0; > + > + jobs = kvmalloc_array(args->job_count, sizeof(*jobs), GFP_KERNEL); > + if (!jobs) { > + DRM_DEBUG("Failed to allocate incoming job array\n"); > + return -ENOMEM; > + } > + > + if (copy_from_user(jobs, > + (void __user *)(uintptr_t)args->jobs, > + args->job_count * sizeof(*jobs))) { > + ret = -EFAULT; > + DRM_DEBUG("Failed to copy incoming job array\n"); > + goto exit; > + } > + > + for (i = 0; i < args->job_count; i++) > + rocket_ioctl_submit_job(dev, file, &jobs[i]); > + > +exit: > + kfree(jobs); > + > + return ret; > +} > diff --git a/drivers/accel/rocket/rocket_job.h b/drivers/accel/rocket/rocket_job.h > new file mode 100644 > index 0000000000000000000000000000000000000000..93fa1f988c72adb7a405acbf08c1c9b87d22f9c5 > --- /dev/null > +++ b/drivers/accel/rocket/rocket_job.h > @@ -0,0 +1,50 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright 2024 Tomeu Vizoso <tomeu(a)tomeuvizoso.net> */ > + > +#ifndef __ROCKET_JOB_H__ > +#define __ROCKET_JOB_H__ > + > +#include <drm/drm_drv.h> > +#include <drm/gpu_scheduler.h> > + > +#include "rocket_core.h" > +#include "rocket_drv.h" > + > +struct rocket_task { > + u64 regcmd; > + u32 regcmd_count; > +}; > + > +struct rocket_job { > + struct drm_sched_job base; > + > + struct rocket_device *rdev; > + > + struct drm_gem_object **in_bos; > + struct drm_gem_object **out_bos; > + > + u32 in_bo_count; > + u32 out_bo_count; > + > + struct rocket_task *tasks; > + u32 task_count; > + u32 next_task_idx; > + > + /* Fence to be signaled by drm-sched once its done with the job */ > + struct dma_fence *inference_done_fence; > + > + /* Fence to be signaled by IRQ handler when the job is complete. */ > + struct dma_fence *done_fence; > + > + struct kref refcount; > +}; > + > +int rocket_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file); > + > +int rocket_job_init(struct rocket_core *core); > +void rocket_job_fini(struct rocket_core *core); > +int rocket_job_open(struct rocket_file_priv *rocket_priv); > +void rocket_job_close(struct rocket_file_priv *rocket_priv); > +int rocket_job_is_idle(struct rocket_core *core); > + > +#endif > diff --git a/include/uapi/drm/rocket_accel.h b/include/uapi/drm/rocket_accel.h > index 8338726a83c31b954608ca505cf78bcd70d3494b..eb886351134ebef62969b1e1182ccc174f88fe9d 100644 > --- a/include/uapi/drm/rocket_accel.h > +++ b/include/uapi/drm/rocket_accel.h > @@ -12,8 +12,10 @@ extern "C" { > #endif > > #define DRM_ROCKET_CREATE_BO 0x00 > +#define DRM_ROCKET_SUBMIT 0x01 > > #define DRM_IOCTL_ROCKET_CREATE_BO DRM_IOWR(DRM_COMMAND_BASE + DRM_ROCKET_CREATE_BO, struct drm_rocket_create_bo) > +#define DRM_IOCTL_ROCKET_SUBMIT DRM_IOW(DRM_COMMAND_BASE + DRM_ROCKET_SUBMIT, struct drm_rocket_submit) > > /** > * struct drm_rocket_create_bo - ioctl argument for creating Rocket BOs. > @@ -36,6 +38,59 @@ struct drm_rocket_create_bo { > __u64 offset; > }; > > +/** > + * struct drm_rocket_task - A task to be run on the NPU > + * > + * A task is the smallest unit of work that can be run on the NPU. > + */ > +struct drm_rocket_task { > + /** DMA address to NPU mapping of register command buffer */ > + __u64 regcmd; > + > + /** Number of commands in the register command buffer */ > + __u32 regcmd_count; > +}; > + > +/** > + * struct drm_rocket_job - A job to be run on the NPU > + * > + * The kernel will schedule the execution of this job taking into account its > + * dependencies with other jobs. All tasks in the same job will be executed > + * sequentially on the same core, to benefit from memory residency in SRAM. > + */ > +struct drm_rocket_job { > + /** Pointer to an array of struct drm_rocket_task. */ > + __u64 tasks; > + > + /** Pointer to a u32 array of the BOs that are read by the job. */ > + __u64 in_bo_handles; > + > + /** Pointer to a u32 array of the BOs that are written to by the job. */ > + __u64 out_bo_handles; > + > + /** Number of tasks passed in. */ > + __u32 task_count; > + > + /** Number of input BO handles passed in (size is that times 4). */ > + __u32 in_bo_handle_count; > + > + /** Number of output BO handles passed in (size is that times 4). */ > + __u32 out_bo_handle_count; > +}; > + > +/** > + * struct drm_rocket_submit - ioctl argument for submitting commands to the NPU. > + * > + * The kernel will schedule the execution of these jobs in dependency order. > + */ > +struct drm_rocket_submit { > + /** Pointer to an array of struct drm_rocket_job. */ > + __u64 jobs; > + > + /** Number of jobs passed in. */ > + __u32 job_count; > +}; > + > #if defined(__cplusplus) > } > #endif > -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)

9 months, 3 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig