On Mon, Apr 14, 2025 at 10:48:49AM +0300, Dmitry Baryshkov wrote:
> On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
> >
> >
> > On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> > >
> > >
> > > On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > > > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekang...@quicinc.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > > > > > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> > > > > > > On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > > > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> > > > > > > > > On 11/18/2024 7:32 PM, Greg KH wrote:
> > > > > > > > > > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta
> > > > > > > > > > wrote:
> > > > > > > > > > > Add changes to support debugfs. The fastrpc directory
> > > > > > > > > > > will be
> > > > > > > > > > > created which will carry debugfs files for all fastrpc
> > > > > > > > > > > processes.
> > > > > > > > > > > The information of fastrpc user and channel contexts are
> > > > > > > > > > > getting
> > > > > > > > > > > captured as part of this change.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Ekansh Gupta <quic_ekang...@quicinc.com>
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/misc/fastrpc/Makefile | 3 +-
> > > > > > > > > > > drivers/misc/fastrpc/fastrpc_debug.c | 156
> > > > > > > > > > > +++++++++++++++++++++++++++
> > > > > > > > > > > drivers/misc/fastrpc/fastrpc_debug.h | 31 ++++++
> > > > > > > > > > > drivers/misc/fastrpc/fastrpc_main.c | 18 +++-
> > > > > > > > > > > 4 files changed, 205 insertions(+), 3 deletions(-)
> > > > > > > > > > > create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > index 020d30789a80..4ff6b64166ae 100644
> > > > > > > > > > > --- a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > +++ b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > @@ -1,3 +1,4 @@
> > > > > > > > > > > # SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > obj-$(CONFIG_QCOM_FASTRPC) += fastrpc.o
> > > > > > > > > > > -fastrpc-objs := fastrpc_main.o
> > > > > > > > > > > \ No newline at end of file
> > > > > > > > > > > +fastrpc-objs := fastrpc_main.o \
> > > > > > > > > > > + fastrpc_debug.o
> > > > > > > > > > Only build this file if debugfs is enabled.
> > > > > > > > > >
> > > > > > > > > > And again, "debug.c"?
> > > > > > > > > I'll add change to build this only if debugfs is enabled.
> > > > > > > > > Going forward I have plans to add
> > > > > > > > > few more debug specific changes, maybe then I'll need to
> > > > > > > > > change the build rules again.
> > > > > > > > > > > diff --git a/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000000000000..cdb4fc6845a8
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > @@ -0,0 +1,156 @@
> > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > +// Copyright (c) 2024 Qualcomm Innovation Center.
> > > > > > > > > > > +
> > > > > > > > > > > +#include <linux/debugfs.h>
> > > > > > > > > > > +#include <linux/seq_file.h>
> > > > > > > > > > > +#include "fastrpc_shared.h"
> > > > > > > > > > > +#include "fastrpc_debug.h"
> > > > > > > > > > > +
> > > > > > > > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > > > > > > Please put the #ifdef in the .h file, not in the .c file.
> > > > > > > > > Ack
> > > > > > > > > > > +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> > > > > > > > > > > +{
> > > > > > > > > > > + char cur_comm[TASK_COMM_LEN];
> > > > > > > > > > > + int domain_id, size;
> > > > > > > > > > > + char *debugfs_buf;
> > > > > > > > > > > + struct dentry *debugfs_dir =
> > > > > > > > > > > fl->cctx->debugfs_dir;
> > > > > > > > > > > +
> > > > > > > > > > > + memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> > > > > > > > > > > + cur_comm[TASK_COMM_LEN-1] = '\0';
> > > > > > > > > > > + if (debugfs_dir != NULL) {
> > > > > > > > > > > + domain_id = fl->cctx->domain_id;
> > > > > > > > > > > + size = snprintf(NULL, 0,
> > > > > > > > > > > "%.10s_%d_%d_%d", cur_comm,
> > > > > > > > > > > + current->pid, fl->tgid,
> > > > > > > > > > > domain_id) + 1;
> > > > > > > > > > > + debugfs_buf = kzalloc(size, GFP_KERNEL);
> > > > > > > > > > > + if (debugfs_buf == NULL)
> > > > > > > > > > > + return;
> > > > > > > > > > > + /*
> > > > > > > > > > > + * Use HLOS process name, HLOS PID,
> > > > > > > > > > > fastrpc user TGID,
> > > > > > > > > > > + * domain_id in debugfs filename to
> > > > > > > > > > > create unique file name
> > > > > > > > > > > + */
> > > > > > > > > > > + snprintf(debugfs_buf, size,
> > > > > > > > > > > "%.10s_%d_%d_%d",
> > > > > > > > > > > + cur_comm, current->pid,
> > > > > > > > > > > fl->tgid, domain_id);
> > > > > > > > > > > + fl->debugfs_file =
> > > > > > > > > > > debugfs_create_file(debugfs_buf, 0644,
> > > > > > > > > > > + debugfs_dir, fl,
> > > > > > > > > > > &fastrpc_debugfs_fops);
> > > > > > > > > > Why are you saving the debugfs file? What do you need to
> > > > > > > > > > do with it
> > > > > > > > > > that you can't just delete the whole directory, or look up
> > > > > > > > > > the name
> > > > > > > > > > again in the future when removing it?
> > > > > > > > > fl structure is specific to a process using fastrpc driver.
> > > > > > > > > The reason to save
> > > > > > > > > this debugfs file is to delete is when the process releases
> > > > > > > > > fastrpc device.
> > > > > > > > > If the file is not deleted, it might flood multiple files in
> > > > > > > > > debugfs directory.
> > > > > > > > >
> > > > > > > > > As part of this change, only the file that is getting created
> > > > > > > > > by a process is
> > > > > > > > > getting removed when process is releasing device and I don't
> > > > > > > > > think we
> > > > > > > > > can clean up the whole directory at this point.
> > > > > > > > My 2c: it might be better to create a single file that conains
> > > > > > > > information for all the processes instead of that. Or use
> > > > > > > > fdinfo data to
> > > > > > > > export process / FD information to userspace.
> > > > > > > Thanks for your review. The reason of not having single file for
> > > > > > > all processes is that
> > > > > > > I can run 100s of iteration for any process(say calculator) and
> > > > > > > every time the properties
> > > > > > > of the process can differ(like buffer, session etc.). For this
> > > > > > > reason, I'm creating and
> > > > > > > deleting the debugfs files for every process run.
> > > > > > >
> > > > > > > Do you see any advantage of using fdinfo over debugfs? I'm not
> > > > > > > sure if we can add all
> > > > > > > the information(like in debugfs) here.
> > > > > > Which information is actually useful / interesting for application
> > > > > > developers? If not for the fdinfo, I might still vote for a single
> > > > > > file
> > > > > > rather than a pile of per-process data.
> > Let’s say I am trying to do debugfs read when 10+ or more sessions are
> > active per channel, then for pushing data of nth process in a single file, I
> > would have to wait for n-1 processes, by that time process data might get
> > changed. How do you suggest handling this?
>
> I'm yet to see the response to my question, what kind of information are
> you outputting? What is actually relevant? Could you please provide an
> example from the running system, so that we don't have to guess?
And meanwhile could you please take a look at existing implementations?
E.g /sys/kernel/debug/dri/*/state, /sys/kernel/debug/dri/*/gem and the
fdinfo files for the process actually using drm/msm driver (you will see
memory consumption data there).
>
> > > > > I have tried to capture all the information that could be useful.
> > > > >
> > > > > I can try changes to maintain single file for all active processes.
> > > > > Having this file specific
> > > > > to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp,
> > > > > etc.? Each file will
> > > > > carry information of all processes using that remoteproc.
> > > > I think it's a better idea, yes.
> > >
> > > Hi all,
> > >
> > > I'm adding Deepika <quic_...@quicinc.com> to this thread who is reworking
> > > on this patch series.
> > >
> > > //Ekansh
> > >
> > > > > --ekansh
> > > > > > > --ekansh
> > > >
> > >
> >
>
> --
> With best wishes
> Dmitry
--
With best wishes
Dmitry
