Re: [PATCH RESEND v5 03/25] drm/msm/dp: Add support for programming p1/p2/p3 register blocks
From: Yongxing Mou
Date: Fri Aug 21 2026 - 05:36:53 EST
On 7/12/2026 7:23 PM, Dmitry Baryshkov wrote:
On Mon, Jun 29, 2026 at 10:14:24PM +0800, Yongxing Mou wrote:Sure. here we should check NULL pointer and also check pixel_clk[i] status. Will fix it.
From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
Add support for additional pixel register blocks (p1, p2, p3) to enable
4‑stream MST pixel clocks. Introduce the helper functions msm_dp_read_pn
and msm_dp_write_pn for pixel register programming. All pixel clocks
share the same register layout but use different base addresses.
Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
Signed-off-by: Yongxing Mou <yongxing.mou@xxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/msm/dp/dp_display.c | 40 +++++++++++++-----
drivers/gpu/drm/msm/dp/dp_panel.c | 82 ++++++++++++++++++-------------------
drivers/gpu/drm/msm/dp/dp_panel.h | 2 +-
3 files changed, 71 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 9cd243411e44..74f481a18164 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -85,8 +85,8 @@ struct msm_dp_display_private {
void __iomem *link_base;
size_t link_len;
- void __iomem *p0_base;
- size_t p0_len;
+ void __iomem *pixel_base[DP_STREAM_MAX];
+ size_t pixel_len;
int max_stream;
};
@@ -564,7 +564,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
goto error_link;
}
- dp->panel = msm_dp_panel_get(dev, dp->aux, dp->link, dp->link_base, dp->p0_base);
+ dp->panel = msm_dp_panel_get(dev, dp->aux, dp->link, dp->link_base, dp->pixel_base[0]);
if (IS_ERR(dp->panel)) {
rc = PTR_ERR(dp->panel);
DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
@@ -850,8 +850,14 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
msm_dp_display->aux_base, "dp_aux");
msm_disp_snapshot_add_block(disp_state, msm_dp_display->link_len,
msm_dp_display->link_base, "dp_link");
- msm_disp_snapshot_add_block(disp_state, msm_dp_display->p0_len,
- msm_dp_display->p0_base, "dp_p0");
+ msm_disp_snapshot_add_block(disp_state, msm_dp_display->pixel_len,
+ msm_dp_display->pixel_base[0], "dp_p0");
+ msm_disp_snapshot_add_block(disp_state, msm_dp_display->pixel_len,
+ msm_dp_display->pixel_base[1], "dp_p1");
+ msm_disp_snapshot_add_block(disp_state, msm_dp_display->pixel_len,
+ msm_dp_display->pixel_base[2], "dp_p2");
+ msm_disp_snapshot_add_block(disp_state, msm_dp_display->pixel_len,
+ msm_dp_display->pixel_base[3], "dp_p3");
It should be:
for int i = 0; i < DP_STREAM_MAX; i++)
Also, you've just added a NULL pointer exception in the crash handler.
Check for the address being non-zero before adding it to the snapshots.
Thanks, got it.}
void msm_dp_display_set_psr(struct msm_dp *msm_dp_display, bool enter)
@@ -1131,6 +1137,7 @@ static void __iomem *msm_dp_ioremap(struct platform_device *pdev, int idx, size_
static int msm_dp_display_get_io(struct msm_dp_display_private *display)
{
struct platform_device *pdev = display->msm_dp_display.pdev;
+ int i;
display->ahb_base = msm_dp_ioremap(pdev, 0, &display->ahb_len);
if (IS_ERR(display->ahb_base))
@@ -1160,8 +1167,8 @@ static int msm_dp_display_get_io(struct msm_dp_display_private *display)
display->aux_len = DP_DEFAULT_AUX_SIZE;
display->link_base = display->ahb_base + DP_DEFAULT_LINK_OFFSET;
display->link_len = DP_DEFAULT_LINK_SIZE;
- display->p0_base = display->ahb_base + DP_DEFAULT_P0_OFFSET;
- display->p0_len = DP_DEFAULT_P0_SIZE;
+ display->pixel_base[0] = display->ahb_base + DP_DEFAULT_P0_OFFSET;
+ display->pixel_len = DP_DEFAULT_P0_SIZE;
return 0;
}
@@ -1172,10 +1179,21 @@ static int msm_dp_display_get_io(struct msm_dp_display_private *display)
return PTR_ERR(display->link_base);
}
- display->p0_base = msm_dp_ioremap(pdev, 3, &display->p0_len);
- if (IS_ERR(display->p0_base)) {
- DRM_ERROR("unable to remap p0 region: %pe\n", display->p0_base);
- return PTR_ERR(display->p0_base);
+ display->pixel_base[0] = msm_dp_ioremap(pdev, 3, &display->pixel_len);
+ if (IS_ERR(display->pixel_base[0])) {
+ DRM_ERROR("unable to remap p0 region: %pe\n", display->pixel_base[0]);
+ return PTR_ERR(display->pixel_base[0]);
+ }
+
+ for (i = DP_STREAM_1; i < DP_STREAM_MAX; i++) {
+ /* pixels clk reg index start from 3*/
+ display->pixel_base[i] = msm_dp_ioremap(pdev, i + 3, &display->pixel_len);
+ if (IS_ERR(display->pixel_base[i])) {
+ DRM_DEBUG_DP("unable to remap p%d region: %pe\n", i,
+ display->pixel_base[i]);
+ display->pixel_base[i] = NULL;
+ break;
Here we should differentiate between the address being not present in
DT (which should be ignored) and any other errors.
The existing comments no longer match what the code is actually doing. We can fix them in this patch. How about this?+ }
}
return 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 745ee6976897..238920c45261 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -25,7 +25,7 @@ struct msm_dp_panel_private {
struct drm_dp_aux *aux;
struct msm_dp_link *link;
void __iomem *link_base;
- void __iomem *p0_base;
+ void __iomem *pixel_base;
bool panel_on;
};
@@ -44,24 +44,24 @@ static inline void msm_dp_write_link(struct msm_dp_panel_private *panel,
writel(data, panel->link_base + offset);
}
-static inline void msm_dp_write_p0(struct msm_dp_panel_private *panel,
- u32 offset, u32 data)
+static inline void msm_dp_write_pn(struct msm_dp_panel_private *panel,
+ u32 offset, u32 data)
{
/*
* To make sure interface reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, panel->p0_base + offset);
+ writel(data, panel->pixel_base + offset);
}
-static inline u32 msm_dp_read_p0(struct msm_dp_panel_private *panel,
- u32 offset)
+static inline u32 msm_dp_read_pn(struct msm_dp_panel_private *panel,
+ u32 offset)
{
/*
* To make sure interface reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
Hmm, so the comment talks about writel(_relaxed), but the code is readl.
Is the comment wrong? Or is it not applcable and we should be using
readl() here?
/*
* Only reads a configuration register: no DMA or memory ordering is
* required, so readl_relaxed() is sufficient.
*/
*/
- return readl_relaxed(panel->p0_base + offset);
+ return readl_relaxed(panel->pixel_base + offset);
}