drm: Add SMI drm graphic driver #972
Conversation
Signed-off-by: Teddy Wang <teddy.wang@siliconmotion.com>
|
Hi @teddywlq. 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. Instructions 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.
We failed to fetch the diff for pull request #972
You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.
|
/ok-to-test |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new DRM driver for SiliconMotion SM750/SM768/SM770 graphic chips, introducing comprehensive DDK768 (Display Driver Kit) support with display management, 2D graphics acceleration, and hardware cursor functionality.
Key Changes:
- Adds complete DDK768 driver framework with display, 2D graphics, clock, chip management, and cursor support
- Implements hardware abstraction layer for SiliconMotion SM768 graphics chips
- Provides VDIF (Video Display Information Format) support for monitor timing management
Reviewed Changes
Copilot reviewed 52 out of 190 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/gpu/drm/smidrm/ddk768/ddk768_display.c | Core display controller implementation with DPMS, panel power sequencing, and LVDS support |
| drivers/gpu/drm/smidrm/ddk768/ddk768_cursor.h | Header defining hardware cursor control interface |
| drivers/gpu/drm/smidrm/ddk768/ddk768_cursor.c | Hardware cursor implementation for position and state management |
| drivers/gpu/drm/smidrm/ddk768/ddk768_clock.h | PLL clock calculation and configuration interface |
| drivers/gpu/drm/smidrm/ddk768/ddk768_clock.c.bak | Backup of clock management implementation |
| drivers/gpu/drm/smidrm/ddk768/ddk768_clock.c | PLL clock calculation and frequency management |
| drivers/gpu/drm/smidrm/ddk768/ddk768_chip.h | Chip identification and initialization interface |
| drivers/gpu/drm/smidrm/ddk768/ddk768_chip.c | SM768 chip detection, memory size detection, and initialization |
| drivers/gpu/drm/smidrm/ddk768/ddk768_2d.h | 2D graphics engine interface with drawing operations |
| drivers/gpu/drm/smidrm/ddk768/ddk768_2d.c.bak | Backup of 2D graphics engine implementation |
| drivers/gpu/drm/smidrm/ddk768/ddk768_2d.c | 2D graphics acceleration with rectangle fill, blitting, and transparency |
| drivers/gpu/drm/smidrm/ddk768/ddk768.h | Main DDK768 header consolidating all module interfaces |
| drivers/gpu/drm/smidrm/ddk750/vdif.h | Video display timing format definitions for monitor support |
Comments suppressed due to low confidence (5)
drivers/gpu/drm/smidrm/ddk768/ddk768_display.c:231
- The variable name 'DC_OFFSET' is used here but 'CHANNEL_OFFSET' is used consistently throughout the rest of the file. This inconsistency should be resolved for better maintainability.
ulRegAddr = (dispControl == CHANNEL0_CTRL)? CURRENT_LINE : (CURRENT_LINE+DC_OFFSET);
drivers/gpu/drm/smidrm/ddk768/ddk768_display.c:439
- Inconsistent use of 'DC_OFFSET' instead of 'CHANNEL_OFFSET'. This should use 'CHANNEL_OFFSET' for consistency with the rest of the codebase.
ulMonitorDetectAddr = (dispControl == CHANNEL0_CTRL)? CRT_DETECT : (CRT_DETECT+DC_OFFSET);
drivers/gpu/drm/smidrm/ddk768/ddk768_display.c:586
- Function name 'DisableDoublePixel' does not follow the consistent naming convention used throughout the file. It should be named 'ddk768_disableDoublePixel' to match the pattern used by other functions.
void DisableDoublePixel(disp_control_t dispControl)
drivers/gpu/drm/smidrm/ddk768/ddk768_display.c:607
- Function name 'EnableDoublePixel' does not follow the consistent naming convention used throughout the file. It should be named 'ddk768_enableDoublePixel' to match the pattern used by other functions.
void EnableDoublePixel(disp_control_t dispControl)
drivers/gpu/drm/smidrm/ddk768/ddk768_cursor.h:43
- The header guard comment 'CURSOR_H' does not match the actual header guard macro 'DDK768_CURSOR_H' defined at the top of the file.
#endif /* _CURSOR_H_ */
| * | ||
| * Output: | ||
| * 0 - Success | ||
| * 1 - Fail |
There was a problem hiding this comment.
The comment describes return values but the actual function returns 'long' type and only returns 0. Consider updating the comment to accurately reflect the function's behavior or implement proper error handling that can return the documented failure value.
Copilot uses AI. Check for mistakes.
| unsigned long base, /* Base Address */ | ||
| unsigned long color1, /* Cursor color 1 in RGB 5:6:5 format */ | ||
| unsigned long color2, /* Cursor color 2 in RGB 5:6:5 format */ | ||
| unsigned long color3 /* Cursor color 3 in RGB 5:6:5 format */ |
There was a problem hiding this comment.
The function parameter 'color3' is documented and declared but never used in the function implementation. This suggests either missing functionality or an unnecessary parameter.
Copilot uses AI. Check for mistakes.
| */ | ||
| void ddk768_enableCursor( | ||
| disp_control_t dispControl, /* Display control (CHANNEL0_CTRL or CHANNEL1_CTRL) */ | ||
| unsigned long mode /* Cursor type - 00 disable, 0x01 mask cursor, 0x02 mono, 0x03 alpha cursor */ |
There was a problem hiding this comment.
The function parameter is named 'mode' but the comment describes it as 'Cursor type'. The function signature comment should use 'enable' to match the function name 'enableCursor', or the parameter should be renamed to 'cursorType' for clarity.
| unsigned long mode /* Cursor type - 00 disable, 0x01 mask cursor, 0x02 mono, 0x03 alpha cursor */ | |
| unsigned long cursorType /* Cursor type - 00 disable, 0x01 mask cursor, 0x02 mono, 0x03 alpha cursor */ |
Copilot uses AI. Check for mistakes.
| * A local function to calculate the output frequency of a given PLL structure. | ||
| * | ||
| */ | ||
| static unsigned long ddk768_calcPLL(pll_value_t *pPLL) |
There was a problem hiding this comment.
The function modifies the input frequency field of the PLL structure (dividing by 1000 and then restoring it). This side effect could cause issues if the function is called multiple times or if the caller expects the input structure to remain unchanged. Consider using a local copy of the input frequency instead.
Copilot uses AI. Check for mistakes.
| /* | ||
| * This function returns frame buffer memory size in Byte units. | ||
| */ | ||
| unsigned long ddk768_getFrameBufSize() |
There was a problem hiding this comment.
The function modifies hardware registers (DDR_CONTROL) as a side effect of determining frame buffer size. This mixing of configuration and querying functionality could lead to unexpected behavior. Consider separating the configuration logic from the size detection logic.
Copilot uses AI. Check for mistakes.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Avenger-285714 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Avenger-285714
merged commit af5e37b
into
deepin-community:linux-6.6.y
Add SM750/SM768/SM770 DRM driver for SiliconMotopm graphic chips.