lkml.org 
[lkml]   [2018]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH 3/6] char: fastrpc: Add support for context Invoke method
    From
    Date


    On 30/11/18 16:19, Arnd Bergmann wrote:
    > On Fri, Nov 30, 2018 at 5:03 PM Srinivas Kandagatla
    > <srinivas.kandagatla@linaro.org> wrote:
    >> On 30/11/18 15:08, Arnd Bergmann wrote:
    >>> On Fri, Nov 30, 2018 at 4:01 PM Srinivas Kandagatla
    >>> <srinivas.kandagatla@linaro.org> 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@linaro.org> 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.
    >
    > That does not sound like enough of a reason then, user space is
    > easy to change here to just allocate the memory from the device itself.
    > The only reason that I can see for needing a dmabuf would be if
    > you have to share a buffer between two instances, and then you
    > can use either of them.

    I agree, I will try rework this and remove the instances that does not
    require sessions!

    Sharing buffer is also a reason for dmabuf here.

    >
    >>>>>> +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.
    >
    > I don't get it yet. What are 'compute sessions'? Do you have
    > multiple threads running on a single instance at the same time?

    compute sessions are "compute context-banks" instances in DSP.

    DSP supports multiple compute banks, Ideally 12 context banks, 4 which
    are reserved for other purposes and 8 of them are used for compute, one
    for each process. So ideally we can run 8 parallel computes.


    > I would have expected to only ever see one thread in the
    > 'wait_for_completion()' above, and others possibly waiting
    > for a chance to get to but not already running.
    >
    >>>> 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;
    >> };
    >
    > That is still two structure, not one ;-)
    >
    >>>> 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.
    >
    > Do you mean we have a variable number 'sc', but each array
    > always has the same length as the other ones? In that
    > case: yes, combining them seems sensible.
    Yes thats what I meant!

    >
    > The other question this raises is: what is 'handle'?
    > Why is the file descriptor not enough to identify the
    > instance we want to talk to?
    This is remote handle to opened interface on which this method has to be
    invoked.
    For example we are running a calculator application, calculator will
    have a unique handle on which calculate() method needs to be invoked.


    thanks,
    srini
    >
    > Arnd
    >

    \
     
     \ /
      Last update: 2018-11-30 17:41    [W:3.896 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site