Re: [PATCH v4 15/39] drm/msm/dp: Add support for programming p1/p2/p3 register blocks

[Dmitry Baryshkov]

On Fri, May 22, 2026 at 03:51:57PM +0800, Yongxing Mou wrote:
> 
> 
> On 5/21/2026 8:20 PM, Dmitry Baryshkov wrote:
> > On Thu, May 21, 2026 at 07:50:30PM +0800, Yongxing Mou wrote:
> > > 
> > > 
> > > On 4/12/2026 2:07 AM, Dmitry Baryshkov wrote:
> > > > On Fri, Apr 10, 2026 at 05:33:50PM +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   | 89 ++++++++++++++++++++-----------------
> > > > >    drivers/gpu/drm/msm/dp/dp_panel.h   |  3 +-
> > > > >    3 files changed, 79 insertions(+), 53 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > index 7984a0f9e938..ff506064a3fa 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;
> > > > >    };
> > > > > @@ -561,7 +561,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);
> > > > > @@ -769,6 +769,7 @@ int msm_dp_display_set_stream_info(struct msm_dp *msm_dp_display,
> > > > >    	}
> > > > >    	panel->stream_id = stream_id;
> > > > > +	msm_dp_panel_set_pixel_base(panel, dp->pixel_base[stream_id]);
> > > > 
> > > > Hmmm.... Would it be better to set it up differently? Allocate one panel
> > > > per the stream from the beginning and then simply get the first
> > > > available panel when required? This would require some minimal resource
> > > > manager, but then we won't have to pass dummy register base to the panel
> > > > code. Or actually allocate a panel when it is required? Do we need a
> > > > panel before atomic_enable()?
> > > > 
> > > In this series, panel come with MST connectors, Because the connectors are
> > > dynamically assigned, we don’t know which connector corresponds to which
> > > stream, so there stream_id and pixel base address are dynamic.
> > 
> > I read this as 'streams are dynamically assigned'. Connectors are fixed
> > and created for each branch point / real physical connector. Streams are
> > assigned on the first-serve bases.
> > 
> Thanks for pointing that out — your statement here is accurate.
> > > we have 2 optionals here:
> > 
> > What is the runtime requirement for the panels? Are they required for
> > parsing of the resources or only for the setup of the actual screen?
> > 
> > If we have 5 monitors connected to a single DP controller (via the
> > complicated topology), how many msm_dp_panel instances do we need to
> > handle the case, present it to the user and still let it select only 4
> > of them for the video output?
> > 
> It seems that if we move the link-related resources from the panel into
> dp_link,
> struct msm_dp_panel {
> 	/* dpcd raw data */
> 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> 	u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
> 	struct msm_dp_link_info link_info;
> ...
> }
>  we would only need to assign the panel during atomic_enable /
> atomic_disable.
> This way, in the scenario you described, only the final four panels would be
> exposed to the user.
> In the current implementation, the panel contains link-related information
> that is required during the link training phase.

link training is a part of atomic_enable. So, what does your answer
mean?


-- 
With best wishes
Dmitry