Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <aaf36e7a1be596f831ca2665375632aae9c8e350.camel@redhat.com>
Subject: 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.
From:   Lyude Paul <lyude@redhat.com>
To:     Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
Cc:     Koba Ko <koba.ko@canonical.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Anthony Wong <anthony.wong@canonical.com>
Date:   Wed, 09 Sep 2020 14:26:11 -0400
In-Reply-To: <20200909182010.GO6112@intel.com>
References: <20200827053053.11271-1-koba.ko@canonical.com>
         <CAJB-X+WPiUR8N5sLMS2UC-rPHj=TGJy2ezeBPjjeUXzbN5bfcg@mail.gmail.com>
         <b1dcaf433b7ebe2d0284693724f0e7d8a7fa4825.camel@redhat.com>
         <20200909182010.GO6112@intel.com>
Organization: Red Hat
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-1.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?
> 
> > 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