Re: [PATCH] drm/dp: For MST hub, Get max_link_rate&max_lane from extended rx capability field if EXTENDED_RECEIVER_CAPABILITY_FILED_PRESENT is set.

[Lyude Paul <lyude@redhat.com>]

On Wed, 2020-09-09 at 21:36 +0300, Ville Syrjälä wrote:
> On Wed, Sep 09, 2020 at 02:26:11PM -0400, Lyude Paul wrote:
> > On Wed, 2020-09-09 at 21:20 +0300, Ville Syrjälä wrote:
> > > On Wed, Sep 09, 2020 at 12:33:09PM -0400, Lyude Paul wrote:
> > > > We just added a new helper for DPCD retrieval to drm_dp_helper.c
> > > > (which
> > > > also
> > > > handles grabbing the extended receiver caps), we should probably make
> > > > use
> > > > of
> > > > it here 
> > > 
> > > Someone should really rework this whole thing so that the driver can
> > > pass in the link rate+lane count when setting up the link. There's no
> > > reason to assume that the source device capabilities match or exceed
> > > the MST device caps. It would also allow the driver the dynamically
> > > adjust these in response to link failures.
> > 
> > I'm a bit confused, I also think we should pass the link rate+lane count
> > in
> > (especially since it'll be very helpful for when we start optimizing PBN
> > handling in regards to bw constraints), but I'm not sure what you mean by
> > "There's no reason to assume that the source device capabilities match or
> > exceed the MST device caps", aren't we doing the opposite here by checking
> > the
> > MST device caps?
> 
> We assume only the MST device caps matter. Which is fine if the source
> device caps are equal or higher. But if the source device isn't as
> capable then we're going to calculate the MST things based on link bw
> we can not actually achieve. End result is that we potentially try to
> push too much data over the link.
> 
> I'm not really sure what actually happens if we just miscompute these
> things but don't actually oversubscribe the link. Maybe the remote
> end gets confused when we tell it some bogus VC parameters? Maybe it
> doesn't care? Dunno.

Ah-I understand what you mean. From my understanding I think MST devices parse
some of the bandwidth information (since a lot of hubs seem to have a
different full_pbn based on the source caps from what I've seen). But yes-we
probably should also start processing all of the relevant DPCD caps on the
source device's side instead of just trusting MST. I'll add this to my todo
list

> 
> > > > On Wed, 2020-09-09 at 14:31 +0800, Koba Ko wrote:
> > > > > On Thu, Aug 27, 2020 at 1:30 PM Koba Ko <koba.ko@canonical.com>
> > > > > wrote:
> > > > > > Currently, DRM get the capability of the mst hub only from
> > > > > > DP_DPCD_REV
> > > > > > and
> > > > > > get the slower speed even the mst hub can run in the faster speed.
> > > > > > 
> > > > > > As per DP-1.3, First check DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT.
> > > > > > If DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is 1, read the
> > > > > > DP_DP13_DPCD_REV
> > > > > > to
> > > > > > get the faster capability.
> > > > > > If DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is 0, read DP_DPCD_REV.
> > > > > > 
> > > > > > Signed-off-by: Koba Ko <koba.ko@canonical.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++++++-
> > > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > index 67dd72ea200e..3b84c6801281 100644
> > > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > @@ -3497,6 +3497,8 @@ static int drm_dp_get_vc_payload_bw(u8
> > > > > > dp_link_bw,
> > > > > > u8  dp_link_count)
> > > > > >  int drm_dp_mst_topology_mgr_set_mst(struct
> > > > > > drm_dp_mst_topology_mgr
> > > > > > *mgr,
> > > > > > bool mst_state)
> > > > > >  {
> > > > > >         int ret = 0;
> > > > > > +       u8 dpcd_ext = 0;
> > > > > > +       unsigned int dpcd_offset = 0;
> > > > > >         struct drm_dp_mst_branch *mstb = NULL;
> > > > > > 
> > > > > >         mutex_lock(&mgr->payload_lock);
> > > > > > @@ -3510,9 +3512,15 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> > > > > > drm_dp_mst_topology_mgr *mgr, bool ms
> > > > > >                 struct drm_dp_payload reset_pay;
> > > > > > 
> > > > > >                 WARN_ON(mgr->mst_primary);
> > > > > > +               drm_dp_dpcd_read(mgr->aux,
> > > > > > +                                DP_TRAINING_AUX_RD_INTERVAL,
> > > > > > +                                &dpcd_ext, sizeof(dpcd_ext));
> > > > > > +
> > > > > > +               dpcd_offset =
> > > > > > +                       ((dpcd_ext &
> > > > > > DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT) ?  DP_DP13_DPCD_REV :
> > > > > > DP_DPCD_REV);
> > > > > > 
> > > > > >                 /* get dpcd info */
> > > > > > -               ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, mgr-
> > > > > > > dpcd,
> > > > > > DP_RECEIVER_CAP_SIZE);
> > > > > > +               ret = drm_dp_dpcd_read(mgr->aux, dpcd_offset, mgr-
> > > > > > > dpcd,
> > > > > > DP_RECEIVER_CAP_SIZE);
> > > > > >                 if (ret != DP_RECEIVER_CAP_SIZE) {
> > > > > >                         DRM_DEBUG_KMS("failed to read DPCD\n");
> > > > > >                         goto out_unlock;
> > > > > > --
> > > > > > 2.25.1
> > > > > > 
> > > > > Add Lyude Paul
> > > > > 
> > > > -- 
> > > > Cheers,
> > > > 	Lyude Paul (she/her)
> > > > 	Software Engineer at Red Hat
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > -- 
> > Cheers,
> > 	Lyude Paul (she/her)
> > 	Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Software Engineer at Red Hat