On 30/11/18 15:08, Arnd Bergmann wrote:
On Fri, Nov 30, 2018 at 4:01 PM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
Thanks Arnd for the review comments!
On 30/11/18 13:41, Arnd Bergmann wrote:
On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
+static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct fastrpc_user *fl = (struct fastrpc_user *)file->private_data;
+ struct fastrpc_channel_ctx *cctx = fl->cctx;
+ char __user *argp = (char __user *)arg;
+ int err;
+
+ if (!fl->sctx) {
+ fl->sctx = fastrpc_session_alloc(cctx, 0);
+ if (!fl->sctx)
+ return -ENOENT;
+ }
Shouldn't that session be allocated during open()?
Yes, and no, we do not need context in all the cases. In cases like we
just want to allocate dmabuf.
Can you give an example what that would be good for?
Currently the instance which does not need session is used as simple
memory allocator (rpcmem), TBH, this is the side effect of trying to fit
in with downstream application infrastructure which uses ion for andriod
usecases.
+static void fastrpc_notify_users(struct fastrpc_user *user)
+{
+ struct fastrpc_invoke_ctx *ctx, *n;will go
+
+ spin_lock(&user->lock);
+ list_for_each_entry_safe(ctx, n, &user->pending, node)
+ complete(&ctx->work);
+ spin_unlock(&user->lock);
+}
Can you explain here what it means to have multiple 'users'
a 'fastrpc_user' structure? Why are they all done at the same time?
user is allocated on every open(). Having multiple users means that
there are more than one compute sessions running on a given dsp.
They reason why all the users are notified here is because the dsp is
going down, so all the compute sessions associated with it will not see
any response from dsp, so any pending/waiting compute contexts are
explicitly notified.
This is the case where users need to be notified if the dsp goes down
due to crash or shut down!
What is a 'user' then? My point is that it seems to refer to two
different things here. I assume 'fastrpc_user' is whoever
has opened the file descriptor.
+struct fastrpc_ioctl_invoke {
+ uint32_t handle; /* remote handle */
+ uint32_t sc; /* scalars describing the data */
+ remote_arg_t *pra; /* remote arguments list */
+ int *fds; /* fd list */
+ unsigned int *attrs; /* attribute list */
+ unsigned int *crc;
+};
This seems too complex for an ioctl argument, with
multiple levels of pointer indirections. I'd normally
try to make each ioctl argument either a scalar, or a
structure with only fixed-length members.
I totally agree with you and many thanks for your expert inputs,
May be something like below with fixed length members would work?
struct fastrpc_remote_arg {
__u64 ptr; /* buffer ptr */
__u64 len; /* length */
__u32 fd; /* dmabuf fd */
__u32 reserved1
__u64 reserved2
};
struct fastrpc_remote_fd {
__u64 fd;
__u64 reserved1
__u64 reserved2
__u64 reserved3
};
struct fastrpc_remote_attr {
__u64 attr;
__u64 reserved1
__u64 reserved2
__u64 reserved3
};
struct fastrpc_remote_crc {
__u64 crc;
__u64 reserved1
__u64 reserved2
__u64 reserved3
};
I don't see a need to add extra served fields for structures
that are already naturally aligned here, e.g. in
fastrpc_remote_arg we need the 'reserved1' but not
the 'reserved2'.
Yes, I see, I overdone it!
Other idea, is, may be I can try to combine these into single structure
something like:
struct fastrpc_invoke_arg {
__u64 ptr;
__u64 len;
__u32 fd;
__u32 reserved1
__u64 attr;
__u64 crc;
};
struct fastrpc_ioctl_invoke {
__u32 handle;
__u32 sc;
/* The minimum size is scalar_length * 32*/
struct fastrpc_invoke_args *args;
};
struct fastrpc_ioctl_invoke {
__u32 handle;
__u32 sc;
/* The minimum size is scalar_length * 32 */
struct fastrpc_remote_args *rargs;
struct fastrpc_remote_fd *fds;
struct fastrpc_remote_attr *attrs;
struct fastrpc_remote_crc *crc;
};
Do these really have to be indirect then? Are they all
lists of variable length? How do you know how long?
Yes, they are variable length and will be scalar length long.
Scalar length is derived from sc variable in this structure.
--srini
Arnd
