drm: phytium: update phytium drm controller driver support to 6.6.0.4#1700
Open
xiaqian1486 wants to merge 5 commits into
Open
drm: phytium: update phytium drm controller driver support to 6.6.0.4#1700xiaqian1486 wants to merge 5 commits into
xiaqian1486 wants to merge 5 commits into
Conversation
For BMC mode, due to OpenBMC KVM need to read edid in phytium_connector_get_modes() function, other application scenarios, read edid in phytium_connector_detect() function. Mainline: NA Signed-off-by: Wang Hao <wanghao1851@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
Fix the issue where the source_max_lane_count parameter cannot be customized during driver installation. Mainline: NA Signed-off-by: WangHao <wanghao1851@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
Add pointer validity checks to the gamma setting function to prevent NULL pointer occurrence when running applications. Mainline: Open-Source Signed-off-by: Yu Da <yuda2152@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
When rmmod drivers, call the shutdown function to release resources and prevent dp and frame buffer leaks. Mainline: Open-Source Signed-off-by: Yu Da <yuda2152@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
There is an issue with DP when handling FIFO delay. When connecting a Dell monitor with a link state of 1.62G/4 lanes, modes of 1600x900 and 1024x768-75Hz can not display. Corrected timing must be configured on the DC to ensure that DP output timing meets the monitor's requirements. Mainline: Open-Source Signed-off-by: Yu Da <yuda2152@phytium.com.cn> Signed-off-by: Wang Yinfeng<wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
Reviewer's GuideUpdates the Phytium DRM DP, PCI, platform, CRTC, GEM, and register code to better support BMC EDID handling, correct lane-count-based timing, prevent NULL dereferences and memory issues, ensure proper shutdown on device removal, and adjust DC hsync timing based on FIFO and link configuration. Sequence diagram for BMC and non-BMC EDID handlingsequenceDiagram
participant DrmCore
participant Connector as drm_connector
participant DpDev as phytium_dp_device
participant AuxDDC as aux_ddc
Note over DrmCore,Connector: Hotplug long pulse handling
DrmCore->>Connector: phytium_dp_long_pulse(hpd_raw_state)
Connector->>DpDev: connector_to_dp_device()
alt !priv->info.bmc_mode
Connector->>Connector: phytium_dp_set_edid()
Connector->>DpDev: phytium_dp_unset_edid()
Connector->>AuxDDC: drm_get_edid(connector, &DpDev.aux.ddc)
AuxDDC-->>Connector: edid or NULL
alt edid == NULL
Connector-->>DrmCore: connector_status_disconnected
else edid valid
Connector->>Connector: drm_detect_monitor_audio(DpDev.detect_edid)
Connector-->>DrmCore: connector_status_connected
end
else priv->info.bmc_mode
Note over Connector,DpDev: EDID read happens later in get_modes
end
Note over DrmCore,Connector: Mode enumeration
DrmCore->>Connector: phytium_connector_get_modes()
Connector->>DpDev: connector_to_dp_device()
alt DpDev.is_edp
Connector->>Connector: edid = DpDev.edp_edid
else priv->info.bmc_mode
Connector->>AuxDDC: drm_get_edid(connector, &DpDev.aux.ddc)
AuxDDC-->>Connector: edid
else non-BMC DP
Connector->>Connector: edid = DpDev.detect_edid
end
alt edid && drm_edid_is_valid(edid)
Connector->>Connector: drm_connector_update_edid_property()
Connector->>Connector: drm_add_edid_modes()
alt priv->info.bmc_mode
Connector->>Connector: drm_detect_monitor_audio(edid)
end
else
Connector->>Connector: DpDev.has_audio = false
end
alt priv->info.bmc_mode
Connector->>Connector: kfree(edid)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @xiaqian1486. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In phytium_dp_modify_dc_hsync_time(), connector->state is dereferenced without a NULL check; please guard connector->state (and potentially crtc->state/mode) before use to avoid NULL pointer dereferences during enable paths or transient states.
- The new PCI and platform shutdown helpers assume dev and dev->dev_private are always valid; consider adding NULL checks so that remove/shutdown paths after partial probe failures or unexpected drvdata states do not crash.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In phytium_dp_modify_dc_hsync_time(), connector->state is dereferenced without a NULL check; please guard connector->state (and potentially crtc->state/mode) before use to avoid NULL pointer dereferences during enable paths or transient states.
- The new PCI and platform shutdown helpers assume dev and dev->dev_private are always valid; consider adding NULL checks so that remove/shutdown paths after partial probe failures or unexpected drvdata states do not crash.
## Individual Comments
### Comment 1
<location path="drivers/gpu/drm/phytium/phytium_dp.c" line_range="2249" />
<code_context>
+ return;
+ }
+
+ crtc = connector->state->crtc;
+ if (!crtc)
+ return;
</code_context>
<issue_to_address>
**issue (bug_risk):** Connector state is dereferenced without a NULL check, which can lead to crashes.
In `phytium_dp_modify_dc_hsync_time`, `connector->state` is used without checking for NULL. In hotplug, early init, or teardown paths this can legitimately be NULL and will oops the kernel. Add a guard, e.g.:
```c
if (!connector->state)
return;
crtc = connector->state->crtc;
```
before accessing `connector->state->crtc`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return; | ||
| } | ||
|
|
||
| crtc = connector->state->crtc; |
There was a problem hiding this comment.
issue (bug_risk): Connector state is dereferenced without a NULL check, which can lead to crashes.
In phytium_dp_modify_dc_hsync_time, connector->state is used without checking for NULL. In hotplug, early init, or teardown paths this can legitimately be NULL and will oops the kernel. Add a guard, e.g.:
if (!connector->state)
return;
crtc = connector->state->crtc;before accessing connector->state->crtc.
There was a problem hiding this comment.
Pull request overview
This PR updates the Phytium DRM DP controller driver with changes around EDID handling, lane-count selection, gamma safety checks, remove/shutdown cleanup, and DP/DC timing adjustment.
Changes:
- Adds cached external-DP EDID handling and BMC-mode EDID reads.
- Adds DP hsync timing adjustment and removes some hard-coded mode rejections.
- Adds shutdown calls on remove paths and guards memory-pool cleanup/gamma paths.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
drivers/gpu/drm/phytium/px210_dp.c |
Adds module-parameter override for PX210 source lane count. |
drivers/gpu/drm/phytium/phytium_reg.h |
Defines the default DP FIFO register value used by hsync adjustment. |
drivers/gpu/drm/phytium/phytium_platform.c |
Calls display shutdown during platform-device removal. |
drivers/gpu/drm/phytium/phytium_pci.c |
Calls display shutdown during PCI-device removal. |
drivers/gpu/drm/phytium/phytium_gem.c |
Adds NULL guards around memory-pool free/destroy calls. |
drivers/gpu/drm/phytium/phytium_dp.h |
Adds cached EDID storage and hsync adjustment constants. |
drivers/gpu/drm/phytium/phytium_dp.c |
Updates EDID handling, audio detection, hsync timing, and mode validation behavior. |
drivers/gpu/drm/phytium/phytium_crtc.c |
Adds defensive checks around gamma LUT programming. |
Comments suppressed due to low confidence (1)
drivers/gpu/drm/phytium/phytium_crtc.c:369
modeis the address ofstate->adjusted_mode, so this NULL check can never be true oncestateis non-NULL. If the goal is to avoid invalid timing crashes, the check needs to validate fields used below (for examplecrtc_clockbefore the division) rather than testing this pointer.
if (!mode)
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1796
to
+1797
| if (status == connector_status_disconnected) | ||
| goto out; |
Comment on lines
+1794
to
+1795
| if (!priv->info.bmc_mode) { | ||
| status = phytium_dp_set_edid(connector); |
Comment on lines
+1739
to
+1750
| phytium_dp->has_audio = false; | ||
| } | ||
|
|
||
| static enum drm_connector_status phytium_dp_set_edid(struct drm_connector *connector) | ||
| { | ||
| struct phytium_dp_device *phytium_dp = connector_to_dp_device(connector); | ||
|
|
||
| phytium_dp_unset_edid(connector); | ||
| phytium_dp->detect_edid = drm_get_edid(connector, &phytium_dp->aux.ddc); | ||
| if (!phytium_dp->detect_edid) | ||
| return connector_status_disconnected; | ||
|
|
Comment on lines
+73
to
+74
| if (priv->memory_pool) | ||
| gen_pool_destroy(priv->memory_pool); |
Comment on lines
+345
to
346
| phytium_pci_shutdown(pdev); | ||
| drm_dev_unregister(dev); |
Comment on lines
+331
to
+332
| if (priv->info.bmc_mode) | ||
| phytium_dp->has_audio = drm_detect_monitor_audio(edid); |
Comment on lines
+2244
to
+2247
| if (!connector) { | ||
| DRM_INFO("%s: connector is null\n", __func__); | ||
| return; | ||
| } |
| phytium_dp_hpd_irq_setup(dev, false); | ||
| cancel_work_sync(&priv->hotplug_work); | ||
| phytium_platform_shutdown(pdev); | ||
| drm_dev_unregister(dev); |
Comment on lines
+899
to
902
| if (source_max_lane_count != 4) | ||
| return source_max_lane_count; | ||
|
|
||
| return px210_dp_source_lane_count[phytium_dp->port]; |
Comment on lines
+351
to
+352
| if (!state) | ||
| return; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patches updates the support for phytium drm controller driver.
Summary by Sourcery
Update Phytium DRM DP controller to improve EDID handling, lane count configuration, gamma safety, cleanup on shutdown, and DC hsync timing.
Bug Fixes:
Enhancements: