video: driver: use global UBWC config on KLM platforms#100
video: driver: use global UBWC config on KLM platforms#100venvenk wants to merge 1 commit intoqualcomm-linux:video.qclinux.mainfrom
Conversation
venvenk
commented
May 4, 2026
venvenk
commented
- Use kernel global UBWC config database for kodiak/lemans/monaco
- Fall back to existing per-platform UBWC defaults when global data is unavailable.
|
Hi @lumag, Implemented to pick UBWC values from central database for overlay driver, could you please help to review? |
|
|
||
| static int msm_vidc_kodiak_set_ubwc_config(void) | ||
| { | ||
| #if MSM_VIDC_HAS_QCOM_UBWC_HEADER |
There was a problem hiding this comment.
No spaghetti, thanks. Please put #if outside of the function body. Ideally you might want to provide a compat layer for the upstream functionality as a separate source file.
There was a problem hiding this comment.
Creased separate source file as suggested.
| ubwc_cfg = qcom_ubwc_config_get_data(); | ||
| if (IS_ERR(ubwc_cfg)) { | ||
| d_vpr_h("%s: global UBWC config not available, using local defaults\n", __func__); | ||
| return 0; |
There was a problem hiding this comment.
Nope, return the error to the caller.
There was a problem hiding this comment.
ubwc_cfg = qcom_ubwc_config_get_data();
if (IS_ERR(ubwc_cfg)) {
long rc = PTR_ERR(ubwc_cfg);
if (rc == -EINVAL) {
d_vpr_h("%s: no global UBWC config for this platform, using platform defaults\n",
__func__);
return 0;
}
d_vpr_e("%s: failed to get global UBWC config: %ld\n",
__func__, rc);
return rc;
}
Addressed return values in new patch set.
If some platform data is not available in central database, then fallback to static UBWC configs, so returning 0 if return value is "-EINVAL" (match not available in database).
There was a problem hiding this comment.
No. All data must be in the database. Otherwise the platform will have the GPU driver broken.
There was a problem hiding this comment.
As an example, consider the UBWC database for kodiak, lemans, and monaco.
The initial UBWC database patches were merged in Linux v6.17.
In that version, UBWC data was available for kodiak and lemans, but monaco data was not included.
Support for monaco was added later, starting from Linux v6.19.
If we return an error from qcom_ubwc_config_get_data() when the database entry is missing, the probe would fail on kernels where monaco support is not yet present.
To avoid breaking valid platforms on earlier kernel versions, the current implementation falls back to platform default UBWC configurations in this scenario.
Please let me if there is any alternate approach for this scenario.
There was a problem hiding this comment.
So, this fallback should be implemented only for monaco, providing a default implementation for that platform.
There was a problem hiding this comment.
I have taken Monaco as an example,
There are other platforms as well which got added in other kernel versions,
Monaco, Kaanapali -> added in v6.19
glymur -> added in v7.0
mahua, eliza -> added in v7.1
Like this, new platforms will get added in kernel stage by stage.
In this scenario, if these platforms are tried to compile with kernel v6.18 then fallback is necessary for all these platforms.
It would be difficult to maintain fallback mechanism for all these platforms and upcoming platforms with older kernels.
There was a problem hiding this comment.
There is no Iris DT entry for those platforms in 6.18, so the kernel needs to be patched anyway (in the qcom-6.18 tree) to let this driver to be bound at all. So, corresponding DB entries also can be backported to qcom-6.18.
| ubwc_config.bank_spreading = | ||
| msm_vidc_qcom_ubwc_bank_spread(ubwc_cfg); | ||
|
|
||
| ubwc_config_kodiak[0] = ubwc_config; |
There was a problem hiding this comment.
Other way around. Drop msm_vidc_ubwc_config_data and use qcom_ubwc_cfg_data inside the driver code.
There was a problem hiding this comment.
qcom_ubwc_cfg_data was merged from specific kernel version.
So, I am updating msm_vidc_ubwc_config_data from qcom_ubwc_cfg_data based on kernel version check.
There was a problem hiding this comment.
It should be done other way around. Drop msm_vidc_ubwc_config_data, provide compatibility entry for qcom_ubwc_cfg_data if it is not supported by the kernel. Once we drop support for older kernels, compat code can be removed too. With you approach, the compat code remains the main codepoint. Dropping it would be much harder. Also, with this approach we don't know if the in-kernel DB is complete or not.
| ubwc_config.bank_spreading = | ||
| msm_vidc_qcom_ubwc_bank_spread(ubwc_cfg); | ||
|
|
||
| ubwc_config_lemans[0] = ubwc_config; |
There was a problem hiding this comment.
Huh. C&P of the kodiak function? Why?
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
- Use kernel global UBWC config database for kodiak/lemans/monaco
No, it should not be limited to KLM. It should be used globally by the driver.
- Use kernel global UBWC config database for kodiak/lemans/monaco - Fall back to existing per-platform UBWC defaults when global data is unavailable. Signed-off-by: Venkata Siva Pavan KumarVenkatapatigari <venvenk@qti.qualcomm.com>
venvenk
force-pushed
the
video.qclinux.main
branch
from
c0fb9d3 to
8cb1da8
Compare
| int msm_vidc_update_ubwc_config(struct msm_vidc_core *core) | ||
| { | ||
| const struct qcom_ubwc_cfg_data *ubwc_cfg; | ||
| struct msm_vidc_ubwc_config_data *ubwc_config; |
There was a problem hiding this comment.
You definitely don't follow the reviews. Stop using msm_vidc_ubwc_config_data, and switch to qcom_ubwc_cfg_data. Provide necessary kludges (and definitions) if the struct is not available (because of the kernel being too old).
| struct msm_vidc_ubwc_config_data *ubwc_config; | ||
| u32 swizzle; | ||
|
|
||
| if (!core || !core->platform || !core->platform->data.ubwc_config) |
There was a problem hiding this comment.
Defensive coding. Why? Can core be NULL here?
2 participants
