Skip to content

fastrpcdlkm: Add recipe to support fastrpc DLKM #857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fastrpcdlkm: Add recipe to support fastrpc DLKM #857

wants to merge 1 commit into from

Conversation

quic-ekangupt
Copy link

FastRPC DLKM carries additional features like static PD restart and enhanced invocation support. Add recipe to support fastrpc DLKM.

@quic-ekangupt
fastrpcdlkm: Add recipe to support fastrpc DLKM
3ed4f50 
FastRPC DLKM carries additional features like static PD restart
and enhanced invocation support. Add recipe to support fastrpc
DLKM.

Signed-off-by: Ekansh Gupta <[email protected]>
@github-actions GitHub Actions
Copy link

Test Results

 3 files  ±0   6 suites  ±0   2m 18s ⏱️ ±0s
23 tests ±0  23 ✅ ±0  0 💤 ±0  0 ❌ ±0 
57 runs  ±0  57 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 3ed4f50. ± Comparison against base commit e701e3a.

@github-actions GitHub Actions
Copy link

Test jobs for commit 3ed4f50

@sbanerjee-quic
Copy link
Contributor

@quic-ekangupt need the corresponding change to include this in the CI

@vkraleti
Copy link
Contributor

@quic-ekangupt as discussed in #854 (comment) as this is a kernel module recipe, place it under recipes-kernel/fastrpc-module.

vkraleti
vkraleti reviewed Apr 30, 2025
View reviewed changes
recipes-multimedia/fastrpcdlkm/fastrpcdlkm_1.0.bb

RPROVIDES:${PN} += "kernel-module-fastrpc-kernel"

FILESPATH =+ "${WORKSPACE}:"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

recipes-multimedia/fastrpcdlkm/fastrpcdlkm_1.0.bb

EXTRA_OEMAKE += "M=${S}"

RPROVIDES:${PN} += "kernel-module-fastrpc-kernel"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why to use an additional RPROVIDES? Which other recipe is referring this module as "fastrpc-kernel" ?

recipes-multimedia/fastrpcdlkm/fastrpcdlkm_1.0.bb

MAKE_TARGETS = "modules"

MODULES_INSTALL_TARGET = "modules_install"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need. modules.bbclass already defaults to this.

recipes-multimedia/fastrpcdlkm/fastrpcdlkm_1.0.bb

SRC_URI = "git://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/dsp-kernel.git;protocol=https;branch=dsp-kernel.lnx.3.3;destsuffix=vendor/qcom/opensource/dsp-kernel"

S = "${WORKDIR}/vendor/qcom/opensource/dsp-kernel"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove destsuffix in SRC_URI and set S = "${WORKDIR}/git"

@lumag
Copy link
Collaborator

lumag commented Apr 30, 2025 •
edited
Loading

Definite NAK. Please include the relevant patches into the linux-yocto / linux-qcom rather than pulling full DKML.
DKLMs are a way to provide missing drivers for the IP blocks that we don't support upstream (like vidc, camera ISP, etc). If we have a driver, we should not be using DKLMs in a way to circumvent upstreaming rules / requirements.

@ricardosalveti
Copy link
Contributor

Didn't compare the sources, but is this a fork of the upstream driver? If so I'm also aligned with @lumag, should be proposed as kernel patches instead.

@quic-ekangupt
Copy link
Author

Definite NAK. Please include the relevant patches into the linux-yocto / linux-qcom rather than pulling full DKML. DKLMs are a way to provide missing drivers for the IP blocks that we don't support upstream (like vidc, camera ISP, etc). If we have a driver, we should not be using DKLMs in a way to circumvent upstreaming rules / requirements.

I have 1 question regarding maintaining patches into linux-yocto / linux-qcom, what are the expectations for any patch carrying uAPI change?

@quic-ekangupt
Copy link
Author

Definite NAK. Please include the relevant patches into the linux-yocto / linux-qcom rather than pulling full DKML. DKLMs are a way to provide missing drivers for the IP blocks that we don't support upstream (like vidc, camera ISP, etc). If we have a driver, we should not be using DKLMs in a way to circumvent upstreaming rules / requirements.

I have 1 question regarding maintaining patches into linux-yocto / linux-qcom, what are the expectations for any patch carrying uAPI change?

I'm seeking details for this, please help with your comments. @lumag, @ricardosalveti

@ndechesne
Copy link
Contributor

if the uAPI change has been discussed and reviewed upstream it's not a problem. If it's a custom never-going-upstream uAPI, we are going to push back on it.

Please note that we are moving soon away from linux-yocto-dev, and we will be using a QCOM hosted kernel (qcom-next). At this point we will no longer carry any kernel patches in meta-qcom.

@quic-ekangupt
Copy link
Author

if the uAPI change has been discussed and reviewed upstream it's not a problem. If it's a custom never-going-upstream uAPI, we are going to push back on it.

Please note that we are moving soon away from linux-yocto-dev, and we will be using a QCOM hosted kernel (qcom-next). At this point we will no longer carry any kernel patches in meta-qcom.

Thanks for the explanation. I have 2 follow-up questions:

  1. For any feature which uAPI change with no immediate upstream plans, What should be the steps once migration to qcom-next is done? I believe such changes cannot be merged in qcom-next(or topic branches).

  2. Say if any uAPI change is under review upstream and is merged onto qcom-next, can I pull the corresponding user library change over here to meta-qcom(maybe as a .patch)?

@ndechesne
Copy link
Contributor

Thanks for the explanation. I have 2 follow-up questions:

  1. For any feature which uAPI change with no immediate upstream plans, What should be the steps once migration to qcom-next is done? I believe such changes cannot be merged in qcom-next(or topic branches).

That's when we get into issues :) and when we need to deep dive a bit more.
The right approach would be to have a 'less features' variant of your software (e.g. what we call the base variant). Which is intended to work purely on top of upstream.

Note that with the 'clean overlay' approach and ongoing discussions, this will be no longer acceptable anyway. e.g. any of our QCOM 'value add' (or non upstream, proprietary, ...) solutions will need to work with a vanilla upstream kernel.

  1. Say if any uAPI change is under review upstream and is merged onto qcom-next, can I pull the corresponding user library change over here to meta-qcom(maybe as a .patch)?

Yes, if the uAPI change is actively being upstream and with some hints that it will eventually get merged, then it will be accepted in qcom-next, and then we will accept corresponding user space changes in meta-qcom, of course.

@lumag
Copy link
Collaborator

lumag commented May 16, 2025

@quic-ekangupt for the sake of it. No, adding extra uAPI to fastrpc driver is not possible. If I read it correctly, it was vetoed by Simona.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vkraleti vkraleti vkraleti left review comments

@ricardosalveti ricardosalveti Awaiting requested review from ricardosalveti ricardosalveti is a code owner

@sbanerjee-quic sbanerjee-quic Awaiting requested review from sbanerjee-quic sbanerjee-quic is a code owner

@ndechesne ndechesne Awaiting requested review from ndechesne ndechesne is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@quic-ekangupt @sbanerjee-quic @vkraleti @lumag @ricardosalveti @ndechesne