camxcommon: package provide common utility API used by all CamX components #1231
Conversation
gkhose-qipl
requested review from
lumag,
ndechesne,
ricardosalveti,
sbanerjee-quic and
vkraleti
as code owners
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
I reviewed the first recipe. Please apply that to all recipes in the PR
| @@ -0,0 +1,21 @@ | |||
| SUMMARY = "Qualcomm camera core driver and pipeline related libraries" | |||
There was a problem hiding this comment.
Please write a proper commit message. Describe your design decisions, issues, etc. It should be a text, rather than a bullet list.
There was a problem hiding this comment.
This is still not addressed.
| @@ -0,0 +1,21 @@ | |||
| SUMMARY = "Qualcomm camera core driver and pipeline related libraries" | |||
| DESCRIPTION = "Collection of prebuilt libraries to support camera downstream functionality." | |||
| LICENSE = "CLOSED" | |||
There was a problem hiding this comment.
Please add a proper license. It should actual use and distribution terms.
| SRC_URI = "https://qartifactory-edge.qualcomm.com/artifactory/qsc_releases/software/chip/component/camx.qclinux.0.0/251120/prebuilt_yocto/${BPN}_${PV}_armv8-2a.tar.gz;subdir=${BPN}-${PV}" | ||
| SRC_URI[sha256sum] = "c0b2f23c0c87df6b113466b83948cf2baf80b38492e1899984193108a0bab8e3" | ||
|
|
||
| DEPENDS += "camxfirmware fastrpc protobuf-native protobuf protobuf-native libxml2 virtual/egl virtual/libgles2" |
There was a problem hiding this comment.
No, libs don't depend on the firmware. Especially not at the build time.
|
Please break into more commits, one single commit adding a bunch of recipes is not ideal. |
gkhose-qipl
force-pushed
the
camx_downstream
branch
2 times, most recently
from
64768be to
78cfd3c
Compare
| @@ -0,0 +1,18 @@ | |||
| SUMMARY = "Qualcomm camera development related libraries, binary" | |||
| DESCRIPTION = "Collection of prebuilt libraries to support camera downstream functionality." | |||
There was a problem hiding this comment.
Summary and description for most recipes are quite similar here, you should make it cover just this component (explain why it exists).
There was a problem hiding this comment.
Like, what chicdk even means?
| @@ -0,0 +1,18 @@ | |||
| SUMMARY = "Qualcomm camera development related libraries, binary" | |||
| DESCRIPTION = "Collection of prebuilt libraries to support camera downstream functionality." | |||
| LICENSE = "CLOSED" | |||
There was a problem hiding this comment.
Need proper license.
Also add NOTICE (from the tarball) in LIC_FILES_CHKSUM.
|
Please. Test your changes before sending the PR. Building |
|
Also, please fix your build system: |
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
On top of everything. Please rename the packages in a sensible way.
Instead of -kt use -kodiak. Likewise add sensible suffix to qcs9100-related packages.
| @@ -0,0 +1,19 @@ | |||
| SUMMARY = "Qualcomm camera common utility API used by camera driver" | |||
There was a problem hiding this comment.
This package includes files under /usr/lib/qcs9100/. Why are they a part of this package? Why is it split from camxlib?
There was a problem hiding this comment.
Also, we are definitely going to have several other versions of camx-something. Please name all packages according to platforms they support.
There was a problem hiding this comment.
These comments were totally ignored.
gkhose-qipl
force-pushed
the
camx_downstream
branch
from
78cfd3c to
3ff362b
Compare
gkhose-qipl
changed the title
gkhose-qipl
force-pushed
the
camx_downstream
branch
from
3ff362b to
2c40208
Compare
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
Please fix all review comments instead of fixing them one by one.
Still being ignored |
recipe name : qcm6490 target lib path recipe name : qcs9100, qcs8300, qcs615, upcoming targets... lib path Proposal : Could you please review proposal and update comments. For now looking for exception in the current form, As part of qcs615 enablement (end of Jan), this will be fixed as per proposal alignment |
How does one go from the platform name to v1 /v2 / etc? Especially since it is not related to any other version nor to the hardware block. |
Also, why do you need more and more exceptions? Why can't the work be done properly from the beginning? |
Last, but not least, why do we need extra level of subdirs? I like the current proposal of |
v1 and v2 with the same versions? Makes no sense to me. Is this supposed to be aligned for a certain family of socs? Can you describe what v1 and v2 actually means? |
- package provide common utility API used by all CamX components - Provides common functionality such as logging, basic data structures, and configuration handling. - Helps reduce duplication by centralizing generic logic that does not belong to any single feature. - Offers small, focused components that can be reused without pulling in heavy dependencies. Signed-off-by: Ganesh Khose <[email protected]>
gkhose-qipl
force-pushed
the
camx_downstream
branch
from
640d3eb to
f7ea0fd
Compare
|
camxv1 and camxv2 are 2 different camera stack camxv1 support qcm6490, camxv2 support qcs9100, qcs8300 , qcs615 and new chipset on camxv2. so we thought of having v1 and v2 in recipe name and as suggested by @lumag we can install libs at ${libdir}/${SoC} |
Test run workflowTest jobs for commit f7ea0fd
|
Currently you provide only |
for qcs615 lib will be installed /usr/lib/qcs615 |
|
/
So, we logically have |
Yes, for every chipset libs are different and install at SOC specific paths |
So, no need for v1/v2/vN and we are settled with the names. |
we need to understand what could be the possible way to install path and recipe name.
|
| LICENSE = "LICENSE.qcom-2" | ||
| LIC_FILES_CHKSUM = "file://${UNPACKDIR}/usr/share/doc/${PN}/NO.LOGIN.BINARY.LICENSE.QTI.pdf;md5=7a5da794b857d786888bbf2b7b7529c8" | ||
|
|
||
| SRC_URI = "https://qartifactory-edge.qualcomm.com/artifactory/qsc_releases/software/chip/component/camx.qclinux.0.0/${PBT_BUILD_DATE}/prebuilt_yocto/${BPN}_${PV}_armv8-2a.tar.gz" |
There was a problem hiding this comment.
Okay... So, you did it again. Tarball changed, PV didn't. Could you please STOP doing that?
There was a problem hiding this comment.
Okay... So, you did it again. Tarball changed, PV didn't. Could you please STOP doing that?
For the base PR, we often see that the artifactory and SHA are updated even when there are no changes in the camxcommon recipe. However, if other recipes are modified, the SHA for all recipes also changes. Therefore, we need to update the SHA for all recipes whenever any changes occur. We were considering updating the version with every change once the base PR is merged. Would that be acceptable?
There was a problem hiding this comment.
There was no PR in my sentence. No recipes. Stop reusing tarball names. Ever. It breaks OE caching. It breaks other possible ways to cache things. It breaks my mind, when I download yet another tarball pointed out by the recipe's URL and then face several files with the same basename. If I or somebody else downloads the tarball, wraps it in deb, rpm, apk or any other package, processes something then reports you an error, how would you determine, which one of the 1.0.1 tarballs had an issue? The rule of thumb is very, very simple: new upload means new tag, new version, new tarball name. If the contents doesn't change, find the ways to create binary reproducible tarballs (yes, it is possible, there are flags for that). But there should never be two tarballs with the same name and with different checksums.
There was a problem hiding this comment.
Yeah, big NAK for tarball changes without any actual content change, it should at least be fully reproducible.
There was a problem hiding this comment.
There was no PR in my sentence. No recipes. Stop reusing tarball names. Ever. It breaks OE caching. It breaks other possible ways to cache things. It breaks my mind, when I download yet another tarball pointed out by the recipe's URL and then face several files with the same basename. If I or somebody else downloads the tarball, wraps it in deb, rpm, apk or any other package, processes something then reports you an error, how would you determine, which one of the 1.0.1 tarballs had an issue? The rule of thumb is very, very simple: new upload means new tag, new version, new tarball name. If the contents doesn't change, find the ways to create binary reproducible tarballs (yes, it is possible, there are flags for that). But there should never be two tarballs with the same name and with different checksums.
As each distribution brings their own GCC, GLIBC and other differences, can we use same binaries for ubuntu distributions?
There was a problem hiding this comment.
As each distribution brings their own GCC, GLIBC and other differences, can we use same binaries for ubuntu distributions?
It's called ABI compatibility. In 99.9 % of the cases the binary compiled with the older GCC against the older Glibc will work with the newer ones. So, a typical approach would be to select some balanced old baseline and test that the binaries built with it still work with the newest one.
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
Commit message, commit subject, package name, summary, description... Nothing changes, no improvememtns. Could you please fix those items for once?
For the commit subject. Does 'foo: add foo' sound good to you? I'd say, too many 'camxcommon' in one line.
Provide virtual packages if required.
Compatibility of recipes or of the packages? I'd prefer having three separate recipes: kodiak, lemans+monaco and talos.
Linarly, not exponentially. There is no exponential growth. |
can we use camxv1, camxv2 , camxv3 for 3 different recipe names or can you help with some suggestion. will check on virtual packages for (qcs9100 and qcs8300) |
We have tried to provide more descriptive commit messages. The discussion about the package name is still ongoing. I have added the summary and description, but could you clarify what exactly is expected for the summary and description? |
Of course not. What do v1, v2, v3 mean? Some virtual internal branches. So, no, it's not acceptable.
hint: |
Commit subject: what actually happens. `camxcommon: add libs common to foo, bar and baz'. Or 'used by foo, bar and baz'. Commit message: English text. Not a bullet list. Not a list of changes in the commit, we can read patches. Describe why it needs to be done and why it needs to be done this particular way. |
so what can be good way to name recipe. camx-kodiak, camx-talos , camx-lemans-monaco? |
if we need to use RPROVIDES , what should be recipe name ? |
camx-lemans_1.0.1.bb, |
|
As a side note I'll probably ask to use codenames for SoC subdirs. We have had enough troubles and confusion about qcs6490 vs qcs6490 vs sc7280, qrb5165 vs sm8250, etc. Using the codenames removes the topic as it points out that this CamX works for all kodiak instances, no matter if it is compute, mobile, IoT or something else. |
Is the expectation described below correct, or do I need to update the approach?
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.