Camera01 #4
Camera01 #4
Conversation
Device tree files in kernel source use C defines for meaningful names of some values. When device tree is compiled in kernel source, the C preprocessor is run first on dts files and then the dtc compiler. To be able to use in overlays values with the same meaningful values, run the C preprocessor first on the dts files. This requires also the kernel source as the definitions of the values are found there. When dtbo files are built it is now required to set KERNEL_PATH variable with kernel source path. Also use dtc from the kernel source path and remove no-avoid_unnecessary_addr_size option as it is not avilable in the dtc compiler from the latest lts kernel (4.14). Signed-off-by: Todor Tomov <[email protected]>
Update camera overlays for OV5645 and OV5640 on DB410c and DB820c. There is no need to keep separate scripts for different camera configs as the only difference is the overlay being applied. Add generic overlay-apply.sh script which can be used to apply all camera overlays. Signed-off-by: Todor Tomov <[email protected]>
| @@ -16,7 +16,12 @@ maintainer-clean : clean | |||
| $(RM) $(DTBS) | |||
|
|
|||
| %.dtbo : %.dts | |||
| dtc -Wno-avoid_default_addr_size -Wno-avoid_unnecessary_addr_size \ | |||
| ifndef KERNEL_PATH | |||
| $(error KERNEL_PATH is not set) | |||
There was a problem hiding this comment.
You don't need an ifdef for this. Its better to conditionally assign (this won't interfere with make all since the error doesn't get reported unless something tries to expand KERNEL_PATH):
KERNEL_PATH ?= $(error KERNEL_PATH is not set)
However...
C pre-processing looks sensible but we don't have a single kernel tree to point KERNEL_PATH at. I know dt-update is currently only populated with examples from Dragonboard family... but we don't want things to stay that way.
I think its relatively easily solved by an approach based on target specific variables such as:
DB410C_KERNEL_PATH ?= $(error DB410C_KERNEL_PATH is not set)
DB410C_DTBS = $(wildcard scripts/db410/overlays/*dts
db410c_dtbs : KERNEL_PATH = $(DB410_KERNEL_PATH)
HIKEY620_KERNEL_PATH ?= $(error HIKEY620_KERNEL_PATH is not set)
HIKEY620_DTBS = $(wildcard scripts/hikey620/overlays/*dts
hikey620_dtbs : KERNEL_PATH = $(HIKEY620_KERNEL_PATH)
dtbs : db410c_dtbs hikey620_dtbs
I don't mind if both DB410C and DB820C share a DRAGONBOARD_KERNEL_PATH (since you have a unified kernel) but I'd like to leave space for other boards to be added.
There was a problem hiding this comment.
Hi Daniel,
Thank you for review. I'll create a new pull request with these suggestions implemented.
| if [ "$?" -ne 0 ]; then | ||
| echo "Failed to enable D3 mezzanine, no camss node, upgrade your bootimg." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Thanks for the refactor. Its been on the TODO list for this code but de-prioritized until the PoC gains more attention (so thanks for your attention to).
I currenty collapse of the shell scripts removes compatibility checks (like this one) and makes it hard to add new ones. Users run old releases so I'd like to have the capacity not to break compatibility with release N-1 just because release N comes out and need a change of overlay. We'll never be perfect w.r.t. version skew but with feedback from the forum I think we can do OK.
Can you add some hook into apply-overlay.sh to hook into version checker code. Perhaps use basename/dirname and friends to generate the name of a shell script fragment (scripts/db410c/db410c-ov5645.sh ) derived from the .dtbo file and, if such a file exists then source it. If it is sourced between the environmental setup+command line parsing and the actual update of the DT then we should have enough info in the shell fragment to do any necessary version checks and apply any fix ups that might be needed[1].
Daniel.
[1]
Loic and I have discussed a trick to retrospectively add enough symbol information to the 18.01 .dtb files to allow overlays with phandles to apply but we haven't yet had a chance to prototype it.
There was a problem hiding this comment.
Hi Daniel,
Thank you for review.
I thought that we should encourage users to use the latest release, or at least not encourage them to use any previous. If anyone would prefer to use older release, can't we just add versioning in the overlay tree and refer the user to an earlier version? Keeping sets of overlays and scripts for older releases together with a set of overlays and scripts for latest release - doesn't sound very nice. What do you think?
There was a problem hiding this comment.
As it happens the check I picked up here is a fail-if-too-old not a fixup-if-too-old. It is important to fail as neatly as possible and with as much advise as possible to the user (even if its just "this overlay requires 18.01 or later").
I do agree that we should encourage users to adopt the latest release and I was careful to talk about N and N-1 in my previous post; not mentioning N-2 is deliberate. However there is a legitimate cross over time between support for N landing in dt-update and support for N-1 being withdrawn. Not least of these reasons is that it allows you to merge N compatibility code into dt-update before you make a release.
Finally "doesn't sound very nice" is a question about nice for who! Given how stable DT bindings tend to be once they have been upstreamed I think version checking code in dt-update will actually be a relatively rare thing (isn't there only one in the code you have cleaned up so far?). Contrasting this against having to fork a maintenance branch or create a tag for dt-update every time any one of the 96Boards has a release doesn't fill me with joy. I'd prefer conservative maintenance on the master branch with a small amount of backward compatibility (and an easy life when we write documentation).
To be clear... I have no intention of requiring someone adding a new overlay to test them on the N-1 release or even to do a version check. They can be added with no shell fragment and rely entirely on generic code. Likewise if there is a version check it is fine for the error emitted to mandate adopting release N. However I would like to have hooks in the shell scripts to permit compatibility checks so we can apply feedback from the forum directly to master and make it available to users without messing about with maintenance branches.
There was a problem hiding this comment.
Hi Daniel,
Maybe (probably) I don't understand completely your idea. Here is what I'm concerned about (using my understanding).
- version check. How will we do a version check from a script? We apply an overlay to a bootimage (or dtb file). I'm not aware of any versioning of the bootimages. What will we check? If we try to apply an overlay and decide which version is the bootimage on the result of whether the overlay applied or failed - for me this is not nice. This is not reliable. Any error (or even success) caused by factor which we have not foreseen could make this version check to give wrong result.
- currently the "apply-overlay.sh" script which I propose is just that - apply overlay. It doesn't know anything about cameras, mezzanines, etc. To add a hook which will run another script, which will 1) magically derive script name and path from a command line parameter which is something else (name of overlay file); 2) source that script, and if it succeeds - abort the execution of "apply-overlay.sh" and then report success for this whole operation - these sound to me as dirty hacks. It is counter-intuitive - doesn't do what the user would expect to happen, and it is error prone.
- adding the new scripts (and overlays), keeping the old ones, then adding more which will do version checking - this makes the tree hard to understand - which script does what and which one exactly does the user needs to use.
If we keep one version in the repository and tag each version, I'm not sure why we would like to make changes of previous tags. I might be wrong but how I imagine it is that we support the latest version. If anyone prefers to use an older version (means older release) and then needs to make custom changes on that - this will be their own effort.
There was a problem hiding this comment.
I think this discussion is why I had suggested at the beginning to keep the overlay in the kernel tree instead. I don't think we can commit to be forward/backward compatible, and I don't think we will be able to encode proper/enough versioning information.
I still think it would be simpler (especially in terms of support) to keep 'release specific' overlays in the kernel tree. They would be tagged/branch'd with the release. We can certainly keep DT overlays tools generic and in this repo, of course, and ensure that we don't break the tools.
If we have 'generic' overlays, like spidev, that are expected to be version independent, then maybe we can keep them here.
|
On Tue, Jul 03, 2018 at 01:55:31PM +0000, todortomov wrote:
- version check. How will we do a version check from a script? We
apply an overlay to a bootimage (or dtb file). I'm not aware of any
versioning of the bootimages. What will we check? If we try to apply
an overlay and decide which version is the bootimage on the result of
whether the overlay applied or failed - for me this is not nice. This
These comments is attached to a simple example of a compatibility check:
verify that camss@1b00000 exists and give a friendly warning otherwise
(admittedly the text of the warning is rather weak at present).
It is also an example of how a compatibility check *should* be. This is it
looks for the actual change in the DT it cares about, not at some global
version number that won't work for snapshots.
is not reliable. Any error (or even success) caused by factor which we
have not foreseen could make this version check to give wrong result.
Isn't exactly the same is true for the absence of version checking?
Where we how not foreseen how it could fail (or just maintained it
badly) then the user will get the wrong result.
- currently the "apply-overlay.sh" script which I propose is just that
apply overlay. It doesn't know anything about cameras, mezzanines,
etc. To add a hook which will run another script, which will 1)
magically derive script name and path from a command line parameter
which is something else (name of overlay file); 2) source that script,
and if it succeeds - abort the execution of "apply-overlay.sh" and
then report success for this whole operation
Don't quite get this.
The sourced shell script will normally either:
1. detect a problem, print an error and terminate the shell process
2. not detect a problem and return execution to apply-overlay.sh so the
overlay can be applied
- these sound to me as dirty hacks. It is counter-intuitive - doesn't
do what the user would expect to happen, and it is error prone.
I disagree. I do not believe a message telling the user what has gone
wrong is counter-intuitive for that user. Likewise I don't understand
why this is error prone.
- adding the new scripts (and overlays), keeping the old ones, then
adding more which will do version checking this makes the tree hard
to understand which script does what and which one exactly does the
user needs to use.
The point of the proposal is that there is only one entry point
(apply-overlay.sh). The user need not care about the shell script
fragments and contributors only need to care about fragments whose names
make it obvious that the impact the overlay they choose to edit.
If we keep one version in the repository and tag each version, I'm not
sure why we would like to make changes of previous tags. I might be
wrong but how I imagine it is that we support the latest version. If
anyone prefers to use an older version (means older release) and then
needs to make custom changes on that - this will be their own effort.
Currently the PoC supports only Dragonboard 410C and, with your changes,
820C. However there are currently 13 96Boards CE edition boards and
several more enterprise boards for which dt-update also makes sense.
Only two of them share a release schedule (DB410C and DB820C) so
maintaining a tag per-board per-release is impractical.
…
-- You are receiving this because you commented. Reply to this email
directly or view it on GitHub:
#4 (comment)
On Tue, Jul 03, 2018 at 01:55:31PM +0000, todortomov wrote:
todortomov commented on this pull request.
> -DBOOTIMG=`which dbootimg`
-DBOOTIMG=${DBOOTIMG:-${DT_ROOT}/tools/dbootimg/dbootimg}
-DTBTOOL=`which dtbtool`
-DTBTOOL=${DTBTOOL:-${DT_ROOT}/tools/dtbtool/dtbtool}
-
-if [ ! -f ${BOOTIMG} ]; then
- echo "Invalid bootimg: ${BOOTIMG}"
- exit 1
-fi
-
-# Check DTB already has camera subsystem node present
-${DBOOTIMG} ${BOOTIMG} -x dtb | ${DTBTOOL} -n ***@***.*** -p > /dev/null
-if [ "$?" -ne 0 ]; then
- echo "Failed to enable D3 mezzanine, no camss node, upgrade your bootimg."
- exit 1
-fi
Hi Daniel,
Maybe (probably) I don't understand completely your idea. Here is what I'm concerned about (using my understanding).
- version check. How will we do a version check from a script? We apply an overlay to a bootimage (or dtb file). I'm not aware of any versioning of the bootimages. What will we check? If we try to apply an overlay and decide which version is the bootimage on the result of whether the overlay applied or failed - for me this is not nice. This is not reliable. Any error (or even success) caused by factor which we have not foreseen could make this version check to give wrong result.
- currently the "apply-overlay.sh" script which I propose is just that - apply overlay. It doesn't know anything about cameras, mezzanines, etc. To add a hook which will run another script, which will 1) magically derive script name and path from a command line parameter which is something else (name of overlay file); 2) source that script, and if it succeeds - abort the execution of "apply-overlay.sh" and then report success for this whole operation - these sound to me as dirty hacks. It is counter-intuitive - doesn't do what the user would expect to happen, and it is error prone.
- adding the new scripts (and overlays), keeping the old ones, then adding more which will do version checking - this makes the tree hard to understand - which script does what and which one exactly does the user needs to use.
If we keep one version in the repository and tag each version, I'm not sure why we would like to make changes of previous tags. I might be wrong but how I imagine it is that we support the latest version. If anyone prefers to use an older version (means older release) and then needs to make custom changes on that - this will be their own effort.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
|
The question about whether to include overlay fragments in dt-update or in the QCom LT release is interesting. To be honest I think it also comes back to Todor's comment on being error prone. I think this depends on whether you view dt-update as something you release or whether it a knowledge base, more akin to executable documentation. Personally I view it more akin to documentation. In particular because like documentation it will bit rot in the same way as documentation does. When a new release for some board or other comes out documentation may become obsolete just as some of the more exotic overlays may stop working correctly. Critically it is impractical to view every problem with docs (or dt-update) as a failure of the test plan. It is better to design a process for quick turn round of fixes than it is to ensure they never happen. Nico proposal is a get it right first time approach. That's great for boards that are included in your release test plans. I'm reasonably happy to ensure tools like dt-update can handle this approach[1]. However I'm not sure the kernel is the right place for untested code (given the 3-6 month release process) so the wider need for a knowledge base for boards that are not in your test plan will likely still exist. Daniel. [1] Although I do not agree it is "easier to support" like this. I suspect it is easier for you to release but it makes support harder (for example we can't easily do a |
Here is a real world problem. I'd like to create a compatibility check for our future release which will not have any camera nodes in the device tree. How do I do that? Following the N and N-1 release rationale (where N = latest release) I have to distinguish between this last and the previous version. The way I understand it is to check that the camera_rear@3b is not present. If camera_rear@3b is present, this could mean that:
If camera_rear@3b is not present, this could mean that:
I don't see much value in these compatibility checks if they are so imperfect. In some cases they can help, in some cases they will fail and when they fail the user will be on their own to understand what happens. Sometimes the additional amount of scripts in the repo could make it harder for the user to understand what happens. Do you have any idea for a better check? |
|
On Fri, Jul 06, 2018 at 08:38:20AM -0700, todortomov wrote:
If ***@***.*** is present, this could mean that:
- the bootimage is from N - 1 release;
- the bootimage is from N release and an overlay is already applied.
What error do we report?
If we genuinely can't tell the difference I think its OK to include both
suggestions.
If ***@***.*** is not present, this could mean that:
- the bootimage is from N release - we apply the overlay correctly.
- the bootimage is from N-1 release and an overlay was applied which
changed the node to ***@***.*** - we try to apply the overlay and
if fails. The user will have to understand why (just like when there
is no compatibility check).
There is no hard requirement to pre-solve every case.
However if this situation appears a lot on the forum then we could look
at writing a more exotic compatibility checks (perhaps looking for
incompatible peer nodes from other overlays, etc). However initially I
think its OK just to try and apply the overlay.
I don't see much value in these compatibility checks if they are so
imperfect. In some cases they can help, in some cases they will fail
and when they fail the user will be on their own to understand what
happens. Sometimes the additional amount of scripts in the repo could
make it harder for the user to understand what happens.
Do you have any idea for a better check?
I'd *like* to get the scripts to make a backup of the original DT. I've
not got a fully completed design for this but I'm tempted to insert a
property into the active device tree giving the filename of the backup.
This allows easy rollbacks and also means the tools would notice if the
user used fastboot (or dd) to restore the not-overlayed boot.img.
AFAICT having backups would solve both of the above questions since then
we would know if the user had previously applied an overlay (at least if
they did so using dt-update).
However...
Overlay in 96Boards has a very long history of inaction whilst we wait
for the "right" approach (a.k.a. perfect as the enemy of good
enough). The reason we started looking at dt-update was to break
this deadlock with a very low tech approach (e.g. shell scripts in
userspace). It would be contrary to the purpose of dt-update to block
your patches until we have implemented a polished backup system!
Daniel.
… --
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#4 (comment)
|
Update camera overlays for use with kernel from https://git.linaro.org/people/todor.tomov/linux.git/log/?h=release/qcomlt-4.14-camera07