Re: [PATCH RESEND v5 13/25] drm/msm/dp: replace power_on with active_stream_cnt for dp_display

[Yongxing Mou]

On 7/13/2026 2:51 AM, Dmitry Baryshkov wrote:
> On Mon, Jun 29, 2026 at 10:14:34PM +0800, Yongxing Mou wrote:
> > From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >
> > For DP MST, the link clock and power domain resources stay on until
> > both streams have been disabled OR we receive hotplug. Introduce an
> > active_stream_cnt to track the number of active streams and necessary
> > state handling. Replace the power_on variable with active_stream_cnt
> > as power_on boolean works only for a single stream.
>
> Last two phrases are redundat. Drop the "Introduce..." one.
>
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > Signed-off-by: Yongxing Mou <yongxing.mou@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_audio.c   |  2 +-
> >   drivers/gpu/drm/msm/dp/dp_display.c | 28 +++++++++++++++++-----------
> >   drivers/gpu/drm/msm/dp/dp_display.h |  2 +-
> >   3 files changed, 19 insertions(+), 13 deletions(-)
> >
> > @@ -670,14 +670,10 @@ static int msm_dp_display_enable(struct msm_dp_display_private *dp,
> >   	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> >   
> >   	drm_dbg_dp(dp->drm_dev, "sink_count=%d\n", dp->link->sink_count);
> > -	if (msm_dp_display->power_on) {
> > -		drm_dbg_dp(dp->drm_dev, "Link already setup, return\n");
> > -		return 0;
> > -	}
> >   
> >   	rc = msm_dp_ctrl_on_stream(dp->ctrl, msm_dp_panel, msm_dp_display->mst_active);
> > -	if (!rc)
> > -		msm_dp_display->power_on = true;
> > +
> > +	msm_dp_display->active_stream_cnt++;
>
> Current power_on prevents from calling msm_dp_ctrl_on_stream() several
> times. How is it prevented after the change?
I think we don't need to prevent it. It call drom .atomic_enable() and 
it should not call twice in SST case.
> >   
> >   	return rc;
> >   }
> > @@ -726,14 +722,14 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp,
> >   {
> >   	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> >   
> > -	if (!msm_dp_display->power_on)
> > +	if (!msm_dp_display->active_stream_cnt)
> >   		return 0;
> >   
> >   	msm_dp_panel_disable_vsc_sdp(msm_dp_panel);
> >   
> >   	msm_dp_ctrl_off_pixel_clk(dp->ctrl, msm_dp_panel->stream_id);
> >   
> > -	msm_dp_display->power_on = false;
> > +	msm_dp_display->active_stream_cnt--;
> >   
> >   	drm_dbg_dp(dp->drm_dev, "sink count: %d\n", dp->link->sink_count);
> >   	return 0;
> > @@ -850,10 +846,10 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
> >   	 * if we are reading registers we need the link clocks to be on
> >   	 * however till DP cable is connected this will not happen as we
> >   	 * do not know the resolution to power up with. Hence check the
> > -	 * power_on status before dumping DP registers to avoid crash due
> > +	 * active_stream_cnt status before dumping DP registers to avoid crash due
> >   	 * to unclocked access
> >   	 */
> > -	if (!dp->power_on)
> > +	if (!dp->active_stream_cnt)
> >   		return;
> >   
> >   	msm_disp_snapshot_add_block(disp_state, msm_dp_display->ahb_len,
> > @@ -1535,6 +1531,11 @@ void msm_dp_display_disable_helper(struct msm_dp *msm_dp_display,
> >   
> >   	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> >   
> > +	if (!msm_dp_display->active_stream_cnt) {
> > +		drm_dbg_dp(dp->drm_dev, "no active streams\n");
> > +		return;
> > +	}
>
> If you get here, there was at least one active stream, wasn't it?
Emm yes..
> > +
> >   	msm_dp_ctrl_push_vcpf(dp->ctrl, msm_dp_panel);
> >   	msm_dp_ctrl_mst_stream_channel_slot_setup(dp->ctrl);
> >   	msm_dp_ctrl_mst_send_act(dp->ctrl, msm_dp_panel);
> > @@ -1555,6 +1556,11 @@ void msm_dp_display_unprepare(struct msm_dp *msm_dp_display)
> >   
> >   	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> >   
> > +	if (msm_dp_display->active_stream_cnt) {
> > +		drm_dbg_dp(dp->drm_dev, "stream still active, return\n");
> > +		return;
>
> Why?
Here, unprepare is only called when the last active stream is also being 
disabled.
> General comment. You are not just replacing power_on with
> active_stream_cnt. You are also introducing sequence changes, but it is
> not covered in the cover letter. It's not clear, why those changes are
> legitimage or why they are required.
Got it. Will modify the commit message.. thanks~
> > +	}
> > +
> >   	/* dongle is still connected but sinks are disconnected */
> >   	if (dp->link->sink_count == 0)
> >   		msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 55ffa22bb233..75dc40261723 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -18,7 +18,7 @@ struct msm_dp {
> >   	struct drm_bridge *next_bridge;
> >   	struct drm_bridge *bridge;
> >   	bool audio_enabled;
> > -	bool power_on;
> > +	u32 active_stream_cnt;
> >   	bool mst_active;
> >   	unsigned int connector_type;
> >   	bool is_edp;
> >
> > --
> > 2.43.0