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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Yongxing Mou <[email protected]>
> > > ---
> > > 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
