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

[Yongxing Mou <yongxing.mou@oss.qualcomm.com>]

On 7/12/2026 7:23 PM, Dmitry Baryshkov wrote:
> On Mon, Jun 29, 2026 at 10:14:24PM +0800, Yongxing Mou wrote:
>> From: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> 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@quicinc.com>
>> Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
>> ---
>>   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.
> 
Sure. here we should check NULL pointer and also check pixel_clk[i] 
status. Will fix 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.
> 
Thanks, got it.
>> +		}
>>   	}
>>   
>>   	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?
> 
The existing comments no longer match what the code is actually doing. 
We can fix them in this patch. How about this?
	/*
	 * 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);
>>   }
>>   
>