[PATCH 31/45] drm/msm/dp: add dp_mst_drm to manage DP MST bridge operations
Dmitry Baryshkov
dmitry.baryshkov at oss.qualcomm.com
Wed May 28 18:02:22 UTC 2025
On Tue, May 27, 2025 at 06:29:49PM +0800, Yongxing Mou wrote:
>
>
> On 2024/12/6 18:12, Dmitry Baryshkov wrote:
> > On Thu, Dec 05, 2024 at 08:32:02PM -0800, Abhinav Kumar wrote:
> > > Add a new file dp_mst_drm to manage the DP MST bridge operations
> > > similar to the dp_drm file which manages the SST bridge operations.
> > > Each MST encoder creates one bridge and each bridge is bound to its
> > > own dp_panel abstraction to manage the operations of its pipeline.
> > >
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> > > ---
> > > drivers/gpu/drm/msm/Makefile | 3 +-
> > > drivers/gpu/drm/msm/dp/dp_display.h | 2 +
> > > drivers/gpu/drm/msm/dp/dp_mst_drm.c | 490 ++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/msm/dp/dp_mst_drm.h | 102 ++++++++
> > > 4 files changed, 596 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > > index f274d9430cc311405f890074c1466ffe2ec45ac9..b1e01b3123d9afc4818f059c5d4e7ca70dca3754 100644
> > > --- a/drivers/gpu/drm/msm/Makefile
> > > +++ b/drivers/gpu/drm/msm/Makefile
> > > @@ -142,7 +142,8 @@ msm-display-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
> > > dp/dp_link.o \
> > > dp/dp_panel.o \
> > > dp/dp_audio.o \
> > > - dp/dp_utils.o
> > > + dp/dp_utils.o \
> > > + dp/dp_mst_drm.o
> > > msm-display-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> > > index 6ab14e969bce0fd07b3a550bae17e99652479232..a5d4893f689c6afbbe622c9b7dfa98d23d754831 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > > @@ -7,6 +7,7 @@
> > > #define _DP_DISPLAY_H_
> > > #include "dp_panel.h"
> > > +#include "dp_mst_drm.h"
> > > #include <sound/hdmi-codec.h>
> > > #include "disp/msm_disp_snapshot.h"
> > > @@ -26,6 +27,7 @@ struct msm_dp {
> > > bool is_edp;
> > > bool internal_hpd;
> > > + struct msm_dp_mst *msm_dp_mst;
> > > hdmi_codec_plugged_cb plugged_cb;
> > > struct msm_dp_audio *msm_dp_audio;
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..e66bd1e565aeb4da3d636eb5f4aa75504d60fd40
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> > > @@ -0,0 +1,490 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > > + * Copyright (c) 2018-2021, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +/*
> > > + * Copyright © 2014 Red Hat.
> > > + *
> > > + * Permission to use, copy, modify, distribute, and sell this software and its
> > > + * documentation for any purpose is hereby granted without fee, provided that
> > > + * the above copyright notice appear in all copies and that both that copyright
> > > + * notice and this permission notice appear in supporting documentation, and
> > > + * that the name of the copyright holders not be used in advertising or
> > > + * publicity pertaining to distribution of the software without specific,
> > > + * written prior permission. The copyright holders make no representations
> > > + * about the suitability of this software for any purpose. It is provided "as
> > > + * is" without express or implied warranty.
> > > + *
> > > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > > + * OF THIS SOFTWARE.
> > > + */
> > > +
> > > +#include "dp_mst_drm.h"
> > > +
> > > +static struct drm_private_state *msm_dp_mst_duplicate_bridge_state(struct drm_private_obj *obj)
> > > +{
> > > + struct msm_dp_mst_bridge_state *state;
> > > +
> > > + state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> > > + if (!state)
> > > + return NULL;
> > > +
> > > + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
> > > +
> > > + return &state->base;
> > > +}
> > > +
> > > +static void msm_dp_mst_destroy_bridge_state(struct drm_private_obj *obj,
> > > + struct drm_private_state *state)
> > > +{
> > > + struct msm_dp_mst_bridge_state *priv_state =
> > > + to_msm_dp_mst_bridge_priv_state(state);
> > > +
> > > + kfree(priv_state);
> > > +}
> > > +
> > > +static const struct drm_private_state_funcs msm_dp_mst_bridge_state_funcs = {
> > > + .atomic_duplicate_state = msm_dp_mst_duplicate_bridge_state,
> > > + .atomic_destroy_state = msm_dp_mst_destroy_bridge_state,
> > > +};
> > > +
> > > +/**
> > > + * dp_mst_find_vcpi_slots() - Find VCPI slots for this PBN value
> > > + * @mgr: manager to use
> > > + * @pbn: payload bandwidth to convert into slots.
> > > + *
> > > + * Calculate the number of VCPI slots that will be required for the given PBN
> > > + * value.
> > > + *
> > > + * RETURNS:
> > > + * The total slots required for this port, or error.
> > > + */
> > > +static int msm_dp_mst_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, int pbn)
> > > +{
> > > + int num_slots;
> > > + struct drm_dp_mst_topology_state *state;
> > > +
> > > + state = to_drm_dp_mst_topology_state(mgr->base.state);
> > > + num_slots = DIV_ROUND_UP(pbn, dfixed_trunc(state->pbn_div));
> >
> > drm_dp_atomic_find_time_slots() uses slightly different maths here, with
> > the different precision. Can we use the data that is set by that function
> > instead (payload->time_slots)?
> >
> Note that drm_dp_atomic_find_time_slots all call in atomic_check func, not
> in other place.So can we call this func in atomic_pre_enable? Also, amg
> driver also have similar usage pattern.
Well, granted that this function can return ENOSPC, it is an error to
call it in atomic_pre_enable(). Nothing in atomic_pre_enable() /
atomic_enable() is allowed to fail.
I think this answers my question: drm_dp_atomic_find_time_slots()
should be called from atomic_check(), this function must be dropped.
> > > +
> > > + /* max. time slots - one slot for MTP header */
> > > + if (num_slots > 63)
> > > + return -ENOSPC;
> > > + return num_slots;
> > > +}
> > > +
> > > +static void _msm_dp_mst_update_timeslots(struct msm_dp_mst *mst,
> > > + struct msm_dp_mst_bridge *mst_bridge,
> > > + struct drm_dp_mst_port *port)
> > > +{
> > > + int i;
> > > + struct msm_dp_mst_bridge *msm_dp_bridge;
> > > + struct drm_dp_mst_topology_state *mst_state;
> > > + struct drm_dp_mst_atomic_payload *payload;
> > > + int prev_start = 0;
> > > + int prev_slots = 0;
> > > +
> > > + mst_state = to_drm_dp_mst_topology_state(mst->mst_mgr.base.state);
> > > + payload = drm_atomic_get_mst_payload_state(mst_state, port);
> > > +
> > > + if (!payload) {
> > > + DRM_ERROR("mst bridge [%d] update_timeslots failed, null payload\n",
> > > + mst_bridge->id);
> > > + return;
> > > + }
> > > +
> > > + for (i = 0; i < mst->max_streams; i++) {
> > > + msm_dp_bridge = &mst->mst_bridge[i];
> > > + if (mst_bridge == msm_dp_bridge) {
> > > + /*
> > > + * When a payload was removed make sure to move any payloads after it
> > > + * to the left so all payloads are aligned to the left.
> > > + */
> >
> > Please don't. drm_dp_remove_payload_part2() should take care of that for
> > us. What is the reason for caching the data if we have to manually
> > handle the cache?
> >
> MST framework is managing the port's bandwidth, but we have a bridhge for
> each stream, so we need to keep track of the payload allcation status for
> each bridge, how much we used and how much we left. So maybe they are manage
> two different part.
Well, still no. MST topology master should handle all payload
allocation. If you need any data, enhance its API instead of duplicating
its functionality.
> > > + if (payload->vc_start_slot < 0) {
> > > + // cache the payload
> > > + prev_start = msm_dp_bridge->start_slot;
> > > + prev_slots = msm_dp_bridge->num_slots;
> > > + msm_dp_bridge->pbn = 0;
> > > + msm_dp_bridge->start_slot = 1;
> > > + msm_dp_bridge->num_slots = 0;
> > > + msm_dp_bridge->vcpi = 0;
> > > + } else { //add payload
> > > + msm_dp_bridge->pbn = payload->pbn;
> > > + msm_dp_bridge->start_slot = payload->vc_start_slot;
> > > + msm_dp_bridge->num_slots = payload->time_slots;
> > > + msm_dp_bridge->vcpi = payload->vcpi;
> > > + }
> > > + }
> > > + }
> > > +
> > > + // Now commit all the updated payloads
> > > + for (i = 0; i < mst->max_streams; i++) {
> > > + msm_dp_bridge = &mst->mst_bridge[i];
> > > +
> > > + //Shift payloads to the left if there was a removed payload.
> > > + if (payload->vc_start_slot < 0 && msm_dp_bridge->start_slot > prev_start)
> > > + msm_dp_bridge->start_slot -= prev_slots;
> > > +
> > > + msm_dp_display_set_stream_info(mst->msm_dp, msm_dp_bridge->msm_dp_panel,
> > > + msm_dp_bridge->id, msm_dp_bridge->start_slot,
> > > + msm_dp_bridge->num_slots,
> > > + msm_dp_bridge->pbn, msm_dp_bridge->vcpi);
> > > + drm_dbg_dp(mst->msm_dp->drm_dev,
> > > + "conn:%d vcpi:%d start_slot:%d num_slots:%d, pbn:%d\n",
> > > + DP_MST_CONN_ID(msm_dp_bridge), msm_dp_bridge->vcpi,
> > > + msm_dp_bridge->start_slot,
> > > + msm_dp_bridge->num_slots, msm_dp_bridge->pbn);
> > > + }
> > > +}
> > > +
[...]
> > > + struct msm_dp_panel *msm_dp_panel;
> > > +
> > > + int vcpi;
> > > + int pbn;
> > > + int num_slots;
> > > + int start_slot;
> >
> > Which of the fields (including in_use) are long-lived and which are a
> > part of the current state? Can we move all state ones to bridge's state?
> >
> in_use only used in bridge_init, so it is long-lived. Looking at it together
> with the next patch, only num_slots changes during atomic_check, so it is in
> bridge_state. pbn/vcpi/start_slots only change during bridge enable/disable,
> so they are placed in the bridge.
Let's look how it will look after refactoring. I'd still push all
changing fields to state. It make a lot of things much easier.
> > > +};
> > > +
> > > +struct msm_dp_mst_bridge_state {
> > > + struct drm_private_state base;
> > > + struct drm_connector *connector;
> > > + struct msm_dp_panel *msm_dp_panel;
> > > + int num_slots;
> > > +};
> > > +
> > > +struct msm_dp_mst {
> > > + bool mst_initialized;
> > > + struct drm_dp_mst_topology_mgr mst_mgr;
> > > + struct msm_dp_mst_bridge *mst_bridge;
> > > + struct msm_dp *msm_dp;
> > > + bool mst_session_hpd_state;
> > > + u32 max_streams;
> > > +};
> > > +
> > > +struct msm_dp_mst_connector {
> > > + struct drm_connector connector;
> > > + struct drm_dp_mst_port *mst_port;
> > > + struct msm_dp *msm_dp;
> > > + struct msm_dp_panel *dp_panel;
> > > +};
> > > +
> > > +#define to_msm_dp_mst_bridge(x) container_of((x), struct msm_dp_mst_bridge, base)
> > > +#define to_msm_dp_mst_bridge_priv(x) \
> > > + container_of((x), struct msm_dp_mst_bridge, obj)
> > > +#define to_msm_dp_mst_bridge_priv_state(x) \
> > > + container_of((x), struct msm_dp_mst_bridge_state, base)
> > > +#define to_msm_dp_mst_bridge_state(x) \
> > > + to_msm_dp_mst_bridge_priv_state((x)->obj.state)
> > > +#define to_msm_dp_mst_connector(x) \
> > > + container_of((x), struct msm_dp_mst_connector, connector)
> > > +int msm_dp_mst_drm_bridge_init(struct msm_dp *dp, struct drm_encoder *encoder);
> > > +
> > > +#endif /* _DP_MST_DRM_H_ */
> > >
> > > --
> > > 2.34.1
> > >
> >
>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list