Re: [Intel-gfx] [PATCH v6 1/1] drm/drm_mst: Use Extended Base Receiver Capability DPCD space

[Lyude Paul <lyude@redhat.com>]

Resend, since I hit enter too early on the first one :).

On Wed, 2021-04-28 at 16:44 -0400, Nikola Cornij wrote:
> [why]
> DP 1.4a spec madates that if DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is
> set, Extended Base Receiver Capability DPCD space must be used. Without
> doing that, the three DPCD values that differ will be wrong, leading to
> incorrect or limited functionality. MST link rate, for example, could
> have a lower value. Also, Synaptics quirk wouldn't work out well when
> Extended DPCD was not read, resulting in no DSC for such hubs.
> 
> [how]
> Modify MST topology manager to use the values from Extended DPCD where
> applicable.
> 
> To prevent regression on the sources that have a lower maximum link rate
> capability than MAX_LINK_RATE from Extended DPCD, have the drivers
> supply maximum lane count and rate at initialization time.
> 
> This also reverts 'commit 2dcab875e763 ("Revert "drm/dp_mst: Retrieve
> extended DPCD caps for topology manager"")', brining the change back to
> the original 'commit ad44c03208e4 ("drm/dp_mst: Retrieve extended DPCD
> caps for topology manager")'.
> 
> Signed-off-by: Nikola Cornij <nikola.cornij@amd.com>
> ---
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  5 +++
>  .../gpu/drm/amd/display/dc/core/dc_link_dp.c  | 18 +++++++++++
>  drivers/gpu/drm/amd/display/dc/dc_link.h      |  2 ++
>  drivers/gpu/drm/drm_dp_mst_topology.c         | 32 ++++++++++++-------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  6 +++-
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  3 +-
>  drivers/gpu/drm/radeon/radeon_dp_mst.c        |  8 +++++
>  include/drm/drm_dp_mst_helper.h               | 12 ++++++-
>  8 files changed, 71 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 997567f6f0ba..b7e01b6fb328 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -429,6 +429,8 @@ void amdgpu_dm_initialize_dp_connector(struct
> amdgpu_display_manager *dm,
>                                        struct amdgpu_dm_connector *aconnector,
>                                        int link_index)
>  {
> +       struct dc_link_settings max_link_enc_cap = {0};
> +
>         aconnector->dm_dp_aux.aux.name =
>                 kasprintf(GFP_KERNEL, "AMDGPU DM aux hw bus %d",
>                           link_index);
> @@ -443,6 +445,7 @@ void amdgpu_dm_initialize_dp_connector(struct
> amdgpu_display_manager *dm,
>         if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
>                 return;
>  
> +       dc_link_dp_get_max_link_enc_cap(aconnector->dc_link,
> &max_link_enc_cap);
>         aconnector->mst_mgr.cbs = &dm_mst_cbs;
>         drm_dp_mst_topology_mgr_init(
>                 &aconnector->mst_mgr,
> @@ -450,6 +453,8 @@ void amdgpu_dm_initialize_dp_connector(struct
> amdgpu_display_manager *dm,
>                 &aconnector->dm_dp_aux.aux,
>                 16,
>                 4,
> +               max_link_enc_cap.lane_count,
> +               max_link_enc_cap.link_rate,
>                 aconnector->connector_id);
>  
>         drm_connector_attach_dp_subconnector_property(&aconnector->base);
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> index 7d2e433c2275..6fe66b7ee53e 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> @@ -1894,6 +1894,24 @@ bool dc_link_dp_sync_lt_end(struct dc_link *link, bool
> link_down)
>         return true;
>  }
>  
> +bool dc_link_dp_get_max_link_enc_cap(const struct dc_link *link, struct
> dc_link_settings *max_link_enc_cap)
> +{
> +       if (!max_link_enc_cap) {
> +               DC_LOG_ERROR("%s: Could not return max link encoder caps",
> __func__);
> +               return false;
> +       }
> +
> +       if (link->link_enc->funcs->get_max_link_cap) {
> +               link->link_enc->funcs->get_max_link_cap(link->link_enc,
> max_link_enc_cap);
> +               return true;
> +       }
> +
> +       DC_LOG_ERROR("%s: Max link encoder caps unknown", __func__);
> +       max_link_enc_cap->lane_count = 1;
> +       max_link_enc_cap->link_rate = 6;
> +       return false;
> +}
> +
>  static struct dc_link_settings get_max_link_cap(struct dc_link *link)
>  {
>         struct dc_link_settings max_link_cap = {0};
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_link.h
> b/drivers/gpu/drm/amd/display/dc/dc_link.h
> index b0013e674864..cb6d0543d839 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_link.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_link.h
> @@ -346,6 +346,8 @@ bool dc_link_dp_set_test_pattern(
>         const unsigned char *p_custom_pattern,
>         unsigned int cust_pattern_size);
>  
> +bool dc_link_dp_get_max_link_enc_cap(const struct dc_link *link, struct
> dc_link_settings *max_link_enc_cap);
> +
>  void dc_link_enable_hpd_filter(struct dc_link *link, bool enable);
>  
>  bool dc_link_is_dp_sink_present(struct dc_link *link);
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5539a91b4031..b7869a1135bf 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3708,19 +3708,23 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr *mgr, bool ms
>         /* set the device into MST mode */
>         if (mst_state) {
>                 struct drm_dp_payload reset_pay;
> +               int lane_count;
> +               int link_rate;
>  
>                 WARN_ON(mgr->mst_primary);
>  
>                 /* get dpcd info */
> -               ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, mgr->dpcd,
> DP_RECEIVER_CAP_SIZE);
> -               if (ret != DP_RECEIVER_CAP_SIZE) {
> -                       drm_dbg_kms(mgr->dev, "failed to read DPCD\n");
> +               ret = drm_dp_read_dpcd_caps(mgr->aux, mgr->dpcd);
> +               if (ret < 0) {
> +                       drm_dbg_kms(mgr->dev, "%s: failed to read DPCD, ret
> %d\n",
> +                                   mgr->aux->name, ret);
>                         goto out_unlock;
>                 }
>  
> +               lane_count = min_t(int, mgr->dpcd[2] & DP_MAX_LANE_COUNT_MASK,
> mgr->max_lane_count);
> +               link_rate = min_t(int, mgr->dpcd[1], mgr->max_link_rate);
>                 mgr->pbn_div = drm_dp_get_vc_payload_bw(mgr,
> -
>                                                        drm_dp_bw_code_to_link_r
> ate(mgr->dpcd[1]),
> -                                                       mgr->dpcd[2] &
> DP_MAX_LANE_COUNT_MASK);
> +                                       drm_dp_bw_code_to_link_rate(link_rate)
> , lane_count);

^ so reformat this like I mentioned before:

mgr->pbn_div = drm_dp_get_vc_payload_bw(mgr, drm_dp_bw_code_to_link_rate(link_rate),
					lane_count);


                if (mgr->pbn_div == 0) {
                        ret = -EINVAL;
                        goto out_unlock;
>
@@ -5448,14 +5452,17 @@ EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
  * @aux: DP helper aux channel to talk to this device
  * @max_dpcd_transaction_bytes: hw specific DPCD transaction limit
  * @max_payloads: maximum number of payloads this GPU can source
+ * @max_lane_count: maximum number of lanes this GPU supports
+ * @max_link_rate: maximum link rate this GPU supports, units as in DPCD
  * @conn_base_id: the connector object ID the MST device is connected to.
  *
  * Return 0 for success, or negative error code on failure
  */
 int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
                                 struct drm_device *dev, struct drm_dp_aux
*aux,
-                                int max_dpcd_transaction_bytes,
-                                int max_payloads, int conn_base_id)
+                                int max_dpcd_transaction_bytes, int
max_payloads,
+                                int max_lane_count, int max_link_rate,
+                                int conn_base_id)
 {
        struct drm_dp_mst_topology_state *mst_state;
 
@@ -5490,6 +5497,8 @@ int drm_dp_mst_topology_mgr_init(struct
drm_dp_mst_topology_mgr *mgr,
        mgr->aux = aux;
        mgr->max_dpcd_transaction_bytes = max_dpcd_transaction_bytes;
        mgr->max_payloads = max_payloads;
+       mgr->max_lane_count = max_lane_count;
+       mgr->max_link_rate = max_link_rate;
        mgr->conn_base_id = conn_base_id;
        if (max_payloads + 1 > sizeof(mgr->payload_mask) * 8 ||
            max_payloads + 1 > sizeof(mgr->vcpi_mask) * 8)
@@ -5896,14 +5905,13 @@ struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct
drm_dp_mst_port *port)
        if (drm_dp_has_quirk(&desc, DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) &&
            port->mgr->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14 &&
            port->parent == port->mgr->mst_primary) {
-               u8 downstreamport;
+               u8 dpcd_ext[DP_RECEIVER_CAP_SIZE];
 
-               if (drm_dp_dpcd_read(&port->aux, DP_DOWNSTREAMPORT_PRESENT,
-                                    &downstreamport, 1) < 0)
+               if (drm_dp_read_dpcd_caps(port->mgr->aux, dpcd_ext) < 0)
                        return NULL;
 
-               if ((downstreamport & DP_DWN_STRM_PORT_PRESENT) &&
-                  ((downstreamport & DP_DWN_STRM_PORT_TYPE_MASK)
+               if ((dpcd_ext[DP_DOWNSTREAMPORT_PRESENT] &
DP_DWN_STRM_PORT_PRESENT) &&
+                   ((dpcd_ext[DP_DOWNSTREAMPORT_PRESENT] &
DP_DWN_STRM_PORT_TYPE_MASK)
                     != DP_DWN_STRM_PORT_TYPE_ANALOG))
                        return port->mgr->aux;
        }
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 860381d68d9d..a4245eb48ef4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -942,6 +942,7 @@ intel_dp_mst_encoder_init(struct intel_digital_port
*dig_port, int conn_base_id)
        struct intel_dp *intel_dp = &dig_port->dp;
        enum port port = dig_port->base.port;
        int ret;
+       int bios_max_link_rate;
 
        if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
                return 0;
@@ -956,8 +957,11 @@ intel_dp_mst_encoder_init(struct intel_digital_port
*dig_port, int conn_base_id)
 
        /* create encoders */
        intel_dp_create_fake_mst_encoders(dig_port);
+       bios_max_link_rate = intel_bios_dp_max_link_rate(&dig_port->base);
        ret = drm_dp_mst_topology_mgr_init(&intel_dp->mst_mgr, &i915->drm,
-                                          &intel_dp->aux, 16, 3,
conn_base_id);
+                                          &intel_dp->aux, 16, 3,
+                                          dig_port->max_lanes,
+                                          bios_max_link_rate / 27000,
conn_base_id);
        if (ret)
                return ret;
 
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 1c9c0cdf85db..e02f9d2d74eb 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1617,7 +1617,8 @@ nv50_mstm_new(struct nouveau_encoder *outp, struct
drm_dp_aux *aux, int aux_max,
        mstm->mgr.cbs = &nv50_mstm;
 
        ret = drm_dp_mst_topology_mgr_init(&mstm->mgr, dev, aux, aux_max,
-                                          max_payloads, conn_base_id);
+                                          max_payloads, outp->dcb-
>dpconf.link_nr,
+                                          outp->dcb->dpconf.link_bw,
conn_base_id);
        if (ret)
                return ret;
 
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 59cf1d288465..cf3be8b14676 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -629,13 +629,21 @@ int
 radeon_dp_mst_init(struct radeon_connector *radeon_connector)
 {
        struct drm_device *dev = radeon_connector->base.dev;
+       struct radeon_connector_atom_dig *dig_connector = radeon_connector-
>con_priv;

Unused variable:

drivers/gpu/drm/radeon/radeon_dp_mst.c: In function ‘radeon_dp_mst_init’:
drivers/gpu/drm/radeon/radeon_dp_mst.c:632:36: warning: unused variable
‘dig_connector’ [-Wunused-variable]
  632 |  struct radeon_connector_atom_dig *dig_connector = radeon_connector-
>con_priv;
      |                                    ^~~~~~~~~~~~~


+       int max_link_rate;
 
        if (!radeon_connector->ddc_bus->has_aux)
                return 0;
 
+       if (radeon_connector_is_dp12_capable(&radeon_connector->base))
+               max_link_rate = 0x14;
+       else
+               max_link_rate = 0x0a;
+
        radeon_connector->mst_mgr.cbs = &mst_cbs;
        return drm_dp_mst_topology_mgr_init(&radeon_connector->mst_mgr, dev,
                                            &radeon_connector->ddc_bus->aux,
16, 6,
+                                           4, max_link_rate,
                                            radeon_connector->base.base.id);
 }
 
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 20dc705642bd..b5b0bf37813b 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -593,6 +593,14 @@ struct drm_dp_mst_topology_mgr {
         * @max_payloads: maximum number of payloads the GPU can generate.
         */
        int max_payloads;
+       /**
+        * @max_lane_count: maximum number of lanes the GPU can drive.
+        */
+       int max_lane_count;
+       /**
+        * @max_link_rate: maximum link rate per lane GPU can output.
+        */
+       int max_link_rate;

Should probably make these (and the arguments that were added to
drm_dp_mst_topology_mgr_init()) u8, just to make it a little clearer that
these are DP codes

With the comments in here addressed:

Reviewed-by: Lyude Paul <lyude@redhat.com>

once you post the respin I can go push this to drm-misc-next

        /**
         * @conn_base_id: DRM connector ID this mgr is connected to. Only used
         * to build the MST connector path value.
@@ -765,7 +773,9 @@ struct drm_dp_mst_topology_mgr {
 int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
                                 struct drm_device *dev, struct drm_dp_aux
*aux,
                                 int max_dpcd_transaction_bytes,
-                                int max_payloads, int conn_base_id);
+                                int max_payloads,
+                                int max_lane_count, int max_link_rate,
+                                int conn_base_id);
 
 void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr);
 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx