Re: [PATCH RESEND v5 03/25] drm/msm/dp: Add support for programming p1/p2/p3 register blocks

[Dmitry Baryshkov]

On Mon, Jun 29, 2026 at 10:14:24PM +0800, Yongxing Mou wrote:
> 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.

>  }
>  
>  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.

> +		}
>  	}
>  
>  	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?

>  	 */
> -	return readl_relaxed(panel->p0_base + offset);
> +	return readl_relaxed(panel->pixel_base + offset);
>  }
>  

-- 
With best wishes
Dmitry