Skip to content
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

Camera01 #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Camera01 #4

wants to merge 2 commits into from

Conversation

todortomov
Copy link

@todortomov
Makefile: Run C preprocessor on dts files
beae935 
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 <todor.tomov@linaro.org>
@todortomov
Update camera overlays
cc93c7b 
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 <todor.tomov@linaro.org>
daniel-thompson
daniel-thompson reviewed Jun 28, 2018
View reviewed changes
Makefile
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Daniel,

Thank you for review. I'll create a new pull request with these suggestions implemented.

daniel-thompson
daniel-thompson reviewed Jun 28, 2018
View reviewed changes
scripts/db410c/enable-d3-mezzanine.sh
if [ "$?" -ne 0 ]; then
echo "Failed to enable D3 mezzanine, no camss node, upgrade your bootimg."
exit 1
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

@daniel-thompson
Copy link
Collaborator

daniel-thompson commented Jul 6, 2018 via email

@daniel-thompson
Copy link
Collaborator

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 git log on the D3 engineering board overlay to get clues about why a different camera board is failing).

@todortomov
Copy link
Author

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.

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:

  • 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 camera_rear@3b 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 camera_rear@3c - we try to apply the overlay and if fails. The user will have to understand why (just like when there is no compatibility check).

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?

@daniel-thompson
Copy link
Collaborator

daniel-thompson commented Jul 6, 2018 via email

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

@ndechesne ndechesne ndechesne left review comments

@daniel-thompson daniel-thompson daniel-thompson left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@todortomov @daniel-thompson @ndechesne