Message-ID: <CAA8EJppDK8MZw0s-2sOM9mf3smRfqCcyy=opq0dEF0AOCnWraA@mail.gmail.com>
Date: Mon, 10 Feb 2025 12:35:46 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Ekansh Gupta <quic_ekangupt@...cinc.com>
Cc: srinivas.kandagatla@...aro.org, linux-arm-msm@...r.kernel.org, 
	gregkh@...uxfoundation.org, quic_bkumar@...cinc.com, 
	linux-kernel@...r.kernel.org, quic_chennak@...cinc.com, 
	dri-devel@...ts.freedesktop.org, arnd@...db.de
Subject: Re: [PATCH v2 5/5] misc: fastrpc: Modify context id mask to support
 polling mode

On Mon, 10 Feb 2025 at 11:05, Ekansh Gupta <quic_ekangupt@...cinc.com> wrote:
>
>
>
>
> On 1/29/2025 4:53 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 29, 2025 at 11:13:05AM +0530, Ekansh Gupta wrote:
> >>
> >>
> >> On 1/29/2025 5:00 AM, Dmitry Baryshkov wrote:
> >>> On Mon, Jan 27, 2025 at 10:12:39AM +0530, Ekansh Gupta wrote:
> >>>> DSP needs last 4 bits of context id to be 0 for polling mode to be
> >>>> supported as setting of last 8 is intended for async mode(not yet
> >>>> supported on upstream driver) and setting these bits restrics
> >>>> writing to poll memory from DSP. Modify context id mask to ensure
> >>>> polling mode is supported.
> >>> Shouldn't this commit come before the previous one?
> >> Yes, I'll change the order in next series.
> >>
> >> Thanks for reviewing the changes.
> > Please consider asking somebody for the internal review before sending
> > patches. This should help you to catch such mistakes.
> >
> > Next, I keep on asking for a sensible userspace and testing suite. No,
> > existing fastrpc doesn't pass that criteria. We have discussed that, but
> > I see no changes in the development. The PR that you've linked in the
> > cover letter contains a single commit, covering documentation, new
> > IOCTL, CRC support, poll mode support, etc. What if the discussion ends
> > up accepting the CRC support but declining the polling mode? Or vice
> > versa, accepting poll mode but declining the CRC support? There is no
> > easy way for us to review userspace impact, corresponding changes, etc.
>
> We are working with our Legal team to push HexagonSDK publicly , that  will
> have sample apps for all features supported by upstream driver and can be used
> for testing.
>
> I'll break down the PR to multiple meaningful commits based on the features
> that are getting added.
>
> >
> > Last, but not least: I want to bring up Sima's response in one of the
> > earlier discussions ([1]): "Yeah, if fastrpc just keeps growing the
> > story will completely different."
> >
> > You are adding new IOCTL and new ivocation API. That totally sounds
> > like "keeps growing", which returns us back to the uAPI question,
> > drm_accel.h and the rest of the questions on the userspace, compiler,
> > etc.
> >
> > [1] https://lore.kernel.org/dri-devel/Znk87-xCx8f3fIUL@phenom.ffwll.local/
>
> Currently, we are upstreaming the features supported on DSP for publicly
> available platforms. No features for future platforms are planned for FastRPC
> driver.
>
> We are also looking in having the driver under drivers/accel for any new features
> that are planned in future platforms.

Granted that there is a single driver, supporting all platforms, I
don't think that supporting new vs old platforms makes any sense.
The message from Sima was about growing the driver / uAPI. From my
point of view, adding a new IOCTL points out active driver development
(from the upstream kernel point of view).
Please talk to your only upstream users - libssc / hexagonrpcd
developers. It should be them reviewing your uAPI changes, not just
me.

And yes, from my side, the question would be the same: if you are
adding a new uAPI, why is it not following drm_accel.h? "It has been
done this way ages ago" isn't an answer for _new_ IOCTLs.

>
> --ekansh
>
> >
> >
> >> --ekansh
> >>
> >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
> >>>> ---
> >>>>  drivers/misc/fastrpc.c | 8 ++++----
> >>>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>> index 257a741af115..ef56c793c564 100644
> >>>> --- a/drivers/misc/fastrpc.c
> >>>> +++ b/drivers/misc/fastrpc.c
> >>>> @@ -40,7 +40,7 @@
> >>>>  #define FASTRPC_INIT_HANDLE       1
> >>>>  #define FASTRPC_DSP_UTILITIES_HANDLE      2
> >>>>  #define FASTRPC_MAX_STATIC_HANDLE (20)
> >>>> -#define FASTRPC_CTXID_MASK (0xFF0)
> >>>> +#define FASTRPC_CTXID_MASK (0xFF0000)
> >>>>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
> >>>>  #define INIT_FILE_NAMELEN_MAX (128)
> >>>>  #define FASTRPC_DEVICE_NAME       "fastrpc"
> >>>> @@ -524,7 +524,7 @@ static void fastrpc_context_free(struct kref *ref)
> >>>>            fastrpc_buf_free(ctx->buf);
> >>>>
> >>>>    spin_lock_irqsave(&cctx->lock, flags);
> >>>> -  idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);
> >>>> +  idr_remove(&cctx->ctx_idr, ctx->ctxid >> 16);
> >>>>    spin_unlock_irqrestore(&cctx->lock, flags);
> >>>>
> >>>>    kfree(ctx->maps);
> >>>> @@ -664,7 +664,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> >>>>            spin_unlock_irqrestore(&cctx->lock, flags);
> >>>>            goto err_idr;
> >>>>    }
> >>>> -  ctx->ctxid = ret << 4;
> >>>> +  ctx->ctxid = ret << 16;
> >>>>    spin_unlock_irqrestore(&cctx->lock, flags);
> >>>>
> >>>>    kref_init(&ctx->refcount);
> >>>> @@ -2675,7 +2675,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
> >>>>    if (len < sizeof(*rsp))
> >>>>            return -EINVAL;
> >>>>
> >>>> -  ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 4);
> >>>> +  ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 16);
> >>>>
> >>>>    spin_lock_irqsave(&cctx->lock, flags);
> >>>>    ctx = idr_find(&cctx->ctx_idr, ctxid);
> >>>> --
> >>>> 2.34.1
> >>>>
>


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists