Skip to content

thermald: install default Qualcomm thermal configuration - #2695

Closed
Priyansh Jain (priyjain1) wants to merge 1 commit into
qualcomm-linux:masterfrom
priyjain1:thermald-qcom-config
Closed

thermald: install default Qualcomm thermal configuration#2695
Priyansh Jain (priyjain1) wants to merge 1 commit into
qualcomm-linux:masterfrom
priyjain1:thermald-qcom-config

Conversation

@priyjain1

Copy link
Copy Markdown

Install thermal-conf.xml as the default thermald
configuration file for Qualcomm platforms.

Install thermal-conf.xml as the default thermald
configuration file for Qualcomm platforms.

Signed-off-by: Priyansh Jain <priyansh.jain@oss.qualcomm.com>

@lumag Dmitry Baryshkov (lumag) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please see

The commit message (the body) must:

Not to mention that your editor is misconfigured, wrapping commit messages too early.

Temperatures are in milli-degrees Celsius (e.g., 95000 = 95°C).
-->

<!--

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, you are adding a config file just to add a huuge comment? I don't think it's a good idea. Use documentation instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should make the end-user do the least amount of work possible, so we can do a few of the following things:

  • Generate configs for all SoCs supported by meta-qcom, add then to meta-qcom
  • Ship a generator script on-device (which no external dependencies, so most likely bash) that runs on first boot

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So, you are adding a config file just to add a huuge comment? I don't think it's a good idea. Use documentation instead.

The intention is not to add the file solely for the comment. The plan is to keep the configuration effectively empty for now and use it as the placeholder/default thermald configuration for Qualcomm platforms. Once the thermal mitigation rules are tuned and validated on supported SoCs, those rules will be added to this XML.
Regarding the documentation, thermald already has comprehensive documentation and examples upstream, so I do not see a need for separate meta-qcom documentation. The example rule in the comment was added only as a reference for future contributors who may add thermald policies for other Qualcomm chipsets.
If you feel that the example comment is unnecessary or too verbose, I am happy to remove it and keep the XML minimal until actual platform-specific rules are added.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We should make the end-user do the least amount of work possible, so we can do a few of the following things:

  • Generate configs for all SoCs supported by meta-qcom, add then to meta-qcom
  • Ship a generator script on-device (which no external dependencies, so most likely bash) that runs on first boot

I agree that we should minimize the amount of work required from end users.
In our case, the intention is to use a single thermal-conf.xml across Qualcomm platforms. I've already added Qualcomm SoC detection logic in thermald itself (PR (under review ): intel/thermal_daemon#584) , so the policy can adapt based on the detected platform rather than requiring a separate XML per SoC.
The current XML is primarily a placeholder to establish the integration path. As we tune and validate thermal mitigation policies, the corresponding rules will be added incrementally.
Also, installing thermal-conf.xml from meta-qcom would only provide a default configuration. Customers can still override it. Let me know your thoughts on this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well. Please don't commit placeholders. Once you have actual rules, please commit them here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need for a placeholder, add once we have something that brings value here.

@priyjain1

Copy link
Copy Markdown
Author

Please see

The commit message (the body) must:

Not to mention that your editor is misconfigured, wrapping commit messages too early.

Thanks for pointing the commit-text wrapping issue. I'll rework the commit message to follow the expected format and rewrap the body appropriately before respinning the patch.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants