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

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

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@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   | 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.
>> 1.panel come with encoder (after bridge remvoed), like that:
>> struct msm_dp_mst_encoder {
>> 	struct drm_encoder *enc;
>> 	int stream_id;
>> 	struct msm_dp_panel *dp_panel;
>> };
> 
> This sounds like a good idea. This way there is no additional resource
> management, etc.
> 
>> 2. During enable and disable, allocate and release dynamically.
>> would you prefer which one?
>>>>    	return rc;
>>>>    }
>