Re: [PATCH v3 14/38] drm/msm/dp: Add support for programming p1/p2/p3 register blocks

[Dmitry Baryshkov]

On Mon, Mar 30, 2026 at 06:27:41PM +0800, Yongxing Mou wrote:
> 
> 
> On 8/26/2025 1:59 AM, Dmitry Baryshkov wrote:
> > On Mon, Aug 25, 2025 at 10:16:00PM +0800, Yongxing Mou wrote:
> > > From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > 
> > > QCS8300 supports 4-stream MST. This patch adds support for the additional
> > > pixel register blocks (p1, p2, p3), enabling multi-stream configurations.
> > > 
> > > To reduce code duplication, introduce helper functions msm_dp_read_pn and
> > > msm_dp_write_pn. All pixel clocks (PCLKs) 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 | 39 +++++++++++++--------
> > >   drivers/gpu/drm/msm/dp/dp_panel.c   | 68 ++++++++++++++++++-------------------
> > >   2 files changed, 59 insertions(+), 48 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 3422f18bdec71a99407edfe943d31957d0e8847a..935a0c57a928b15a1e9a6f1fab2576b7b09acb8e 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -84,8 +84,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;
> > >   };
> > > @@ -619,7 +619,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);
> > 
> > Why do we need to pass pixel base here? Shouldn't it be pixel_base[P0]?
> > 
> In the current implementation, dp->panel holds the full
> pixel_base[DP_STREAM_MAX] instead of just the pixel base corresponding to
> itself, so this likely needs to be revisited.
> 
> > >   	if (IS_ERR(dp->panel)) {
> > >   		rc = PTR_ERR(dp->panel);
> > >   		DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
> > > @@ -937,8 +937,8 @@ 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");
> > 
> > This should add all blocks used on this platform.
> > 
> Let mark to be done here. Since a helper function is required here to check
> whether the pixel clocks for stream 1/2/3 are really enabled.

I don't understand the comment.

> > >   }
> > >   void msm_dp_display_set_psr(struct msm_dp *msm_dp_display, bool enter)
> > > @@ -1181,12 +1181,13 @@ static void __iomem *msm_dp_ioremap(struct platform_device *pdev, int idx, size_
> > >   #define DP_DEFAULT_AUX_SIZE	0x0200
> > >   #define DP_DEFAULT_LINK_OFFSET	0x0400
> > >   #define DP_DEFAULT_LINK_SIZE	0x0C00
> > > -#define DP_DEFAULT_P0_OFFSET	0x1000
> > > -#define DP_DEFAULT_P0_SIZE	0x0400
> > > +#define DP_DEFAULT_PIXEL_OFFSET	0x1000
> > > +#define DP_DEFAULT_PIXEL_SIZE	0x0400
> > 
> > No need to touch this. It's only required for legacy bindings.
> > 
> Thanks, will fix it.
> > >   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))
> > > @@ -1206,7 +1207,7 @@ static int msm_dp_display_get_io(struct msm_dp_display_private *display)
> > >   		 * reg is specified, so fill in the sub-region offsets and
> > >   		 * lengths based on this single region.
> > >   		 */
> > > -		if (display->ahb_len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) {
> > > +		if (display->ahb_len < DP_DEFAULT_PIXEL_OFFSET + DP_DEFAULT_PIXEL_SIZE) {
> > >   			DRM_ERROR("legacy memory region not large enough\n");
> > >   			return -EINVAL;
> > >   		}
> > > @@ -1216,8 +1217,10 @@ 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;
> > > +		for (i = DP_STREAM_0; i < display->max_stream; i++)
> > > +			display->pixel_base[i] = display->ahb_base +
> > > +						 (i+1) * DP_DEFAULT_PIXEL_OFFSET;
> > > +		display->pixel_len = DP_DEFAULT_PIXEL_SIZE;
> > >   		return 0;
> > >   	}
> > > @@ -1228,10 +1231,18 @@ 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 < display->max_stream; 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]);
> > >   	}
> > >   	return 0;
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> > > index eae125972934bb2fb3b716dc47ae71cd0421bd1a..e8c1cf0c7dab7217b8bfe7ecd586af33d7547ca9 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> > > @@ -26,7 +26,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[DP_STREAM_MAX];
> > >   	bool panel_on;
> > >   };
> > > @@ -45,24 +45,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,
> > > +static inline void msm_dp_write_pn(struct msm_dp_panel_private *panel,
> > >   			       u32 offset, u32 data)
> > 
> > Is it really multiplexed on the panel level? I'd assume that each panel
> > is connected to only one stream instance... If that's not the case, such
> > details must be explained in the commit message.
> > 
> Yes, each panel is connected to only one stream instance. We just use
> **pixel_base + stream_id to determine which pixel block the request should
> be sent to. Just like your first comment, do you mean that the panel should
> only carry the pixel block address corresponding to itself, rather than the
> 4 blocks?

Yes.


-- 
With best wishes
Dmitry