On Mon, 10 Feb 2025 at 11:05, Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx> 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@xxxxxxxxxxx> > >>>> --- > >>>> 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
