drm: Update MST First Link Slot Information Based on Encoding Format

Commit Message

Bhawanpreet Lakha Oct. 12, 2021, 9:58 p.m. UTC

8b/10b encoding format requires to reserve the first slot for
recording metadata. Real data transmission starts from the second slot,
with a total of available 63 slots available.

In 128b/132b encoding format, metadata is transmitted separately
in LLCP packet before MTP. Real data transmission starts from
the first slot, with a total of 64 slots available.

v2:
* Remove get_mst_link_encoding_cap
* Move total/start slots to mst_state, and copy it to mst_mgr in
atomic_check

Signed-off-by: Fangzhi Zuo <[e-mail]>
Signed-off-by: Bhawanpreet Lakha <[e-mail]>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
 drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
 include/drm/drm_dp_mst_helper.h               | 13 +++++++
 3 files changed, 69 insertions(+), 7 deletions(-)

Comments

Bhawanpreet Lakha Oct. 12, 2021, 10:02 p.m. UTC | #1

[AMD Official Use Only]


Hi Lyude,

Jerry is busy these few weeks so I will be taking a look at this. I added the start/total slots to the mst_state and am updating them in atomic_check.

Also ignore the V2 "* Remove get_mst_link_encoding_cap" I got a bit lost in trying to figure out the patch layout that was sent before.


Thanks,
Bhawan

Lyude Paul Oct. 12, 2021, 10:09 p.m. UTC | #2

No problem, thanks for the patches! Will take a look at this tomorrow

On Tue, 2021-10-12 at 22:02 +0000, Lakha, Bhawanpreet wrote:
> [AMD Official Use Only]
> 
> Hi Lyude,
> 
> Jerry is busy these few weeks so I will be taking a look at this. I added
> the start/total slots to the mst_state and am updating them in atomic_check.
> 
> Also ignore the V2 "* Remove get_mst_link_encoding_cap" I got a bit lost in
> trying to figure out the patch layout that was sent before.
> 
> 
> Thanks,
> Bhawan
> 
> From: Bhawanpreet Lakha <[e-mail]>
> Sent: October 12, 2021 5:58 PM
> To: Zuo, Jerry <[e-mail]>; [e-mail] <dri-
> [e-mail]>; [e-mail] <[e-mail]>
> Cc: Wentland, Harry <[e-mail]>; Lin, Wayne
> <[e-mail]>; Kazlauskas, Nicholas <[e-mail]>;
> Lakha, Bhawanpreet <[e-mail]>
> Subject: [PATCH] drm: Update MST First Link Slot Information Based on
> Encoding Format 
> 8b/10b encoding format requires to reserve the first slot for
> recording metadata. Real data transmission starts from the second slot,
> with a total of available 63 slots available.
> 
> In 128b/132b encoding format, metadata is transmitted separately
> in LLCP packet before MTP. Real data transmission starts from
> the first slot, with a total of 64 slots available.
> 
> v2:
> * Remove get_mst_link_encoding_cap
> * Move total/start slots to mst_state, and copy it to mst_mgr in
> atomic_check
> 
> Signed-off-by: Fangzhi Zuo <[e-mail]>
> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>  drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
>  include/drm/drm_dp_mst_helper.h               | 13 +++++++
>  3 files changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5020f2d36fe1..4ad50eb0091a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
>  #if defined(CONFIG_DRM_AMD_DC_DCN)
>          struct dsc_mst_fairness_vars vars[MAX_PIPES];
>  #endif
> +       struct drm_dp_mst_topology_state *mst_state;
> +       struct drm_dp_mst_topology_mgr *mgr;
>  
>          trace_amdgpu_dm_atomic_check_begin(state);
>  
> @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
>                  lock_and_validation_needed = true;
>          }
>  
> +#if defined(CONFIG_DRM_AMD_DC_DCN)
> +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
> +               struct amdgpu_dm_connector *aconnector;
> +               struct drm_connector *connector;
> +               struct drm_connector_list_iter iter;
> +               u8 link_coding_cap;
> +
> +               if (!mgr->mst_state )
> +                       continue;
> +
> +               drm_connector_list_iter_begin(dev, &iter);
> +               drm_for_each_connector_iter(connector, &iter) {
> +                       int id = connector->index;
> +
> +                       if (id == mst_state->mgr->conn_base_id) {
> +                               aconnector =
> to_amdgpu_dm_connector(connector);
> +                               link_coding_cap =
> dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
> +                               drm_dp_mst_update_coding_cap(mst_state,
> link_coding_cap);
> +
> +                               break;
> +                       }
> +               }
> +               drm_connector_list_iter_end(&iter);
> +
> +       }
> +#endif
>          /**
>           * Streams and planes are reset when there are changes that affect
>           * bandwidth. Anything that affects bandwidth needs to go through
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ad0795afc21c..fb5c47c4cb2e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
> drm_dp_mst_topology_mgr *mgr)
>          struct drm_dp_payload req_payload;
>          struct drm_dp_mst_port *port;
>          int i, j;
> -       int cur_slots = 1;
> +       int cur_slots = mgr->start_slot;
>          bool skip;
>  
>          mutex_lock(&mgr->payload_lock);
> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
> drm_dp_mst_topology_mgr *mgr,
>          num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>  
>          /* max. time slots - one slot for MTP header */
> -       if (num_slots > 63)
> +       if (num_slots > mgr->total_avail_slots)
>                  return -ENOSPC;
>          return num_slots;
>  }
> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>          int ret;
>  
>          /* max. time slots - one slot for MTP header */
> -       if (slots > 63)
> +       if (slots > mgr->total_avail_slots)
>                  return -ENOSPC;
>  
>          vcpi->pbn = pbn;
> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
> drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>  
> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> *mst_state, uint8_t link_coding_cap)
> +{
> +       if (link_coding_cap == DP_CAP_ANSI_128B132B) {
> +               mst_state->total_avail_slots = 64;
> +               mst_state->start_slot = 0;
> +       }
> +
> +       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
> +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ?
> "128b/132b":"8b/10b", mst_state->mgr);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
> +
>  /**
>   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>   * @mgr: manager for this port
> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>  
>          ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>          if (ret) {
> -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63
> ret=%d\n",
> -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d
> ret=%d\n",
> +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
> >total_avail_slots, ret);
>                  drm_dp_mst_topology_put_port(port);
>                  goto out;
>          }
> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> drm_dp_mst_topology_mgr *mgr,
>                                           struct drm_dp_mst_topology_state
> *mst_state)
>  {
>          struct drm_dp_vcpi_allocation *vcpi;
> -       int avail_slots = 63, payload_count = 0;
> +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
>  
>          list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>                  /* Releasing VCPI is always OK-even if the port is gone */
> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> drm_dp_mst_topology_mgr *mgr,
>                  }
>          }
>          drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d
> used=%d\n",
> -                      mgr, mst_state, avail_slots, 63 - avail_slots);
> +                      mgr, mst_state, avail_slots, mgr->total_avail_slots -
> avail_slots);
>  
>          return 0;
>  }
> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state
> *state)
>                          break;
>  
>                  mutex_lock(&mgr->lock);
> +
> +               mgr->start_slot = mst_state->start_slot;
> +               mgr->total_avail_slots = mst_state->total_avail_slots;
> +
>                  ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> >mst_primary,
>                                                              mst_state);
>                  mutex_unlock(&mgr->lock);
> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
>          if (!mgr->proposed_vcpis)
>                  return -ENOMEM;
>          set_bit(0, &mgr->payload_mask);
> +       mgr->total_avail_slots = 63;
> +       mgr->start_slot = 1;
>  
>          mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>          if (mst_state == NULL)
>                  return -ENOMEM;
>  
> +       mst_state->total_avail_slots = 63;
> +       mst_state->start_slot = 1;
> +
>          mst_state->mgr = mgr;
>          INIT_LIST_HEAD(&mst_state->vcpis);
>  
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index ddb9231d0309..f8152dfb34ed 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>          struct drm_private_state base;
>          struct list_head vcpis;
>          struct drm_dp_mst_topology_mgr *mgr;
> +       u8 total_avail_slots;
> +       u8 start_slot;
>  };
>  
>  #define to_dp_mst_topology_mgr(x) container_of(x, struct
> drm_dp_mst_topology_mgr, base)
> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>           */
>          int pbn_div;
>  
> +       /**
> +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> +        */
> +       u8 total_avail_slots;
> +
> +       /**
> +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> +        */
> +       u8 start_slot;
> +
>          /**
>           * @funcs: Atomic helper callbacks
>           */
> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>  
>  void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port);
>  
> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> *mst_state, uint8_t link_coding_cap);
>  
>  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>                                  struct drm_dp_mst_port *port);

Jani Nikula Oct. 13, 2021, 4:09 p.m. UTC | #3

On Tue, 12 Oct 2021, Bhawanpreet Lakha <[e-mail]> wrote:
> 8b/10b encoding format requires to reserve the first slot for
> recording metadata. Real data transmission starts from the second slot,
> with a total of available 63 slots available.
>
> In 128b/132b encoding format, metadata is transmitted separately
> in LLCP packet before MTP. Real data transmission starts from
> the first slot, with a total of 64 slots available.
>
> v2:
> * Remove get_mst_link_encoding_cap
> * Move total/start slots to mst_state, and copy it to mst_mgr in
> atomic_check
>
> Signed-off-by: Fangzhi Zuo <[e-mail]>
> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>  drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
>  include/drm/drm_dp_mst_helper.h               | 13 +++++++
>  3 files changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5020f2d36fe1..4ad50eb0091a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  #if defined(CONFIG_DRM_AMD_DC_DCN)
>  	struct dsc_mst_fairness_vars vars[MAX_PIPES];
>  #endif
> +	struct drm_dp_mst_topology_state *mst_state;
> +	struct drm_dp_mst_topology_mgr *mgr;
>  
>  	trace_amdgpu_dm_atomic_check_begin(state);
>  
> @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  		lock_and_validation_needed = true;
>  	}
>  
> +#if defined(CONFIG_DRM_AMD_DC_DCN)
> +	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
> +		struct amdgpu_dm_connector *aconnector;
> +		struct drm_connector *connector;
> +		struct drm_connector_list_iter iter;
> +		u8 link_coding_cap;
> +
> +		if (!mgr->mst_state )
> +			continue;
> +
> +		drm_connector_list_iter_begin(dev, &iter);
> +		drm_for_each_connector_iter(connector, &iter) {
> +			int id = connector->index;
> +
> +			if (id == mst_state->mgr->conn_base_id) {
> +				aconnector = to_amdgpu_dm_connector(connector);
> +				link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
> +				drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);
> +
> +				break;
> +			}
> +		}
> +		drm_connector_list_iter_end(&iter);
> +
> +	}
> +#endif

I wonder if we could split this to separate drm dp helper and amd driver
patches?

>  	/**
>  	 * Streams and planes are reset when there are changes that affect
>  	 * bandwidth. Anything that affects bandwidth needs to go through
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ad0795afc21c..fb5c47c4cb2e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>  	struct drm_dp_payload req_payload;
>  	struct drm_dp_mst_port *port;
>  	int i, j;
> -	int cur_slots = 1;
> +	int cur_slots = mgr->start_slot;
>  	bool skip;
>  
>  	mutex_lock(&mgr->payload_lock);
> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>  	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>  
>  	/* max. time slots - one slot for MTP header */
> -	if (num_slots > 63)
> +	if (num_slots > mgr->total_avail_slots)
>  		return -ENOSPC;
>  	return num_slots;
>  }
> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  	int ret;
>  
>  	/* max. time slots - one slot for MTP header */
> -	if (slots > 63)
> +	if (slots > mgr->total_avail_slots)
>  		return -ENOSPC;
>  
>  	vcpi->pbn = pbn;
> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>  
> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap)
> +{
> +	if (link_coding_cap == DP_CAP_ANSI_128B132B) {
> +		mst_state->total_avail_slots = 64;
> +		mst_state->start_slot = 0;
> +	}

The values never change AFAICT, should we store the channel encoding
instead, and use that information to initialize the values?

(Alternatively, why aren't the 8b/10b values initialized here if
128b/132b are?)

> +
> +	DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
> +			(link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
> +
>  /**
>   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>   * @mgr: manager for this port
> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  
>  	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>  	if (ret) {
> -		drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
> -			    DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> +		drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n",
> +			    DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->total_avail_slots, ret);
>  		drm_dp_mst_topology_put_port(port);
>  		goto out;
>  	}
> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,
>  					 struct drm_dp_mst_topology_state *mst_state)
>  {
>  	struct drm_dp_vcpi_allocation *vcpi;
> -	int avail_slots = 63, payload_count = 0;
> +	int avail_slots = mgr->total_avail_slots, payload_count = 0;

>  
>  	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>  		/* Releasing VCPI is always OK-even if the port is gone */
> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,
>  		}
>  	}
>  	drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
> -		       mgr, mst_state, avail_slots, 63 - avail_slots);
> +		       mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);
>  
>  	return 0;
>  }
> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
>  			break;
>  
>  		mutex_lock(&mgr->lock);
> +
> +		mgr->start_slot = mst_state->start_slot;
> +		mgr->total_avail_slots = mst_state->total_avail_slots;
> +

It feels wrong to me to be copying stuff from mst_state to mgr in
general, and in atomic check hook in particular.

>  		ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary,
>  							    mst_state);
>  		mutex_unlock(&mgr->lock);
> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>  	if (!mgr->proposed_vcpis)
>  		return -ENOMEM;
>  	set_bit(0, &mgr->payload_mask);
> +	mgr->total_avail_slots = 63;
> +	mgr->start_slot = 1;
>  
>  	mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>  	if (mst_state == NULL)
>  		return -ENOMEM;
>  
> +	mst_state->total_avail_slots = 63;
> +	mst_state->start_slot = 1;
> +

The magic values for 8b/10b are now duplicated to two places, with the
128b/132b values in a separate place.

BR,
Jani.

>  	mst_state->mgr = mgr;
>  	INIT_LIST_HEAD(&mst_state->vcpis);
>  
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index ddb9231d0309..f8152dfb34ed 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>  	struct drm_private_state base;
>  	struct list_head vcpis;
>  	struct drm_dp_mst_topology_mgr *mgr;
> +	u8 total_avail_slots;
> +	u8 start_slot;
>  };
>  
>  #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base)
> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>  	 */
>  	int pbn_div;
>  
> +	/**
> +	 * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> +	 */
> +	u8 total_avail_slots;
> +
> +	/**
> +	 * @start_slot: 1 for 8b/10b, 0 for 128/132b
> +	 */
> +	u8 start_slot;
> +
>  	/**
>  	 * @funcs: Atomic helper callbacks
>  	 */
> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
>  
>  void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
>  
> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);
>  
>  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  				struct drm_dp_mst_port *port);

kernel test robot Oct. 13, 2021, 6:52 p.m. UTC | #4

Hi Bhawanpreet,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip linus/master v5.15-rc5 next-20211013]
[cannot apply to drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bhawanpreet-Lakha/drm-Update-MST-First-Link-Slot-Information-Based-on-Encoding-Format/20211013-060001
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a003-20211012 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/5604bf980dcbfdd7650b7e1d5d4a2fd9f18cd866
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bhawanpreet-Lakha/drm-Update-MST-First-Link-Slot-Information-Based-on-Encoding-Format/20211013-060001
        git checkout 5604bf980dcbfdd7650b7e1d5d4a2fd9f18cd866
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/amd/amdgpu/../display/dmub/dmub_srv.h:67,
                    from drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:35:
   drivers/gpu/drm/amd/amdgpu/../display/dmub/inc/dmub_cmd.h: In function 'dmub_rb_flush_pending':
   drivers/gpu/drm/amd/amdgpu/../display/dmub/inc/dmub_cmd.h:2819:12: warning: variable 'temp' set but not used [-Wunused-but-set-variable]
    2819 |   uint64_t temp;
         |            ^~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: At top level:
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:633:6: warning: no previous prototype for 'dmub_aux_setconfig_callback' [-Wmissing-prototypes]
     633 | void dmub_aux_setconfig_callback(struct amdgpu_device *adev, struct dmub_notification *notify)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:649:6: warning: no previous prototype for 'dmub_hpd_callback' [-Wmissing-prototypes]
     649 | void dmub_hpd_callback(struct amdgpu_device *adev, struct dmub_notification *notify)
         |      ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:706:6: warning: no previous prototype for 'register_dmub_notify_callback' [-Wmissing-prototypes]
     706 | bool register_dmub_notify_callback(struct amdgpu_device *adev, enum dmub_notification_type type,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 'dm_update_mst_vcpi_slots_for_dsc':
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:7174:12: warning: variable 'clock' set but not used [-Wunused-but-set-variable]
    7174 |  int i, j, clock;
         |            ^~~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 'amdgpu_dm_atomic_check':
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:10912:23: error: implicit declaration of function 'dc_link_dp_mst_decide_link_encoding_format' [-Werror=implicit-function-declaration]
   10912 |     link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: At top level:
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11394:5: warning: no previous prototype for 'amdgpu_dm_set_dmub_async_sync_status' [-Wmissing-prototypes]
   11394 | int amdgpu_dm_set_dmub_async_sync_status(bool is_cmd_aux, struct dc_context *ctx,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/dc_link_dp_mst_decide_link_encoding_format +10912 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c

 10643	
 10644	/**
 10645	 * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
 10646	 * @dev: The DRM device
 10647	 * @state: The atomic state to commit
 10648	 *
 10649	 * Validate that the given atomic state is programmable by DC into hardware.
 10650	 * This involves constructing a &struct dc_state reflecting the new hardware
 10651	 * state we wish to commit, then querying DC to see if it is programmable. It's
 10652	 * important not to modify the existing DC state. Otherwise, atomic_check
 10653	 * may unexpectedly commit hardware changes.
 10654	 *
 10655	 * When validating the DC state, it's important that the right locks are
 10656	 * acquired. For full updates case which removes/adds/updates streams on one
 10657	 * CRTC while flipping on another CRTC, acquiring global lock will guarantee
 10658	 * that any such full update commit will wait for completion of any outstanding
 10659	 * flip using DRMs synchronization events.
 10660	 *
 10661	 * Note that DM adds the affected connectors for all CRTCs in state, when that
 10662	 * might not seem necessary. This is because DC stream creation requires the
 10663	 * DC sink, which is tied to the DRM connector state. Cleaning this up should
 10664	 * be possible but non-trivial - a possible TODO item.
 10665	 *
 10666	 * Return: -Error code if validation failed.
 10667	 */
 10668	static int amdgpu_dm_atomic_check(struct drm_device *dev,
 10669					  struct drm_atomic_state *state)
 10670	{
 10671		struct amdgpu_device *adev = drm_to_adev(dev);
 10672		struct dm_atomic_state *dm_state = NULL;
 10673		struct dc *dc = adev->dm.dc;
 10674		struct drm_connector *connector;
 10675		struct drm_connector_state *old_con_state, *new_con_state;
 10676		struct drm_crtc *crtc;
 10677		struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 10678		struct drm_plane *plane;
 10679		struct drm_plane_state *old_plane_state, *new_plane_state;
 10680		enum dc_status status;
 10681		int ret, i;
 10682		bool lock_and_validation_needed = false;
 10683		struct dm_crtc_state *dm_old_crtc_state;
 10684	#if defined(CONFIG_DRM_AMD_DC_DCN)
 10685		struct dsc_mst_fairness_vars vars[MAX_PIPES];
 10686	#endif
 10687		struct drm_dp_mst_topology_state *mst_state;
 10688		struct drm_dp_mst_topology_mgr *mgr;
 10689	
 10690		trace_amdgpu_dm_atomic_check_begin(state);
 10691	
 10692		ret = drm_atomic_helper_check_modeset(dev, state);
 10693		if (ret)
 10694			goto fail;
 10695	
 10696		/* Check connector changes */
 10697		for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 10698			struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
 10699			struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 10700	
 10701			/* Skip connectors that are disabled or part of modeset already. */
 10702			if (!old_con_state->crtc && !new_con_state->crtc)
 10703				continue;
 10704	
 10705			if (!new_con_state->crtc)
 10706				continue;
 10707	
 10708			new_crtc_state = drm_atomic_get_crtc_state(state, new_con_state->crtc);
 10709			if (IS_ERR(new_crtc_state)) {
 10710				ret = PTR_ERR(new_crtc_state);
 10711				goto fail;
 10712			}
 10713	
 10714			if (dm_old_con_state->abm_level !=
 10715			    dm_new_con_state->abm_level)
 10716				new_crtc_state->connectors_changed = true;
 10717		}
 10718	
 10719	#if defined(CONFIG_DRM_AMD_DC_DCN)
 10720		if (dc_resource_is_dsc_encoding_supported(dc)) {
 10721			for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10722				if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
 10723					ret = add_affected_mst_dsc_crtcs(state, crtc);
 10724					if (ret)
 10725						goto fail;
 10726				}
 10727			}
 10728		}
 10729	#endif
 10730		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10731			dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
 10732	
 10733			if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
 10734			    !new_crtc_state->color_mgmt_changed &&
 10735			    old_crtc_state->vrr_enabled == new_crtc_state->vrr_enabled &&
 10736				dm_old_crtc_state->dsc_force_changed == false)
 10737				continue;
 10738	
 10739			ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
 10740			if (ret)
 10741				goto fail;
 10742	
 10743			if (!new_crtc_state->enable)
 10744				continue;
 10745	
 10746			ret = drm_atomic_add_affected_connectors(state, crtc);
 10747			if (ret)
 10748				return ret;
 10749	
 10750			ret = drm_atomic_add_affected_planes(state, crtc);
 10751			if (ret)
 10752				goto fail;
 10753	
 10754			if (dm_old_crtc_state->dsc_force_changed)
 10755				new_crtc_state->mode_changed = true;
 10756		}
 10757	
 10758		/*
 10759		 * Add all primary and overlay planes on the CRTC to the state
 10760		 * whenever a plane is enabled to maintain correct z-ordering
 10761		 * and to enable fast surface updates.
 10762		 */
 10763		drm_for_each_crtc(crtc, dev) {
 10764			bool modified = false;
 10765	
 10766			for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 10767				if (plane->type == DRM_PLANE_TYPE_CURSOR)
 10768					continue;
 10769	
 10770				if (new_plane_state->crtc == crtc ||
 10771				    old_plane_state->crtc == crtc) {
 10772					modified = true;
 10773					break;
 10774				}
 10775			}
 10776	
 10777			if (!modified)
 10778				continue;
 10779	
 10780			drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
 10781				if (plane->type == DRM_PLANE_TYPE_CURSOR)
 10782					continue;
 10783	
 10784				new_plane_state =
 10785					drm_atomic_get_plane_state(state, plane);
 10786	
 10787				if (IS_ERR(new_plane_state)) {
 10788					ret = PTR_ERR(new_plane_state);
 10789					goto fail;
 10790				}
 10791			}
 10792		}
 10793	
 10794		/* Remove exiting planes if they are modified */
 10795		for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
 10796			ret = dm_update_plane_state(dc, state, plane,
 10797						    old_plane_state,
 10798						    new_plane_state,
 10799						    false,
 10800						    &lock_and_validation_needed);
 10801			if (ret)
 10802				goto fail;
 10803		}
 10804	
 10805		/* Disable all crtcs which require disable */
 10806		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10807			ret = dm_update_crtc_state(&adev->dm, state, crtc,
 10808						   old_crtc_state,
 10809						   new_crtc_state,
 10810						   false,
 10811						   &lock_and_validation_needed);
 10812			if (ret)
 10813				goto fail;
 10814		}
 10815	
 10816		/* Enable all crtcs which require enable */
 10817		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10818			ret = dm_update_crtc_state(&adev->dm, state, crtc,
 10819						   old_crtc_state,
 10820						   new_crtc_state,
 10821						   true,
 10822						   &lock_and_validation_needed);
 10823			if (ret)
 10824				goto fail;
 10825		}
 10826	
 10827		ret = validate_overlay(state);
 10828		if (ret)
 10829			goto fail;
 10830	
 10831		/* Add new/modified planes */
 10832		for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
 10833			ret = dm_update_plane_state(dc, state, plane,
 10834						    old_plane_state,
 10835						    new_plane_state,
 10836						    true,
 10837						    &lock_and_validation_needed);
 10838			if (ret)
 10839				goto fail;
 10840		}
 10841	
 10842		/* Run this here since we want to validate the streams we created */
 10843		ret = drm_atomic_helper_check_planes(dev, state);
 10844		if (ret)
 10845			goto fail;
 10846	
 10847		/* Check cursor planes scaling */
 10848		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 10849			ret = dm_check_crtc_cursor(state, crtc, new_crtc_state);
 10850			if (ret)
 10851				goto fail;
 10852		}
 10853	
 10854		if (state->legacy_cursor_update) {
 10855			/*
 10856			 * This is a fast cursor update coming from the plane update
 10857			 * helper, check if it can be done asynchronously for better
 10858			 * performance.
 10859			 */
 10860			state->async_update =
 10861				!drm_atomic_helper_async_check(dev, state);
 10862	
 10863			/*
 10864			 * Skip the remaining global validation if this is an async
 10865			 * update. Cursor updates can be done without affecting
 10866			 * state or bandwidth calcs and this avoids the performance
 10867			 * penalty of locking the private state object and
 10868			 * allocating a new dc_state.
 10869			 */
 10870			if (state->async_update)
 10871				return 0;
 10872		}
 10873	
 10874		/* Check scaling and underscan changes*/
 10875		/* TODO Removed scaling changes validation due to inability to commit
 10876		 * new stream into context w\o causing full reset. Need to
 10877		 * decide how to handle.
 10878		 */
 10879		for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 10880			struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
 10881			struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 10882			struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
 10883	
 10884			/* Skip any modesets/resets */
 10885			if (!acrtc || drm_atomic_crtc_needs_modeset(
 10886					drm_atomic_get_new_crtc_state(state, &acrtc->base)))
 10887				continue;
 10888	
 10889			/* Skip any thing not scale or underscan changes */
 10890			if (!is_scaling_state_different(dm_new_con_state, dm_old_con_state))
 10891				continue;
 10892	
 10893			lock_and_validation_needed = true;
 10894		}
 10895	
 10896	#if defined(CONFIG_DRM_AMD_DC_DCN)
 10897		for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
 10898			struct amdgpu_dm_connector *aconnector;
 10899			struct drm_connector *connector;
 10900			struct drm_connector_list_iter iter;
 10901			u8 link_coding_cap;
 10902	
 10903			if (!mgr->mst_state )
 10904				continue;
 10905	
 10906			drm_connector_list_iter_begin(dev, &iter);
 10907			drm_for_each_connector_iter(connector, &iter) {
 10908				int id = connector->index;
 10909	
 10910				if (id == mst_state->mgr->conn_base_id) {
 10911					aconnector = to_amdgpu_dm_connector(connector);
 10912					link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
 10913					drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);
 10914	
 10915					break;
 10916				}
 10917			}
 10918			drm_connector_list_iter_end(&iter);
 10919	
 10920		}
 10921	#endif
 10922		/**
 10923		 * Streams and planes are reset when there are changes that affect
 10924		 * bandwidth. Anything that affects bandwidth needs to go through
 10925		 * DC global validation to ensure that the configuration can be applied
 10926		 * to hardware.
 10927		 *
 10928		 * We have to currently stall out here in atomic_check for outstanding
 10929		 * commits to finish in this case because our IRQ handlers reference
 10930		 * DRM state directly - we can end up disabling interrupts too early
 10931		 * if we don't.
 10932		 *
 10933		 * TODO: Remove this stall and drop DM state private objects.
 10934		 */
 10935		if (lock_and_validation_needed) {
 10936			ret = dm_atomic_get_state(state, &dm_state);
 10937			if (ret)
 10938				goto fail;
 10939	
 10940			ret = do_aquire_global_lock(dev, state);
 10941			if (ret)
 10942				goto fail;
 10943	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Bhawanpreet Lakha Oct. 13, 2021, 7:33 p.m. UTC | #5

On 2021-10-13 12:09 p.m., Jani Nikula wrote:
> On Tue, 12 Oct 2021, Bhawanpreet Lakha <[e-mail]> wrote:
>> 8b/10b encoding format requires to reserve the first slot for
>> recording metadata. Real data transmission starts from the second slot,
>> with a total of available 63 slots available.
>>
>> In 128b/132b encoding format, metadata is transmitted separately
>> in LLCP packet before MTP. Real data transmission starts from
>> the first slot, with a total of 64 slots available.
>>
>> v2:
>> * Remove get_mst_link_encoding_cap
>> * Move total/start slots to mst_state, and copy it to mst_mgr in
>> atomic_check
>>
>> Signed-off-by: Fangzhi Zuo <[e-mail]>
>> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>>   drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
>>   include/drm/drm_dp_mst_helper.h               | 13 +++++++
>>   3 files changed, 69 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 5020f2d36fe1..4ad50eb0091a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>   #if defined(CONFIG_DRM_AMD_DC_DCN)
>>   	struct dsc_mst_fairness_vars vars[MAX_PIPES];
>>   #endif
>> +	struct drm_dp_mst_topology_state *mst_state;
>> +	struct drm_dp_mst_topology_mgr *mgr;
>>   
>>   	trace_amdgpu_dm_atomic_check_begin(state);
>>   
>> @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>   		lock_and_validation_needed = true;
>>   	}
>>   
>> +#if defined(CONFIG_DRM_AMD_DC_DCN)
>> +	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
>> +		struct amdgpu_dm_connector *aconnector;
>> +		struct drm_connector *connector;
>> +		struct drm_connector_list_iter iter;
>> +		u8 link_coding_cap;
>> +
>> +		if (!mgr->mst_state )
>> +			continue;
>> +
>> +		drm_connector_list_iter_begin(dev, &iter);
>> +		drm_for_each_connector_iter(connector, &iter) {
>> +			int id = connector->index;
>> +
>> +			if (id == mst_state->mgr->conn_base_id) {
>> +				aconnector = to_amdgpu_dm_connector(connector);
>> +				link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
>> +				drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);
>> +
>> +				break;
>> +			}
>> +		}
>> +		drm_connector_list_iter_end(&iter);
>> +
>> +	}
>> +#endif
> I wonder if we could split this to separate drm dp helper and amd driver
> patches?
>
I believe that was the original structure but, lyude recommended to put 
them into the same patch to see how it is being used
>>   	/**
>>   	 * Streams and planes are reset when there are changes that affect
>>   	 * bandwidth. Anything that affects bandwidth needs to go through
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index ad0795afc21c..fb5c47c4cb2e 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
>>   	struct drm_dp_payload req_payload;
>>   	struct drm_dp_mst_port *port;
>>   	int i, j;
>> -	int cur_slots = 1;
>> +	int cur_slots = mgr->start_slot;
>>   	bool skip;
>>   
>>   	mutex_lock(&mgr->payload_lock);
>> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>>   	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>>   
>>   	/* max. time slots - one slot for MTP header */
>> -	if (num_slots > 63)
>> +	if (num_slots > mgr->total_avail_slots)
>>   		return -ENOSPC;
>>   	return num_slots;
>>   }
>> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>>   	int ret;
>>   
>>   	/* max. time slots - one slot for MTP header */
>> -	if (slots > 63)
>> +	if (slots > mgr->total_avail_slots)
>>   		return -ENOSPC;
>>   
>>   	vcpi->pbn = pbn;
>> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>>   }
>>   EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>>   
>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap)
>> +{
>> +	if (link_coding_cap == DP_CAP_ANSI_128B132B) {
>> +		mst_state->total_avail_slots = 64;
>> +		mst_state->start_slot = 0;
>> +	}
> The values never change AFAICT, should we store the channel encoding
> instead, and use that information to initialize the values?
>
> (Alternatively, why aren't the 8b/10b values initialized here if
> 128b/132b are?)
I agree, 8b/10 are the default, but in case where we switch from 
128b/132 -> 8b/10b we should be updating them here aswell.
>> +
>> +	DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
>> +			(link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);
>> +}
>> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
>> +
>>   /**
>>    * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>>    * @mgr: manager for this port
>> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>>   
>>   	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>>   	if (ret) {
>> -		drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
>> -			    DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
>> +		drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n",
>> +			    DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->total_avail_slots, ret);
>>   		drm_dp_mst_topology_put_port(port);
>>   		goto out;
>>   	}
>> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,
>>   					 struct drm_dp_mst_topology_state *mst_state)
>>   {
>>   	struct drm_dp_vcpi_allocation *vcpi;
>> -	int avail_slots = 63, payload_count = 0;
>> +	int avail_slots = mgr->total_avail_slots, payload_count = 0;
>>   
>>   	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>>   		/* Releasing VCPI is always OK-even if the port is gone */
>> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,
>>   		}
>>   	}
>>   	drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
>> -		       mgr, mst_state, avail_slots, 63 - avail_slots);
>> +		       mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);
>>   
>>   	return 0;
>>   }
>> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
>>   			break;
>>   
>>   		mutex_lock(&mgr->lock);
>> +
>> +		mgr->start_slot = mst_state->start_slot;
>> +		mgr->total_avail_slots = mst_state->total_avail_slots;
>> +
> It feels wrong to me to be copying stuff from mst_state to mgr in
> general, and in atomic check hook in particular.
previously we were setting it directly in the mgr, and this was 
suggested by lyude. I am not sure what the alternative is.
>>   		ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary,
>>   							    mst_state);
>>   		mutex_unlock(&mgr->lock);
>> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>>   	if (!mgr->proposed_vcpis)
>>   		return -ENOMEM;
>>   	set_bit(0, &mgr->payload_mask);
>> +	mgr->total_avail_slots = 63;
>> +	mgr->start_slot = 1;
>>   
>>   	mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>>   	if (mst_state == NULL)
>>   		return -ENOMEM;
>>   
>> +	mst_state->total_avail_slots = 63;
>> +	mst_state->start_slot = 1;
>> +
> The magic values for 8b/10b are now duplicated to two places, with the
> 128b/132b values in a separate place.

8b/10b is the default (to make sure we don't break any existing driver 
that doesn't use 128b/132b). Are you against setting it as the default 
here? or do you mean we should use #define for this? the magic numbers 
are currently being used directly right now (inside 
drm_dp_find_vcpi_slots, drm_dp_init_vcpi).

Bhawan

> BR,
> Jani.
>
>>   	mst_state->mgr = mgr;
>>   	INIT_LIST_HEAD(&mst_state->vcpis);
>>   
>> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
>> index ddb9231d0309..f8152dfb34ed 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>>   	struct drm_private_state base;
>>   	struct list_head vcpis;
>>   	struct drm_dp_mst_topology_mgr *mgr;
>> +	u8 total_avail_slots;
>> +	u8 start_slot;
>>   };
>>   
>>   #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base)
>> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>>   	 */
>>   	int pbn_div;
>>   
>> +	/**
>> +	 * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
>> +	 */
>> +	u8 total_avail_slots;
>> +
>> +	/**
>> +	 * @start_slot: 1 for 8b/10b, 0 for 128/132b
>> +	 */
>> +	u8 start_slot;
>> +
>>   	/**
>>   	 * @funcs: Atomic helper callbacks
>>   	 */
>> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
>>   
>>   void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
>>   
>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);
>>   
>>   void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>>   				struct drm_dp_mst_port *port);

Lyude Paul Oct. 13, 2021, 7:37 p.m. UTC | #6

On Wed, 2021-10-13 at 15:33 -0400, Bhawanpreet Lakha wrote:
> 
> > I wonder if we could split this to separate drm dp helper and amd driver
> > patches?

Whoops! I thought it was strange that I would say this but it seems there was
a misunderstanding on my part: when the original patch series was submitted I
was only CC'd on the first patch and I guess I must not have noticed the 1/2
in the subject line, so I thought Jerry had submitted just a single patch for
the helper. Looking back in my email history though that definitely wasn't
correct, and the original patch structure was what we wanted to go with.

Sorry for the confusion on my part!

> > 
> I believe that was the original structure but, lyude recommended to put 
> them into the same patch to see how it is being used
> > >         /**
> > >          * Streams and planes are reset when there are changes that
> > > affect
> > >          * bandwidth. Anything that affects bandwidth needs to go
> > > through
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index ad0795afc21c..fb5c47c4cb2e 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
> > > drm_dp_mst_topology_mgr *mgr)
> > >         struct drm_dp_payload req_payload;
> > >         struct drm_dp_mst_port *port;
> > >         int i, j;
> > > -       int cur_slots = 1;
> > > +       int cur_slots = mgr->start_slot;
> > >         bool skip;
> > >   
> > >         mutex_lock(&mgr->payload_lock);
> > > @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > >   
> > >         /* max. time slots - one slot for MTP header */
> > > -       if (num_slots > 63)
> > > +       if (num_slots > mgr->total_avail_slots)
> > >                 return -ENOSPC;
> > >         return num_slots;
> > >   }
> > > @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >         int ret;
> > >   
> > >         /* max. time slots - one slot for MTP header */
> > > -       if (slots > 63)
> > > +       if (slots > mgr->total_avail_slots)
> > >                 return -ENOSPC;
> > >   
> > >         vcpi->pbn = pbn;
> > > @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
> > > drm_atomic_state *state,
> > >   }
> > >   EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> > >   
> > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > *mst_state, uint8_t link_coding_cap)
> > > +{
> > > +       if (link_coding_cap == DP_CAP_ANSI_128B132B) {
> > > +               mst_state->total_avail_slots = 64;
> > > +               mst_state->start_slot = 0;
> > > +       }
> > The values never change AFAICT, should we store the channel encoding
> > instead, and use that information to initialize the values?
> > 
> > (Alternatively, why aren't the 8b/10b values initialized here if
> > 128b/132b are?)
> I agree, 8b/10 are the default, but in case where we switch from 
> 128b/132 -> 8b/10b we should be updating them here aswell.
> > > +
> > > +       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
> > > +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ?
> > > "128b/132b":"8b/10b", mst_state->mgr);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
> > > +
> > >   /**
> > >    * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> > >    * @mgr: manager for this port
> > > @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >   
> > >         ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
> > >         if (ret) {
> > > -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d
> > > max=63 ret=%d\n",
> > > -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> > > +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d
> > > max=%d ret=%d\n",
> > > +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
> > > >total_avail_slots, ret);
> > >                 drm_dp_mst_topology_put_port(port);
> > >                 goto out;
> > >         }
> > > @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >                                          struct
> > > drm_dp_mst_topology_state *mst_state)
> > >   {
> > >         struct drm_dp_vcpi_allocation *vcpi;
> > > -       int avail_slots = 63, payload_count = 0;
> > > +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
> > >   
> > >         list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> > >                 /* Releasing VCPI is always OK-even if the port is gone
> > > */
> > > @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >                 }
> > >         }
> > >         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI
> > > avail=%d used=%d\n",
> > > -                      mgr, mst_state, avail_slots, 63 - avail_slots);
> > > +                      mgr, mst_state, avail_slots, mgr-
> > > >total_avail_slots - avail_slots);
> > >   
> > >         return 0;
> > >   }
> > > @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct
> > > drm_atomic_state *state)
> > >                         break;
> > >   
> > >                 mutex_lock(&mgr->lock);
> > > +
> > > +               mgr->start_slot = mst_state->start_slot;
> > > +               mgr->total_avail_slots = mst_state->total_avail_slots;
> > > +
> > It feels wrong to me to be copying stuff from mst_state to mgr in
> > general, and in atomic check hook in particular.
> previously we were setting it directly in the mgr, and this was 
> suggested by lyude. I am not sure what the alternative is.
> > >                 ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> > > >mst_primary,
> > >                                                             mst_state);
> > >                 mutex_unlock(&mgr->lock);
> > > @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >         if (!mgr->proposed_vcpis)
> > >                 return -ENOMEM;
> > >         set_bit(0, &mgr->payload_mask);
> > > +       mgr->total_avail_slots = 63;
> > > +       mgr->start_slot = 1;
> > >   
> > >         mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> > >         if (mst_state == NULL)
> > >                 return -ENOMEM;
> > >   
> > > +       mst_state->total_avail_slots = 63;
> > > +       mst_state->start_slot = 1;
> > > +
> > The magic values for 8b/10b are now duplicated to two places, with the
> > 128b/132b values in a separate place.
> 
> 8b/10b is the default (to make sure we don't break any existing driver 
> that doesn't use 128b/132b). Are you against setting it as the default 
> here? or do you mean we should use #define for this? the magic numbers 
> are currently being used directly right now (inside 
> drm_dp_find_vcpi_slots, drm_dp_init_vcpi).
> 
> Bhawan
> 
> > BR,
> > Jani.
> > 
> > >         mst_state->mgr = mgr;
> > >         INIT_LIST_HEAD(&mst_state->vcpis);
> > >   
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index ddb9231d0309..f8152dfb34ed 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
> > >         struct drm_private_state base;
> > >         struct list_head vcpis;
> > >         struct drm_dp_mst_topology_mgr *mgr;
> > > +       u8 total_avail_slots;
> > > +       u8 start_slot;
> > >   };
> > >   
> > >   #define to_dp_mst_topology_mgr(x) container_of(x, struct
> > > drm_dp_mst_topology_mgr, base)
> > > @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
> > >          */
> > >         int pbn_div;
> > >   
> > > +       /**
> > > +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> > > +        */
> > > +       u8 total_avail_slots;
> > > +
> > > +       /**
> > > +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> > > +        */
> > > +       u8 start_slot;
> > > +
> > >         /**
> > >          * @funcs: Atomic helper callbacks
> > >          */
> > > @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> > > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > >   
> > >   void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> > > struct drm_dp_mst_port *port);
> > >   
> > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > *mst_state, uint8_t link_coding_cap);
> > >   
> > >   void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > >                                 struct drm_dp_mst_port *port);
>

kernel test robot Oct. 13, 2021, 9:54 p.m. UTC | #7

Hi Bhawanpreet,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip linus/master v5.15-rc5 next-20211013]
[cannot apply to drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bhawanpreet-Lakha/drm-Update-MST-First-Link-Slot-Information-Based-on-Encoding-Format/20211013-060001
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-a005-20211013 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a8c695542b2987eb9a203d5663a0740cb4725f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5604bf980dcbfdd7650b7e1d5d4a2fd9f18cd866
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bhawanpreet-Lakha/drm-Update-MST-First-Link-Slot-Information-Based-on-Encoding-Format/20211013-060001
        git checkout 5604bf980dcbfdd7650b7e1d5d4a2fd9f18cd866
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:35:
   In file included from drivers/gpu/drm/amd/amdgpu/../display/dmub/dmub_srv.h:67:
   drivers/gpu/drm/amd/amdgpu/../display/dmub/inc/dmub_cmd.h:2819:12: warning: variable 'temp' set but not used [-Wunused-but-set-variable]
                   uint64_t temp;
                            ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:633:6: warning: no previous prototype for function 'dmub_aux_setconfig_callback' [-Wmissing-prototypes]
   void dmub_aux_setconfig_callback(struct amdgpu_device *adev, struct dmub_notification *notify)
        ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:633:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void dmub_aux_setconfig_callback(struct amdgpu_device *adev, struct dmub_notification *notify)
   ^
   static 
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:649:6: warning: no previous prototype for function 'dmub_hpd_callback' [-Wmissing-prototypes]
   void dmub_hpd_callback(struct amdgpu_device *adev, struct dmub_notification *notify)
        ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:649:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void dmub_hpd_callback(struct amdgpu_device *adev, struct dmub_notification *notify)
   ^
   static 
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:706:6: warning: no previous prototype for function 'register_dmub_notify_callback' [-Wmissing-prototypes]
   bool register_dmub_notify_callback(struct amdgpu_device *adev, enum dmub_notification_type type,
        ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:706:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   bool register_dmub_notify_callback(struct amdgpu_device *adev, enum dmub_notification_type type,
   ^
   static 
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:7174:12: warning: variable 'clock' set but not used [-Wunused-but-set-variable]
           int i, j, clock;
                     ^
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:10912:23: error: implicit declaration of function 'dc_link_dp_mst_decide_link_encoding_format' [-Werror,-Wimplicit-function-declaration]
                                   link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
                                                     ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11394:5: warning: no previous prototype for function 'amdgpu_dm_set_dmub_async_sync_status' [-Wmissing-prototypes]
   int amdgpu_dm_set_dmub_async_sync_status(bool is_cmd_aux, struct dc_context *ctx,
       ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11394:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int amdgpu_dm_set_dmub_async_sync_status(bool is_cmd_aux, struct dc_context *ctx,
   ^
   static 
   6 warnings and 1 error generated.


vim +/dc_link_dp_mst_decide_link_encoding_format +10912 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c

 10643	
 10644	/**
 10645	 * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
 10646	 * @dev: The DRM device
 10647	 * @state: The atomic state to commit
 10648	 *
 10649	 * Validate that the given atomic state is programmable by DC into hardware.
 10650	 * This involves constructing a &struct dc_state reflecting the new hardware
 10651	 * state we wish to commit, then querying DC to see if it is programmable. It's
 10652	 * important not to modify the existing DC state. Otherwise, atomic_check
 10653	 * may unexpectedly commit hardware changes.
 10654	 *
 10655	 * When validating the DC state, it's important that the right locks are
 10656	 * acquired. For full updates case which removes/adds/updates streams on one
 10657	 * CRTC while flipping on another CRTC, acquiring global lock will guarantee
 10658	 * that any such full update commit will wait for completion of any outstanding
 10659	 * flip using DRMs synchronization events.
 10660	 *
 10661	 * Note that DM adds the affected connectors for all CRTCs in state, when that
 10662	 * might not seem necessary. This is because DC stream creation requires the
 10663	 * DC sink, which is tied to the DRM connector state. Cleaning this up should
 10664	 * be possible but non-trivial - a possible TODO item.
 10665	 *
 10666	 * Return: -Error code if validation failed.
 10667	 */
 10668	static int amdgpu_dm_atomic_check(struct drm_device *dev,
 10669					  struct drm_atomic_state *state)
 10670	{
 10671		struct amdgpu_device *adev = drm_to_adev(dev);
 10672		struct dm_atomic_state *dm_state = NULL;
 10673		struct dc *dc = adev->dm.dc;
 10674		struct drm_connector *connector;
 10675		struct drm_connector_state *old_con_state, *new_con_state;
 10676		struct drm_crtc *crtc;
 10677		struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 10678		struct drm_plane *plane;
 10679		struct drm_plane_state *old_plane_state, *new_plane_state;
 10680		enum dc_status status;
 10681		int ret, i;
 10682		bool lock_and_validation_needed = false;
 10683		struct dm_crtc_state *dm_old_crtc_state;
 10684	#if defined(CONFIG_DRM_AMD_DC_DCN)
 10685		struct dsc_mst_fairness_vars vars[MAX_PIPES];
 10686	#endif
 10687		struct drm_dp_mst_topology_state *mst_state;
 10688		struct drm_dp_mst_topology_mgr *mgr;
 10689	
 10690		trace_amdgpu_dm_atomic_check_begin(state);
 10691	
 10692		ret = drm_atomic_helper_check_modeset(dev, state);
 10693		if (ret)
 10694			goto fail;
 10695	
 10696		/* Check connector changes */
 10697		for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 10698			struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
 10699			struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 10700	
 10701			/* Skip connectors that are disabled or part of modeset already. */
 10702			if (!old_con_state->crtc && !new_con_state->crtc)
 10703				continue;
 10704	
 10705			if (!new_con_state->crtc)
 10706				continue;
 10707	
 10708			new_crtc_state = drm_atomic_get_crtc_state(state, new_con_state->crtc);
 10709			if (IS_ERR(new_crtc_state)) {
 10710				ret = PTR_ERR(new_crtc_state);
 10711				goto fail;
 10712			}
 10713	
 10714			if (dm_old_con_state->abm_level !=
 10715			    dm_new_con_state->abm_level)
 10716				new_crtc_state->connectors_changed = true;
 10717		}
 10718	
 10719	#if defined(CONFIG_DRM_AMD_DC_DCN)
 10720		if (dc_resource_is_dsc_encoding_supported(dc)) {
 10721			for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10722				if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
 10723					ret = add_affected_mst_dsc_crtcs(state, crtc);
 10724					if (ret)
 10725						goto fail;
 10726				}
 10727			}
 10728		}
 10729	#endif
 10730		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10731			dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
 10732	
 10733			if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
 10734			    !new_crtc_state->color_mgmt_changed &&
 10735			    old_crtc_state->vrr_enabled == new_crtc_state->vrr_enabled &&
 10736				dm_old_crtc_state->dsc_force_changed == false)
 10737				continue;
 10738	
 10739			ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
 10740			if (ret)
 10741				goto fail;
 10742	
 10743			if (!new_crtc_state->enable)
 10744				continue;
 10745	
 10746			ret = drm_atomic_add_affected_connectors(state, crtc);
 10747			if (ret)
 10748				return ret;
 10749	
 10750			ret = drm_atomic_add_affected_planes(state, crtc);
 10751			if (ret)
 10752				goto fail;
 10753	
 10754			if (dm_old_crtc_state->dsc_force_changed)
 10755				new_crtc_state->mode_changed = true;
 10756		}
 10757	
 10758		/*
 10759		 * Add all primary and overlay planes on the CRTC to the state
 10760		 * whenever a plane is enabled to maintain correct z-ordering
 10761		 * and to enable fast surface updates.
 10762		 */
 10763		drm_for_each_crtc(crtc, dev) {
 10764			bool modified = false;
 10765	
 10766			for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 10767				if (plane->type == DRM_PLANE_TYPE_CURSOR)
 10768					continue;
 10769	
 10770				if (new_plane_state->crtc == crtc ||
 10771				    old_plane_state->crtc == crtc) {
 10772					modified = true;
 10773					break;
 10774				}
 10775			}
 10776	
 10777			if (!modified)
 10778				continue;
 10779	
 10780			drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
 10781				if (plane->type == DRM_PLANE_TYPE_CURSOR)
 10782					continue;
 10783	
 10784				new_plane_state =
 10785					drm_atomic_get_plane_state(state, plane);
 10786	
 10787				if (IS_ERR(new_plane_state)) {
 10788					ret = PTR_ERR(new_plane_state);
 10789					goto fail;
 10790				}
 10791			}
 10792		}
 10793	
 10794		/* Remove exiting planes if they are modified */
 10795		for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
 10796			ret = dm_update_plane_state(dc, state, plane,
 10797						    old_plane_state,
 10798						    new_plane_state,
 10799						    false,
 10800						    &lock_and_validation_needed);
 10801			if (ret)
 10802				goto fail;
 10803		}
 10804	
 10805		/* Disable all crtcs which require disable */
 10806		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10807			ret = dm_update_crtc_state(&adev->dm, state, crtc,
 10808						   old_crtc_state,
 10809						   new_crtc_state,
 10810						   false,
 10811						   &lock_and_validation_needed);
 10812			if (ret)
 10813				goto fail;
 10814		}
 10815	
 10816		/* Enable all crtcs which require enable */
 10817		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 10818			ret = dm_update_crtc_state(&adev->dm, state, crtc,
 10819						   old_crtc_state,
 10820						   new_crtc_state,
 10821						   true,
 10822						   &lock_and_validation_needed);
 10823			if (ret)
 10824				goto fail;
 10825		}
 10826	
 10827		ret = validate_overlay(state);
 10828		if (ret)
 10829			goto fail;
 10830	
 10831		/* Add new/modified planes */
 10832		for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
 10833			ret = dm_update_plane_state(dc, state, plane,
 10834						    old_plane_state,
 10835						    new_plane_state,
 10836						    true,
 10837						    &lock_and_validation_needed);
 10838			if (ret)
 10839				goto fail;
 10840		}
 10841	
 10842		/* Run this here since we want to validate the streams we created */
 10843		ret = drm_atomic_helper_check_planes(dev, state);
 10844		if (ret)
 10845			goto fail;
 10846	
 10847		/* Check cursor planes scaling */
 10848		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 10849			ret = dm_check_crtc_cursor(state, crtc, new_crtc_state);
 10850			if (ret)
 10851				goto fail;
 10852		}
 10853	
 10854		if (state->legacy_cursor_update) {
 10855			/*
 10856			 * This is a fast cursor update coming from the plane update
 10857			 * helper, check if it can be done asynchronously for better
 10858			 * performance.
 10859			 */
 10860			state->async_update =
 10861				!drm_atomic_helper_async_check(dev, state);
 10862	
 10863			/*
 10864			 * Skip the remaining global validation if this is an async
 10865			 * update. Cursor updates can be done without affecting
 10866			 * state or bandwidth calcs and this avoids the performance
 10867			 * penalty of locking the private state object and
 10868			 * allocating a new dc_state.
 10869			 */
 10870			if (state->async_update)
 10871				return 0;
 10872		}
 10873	
 10874		/* Check scaling and underscan changes*/
 10875		/* TODO Removed scaling changes validation due to inability to commit
 10876		 * new stream into context w\o causing full reset. Need to
 10877		 * decide how to handle.
 10878		 */
 10879		for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 10880			struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
 10881			struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 10882			struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
 10883	
 10884			/* Skip any modesets/resets */
 10885			if (!acrtc || drm_atomic_crtc_needs_modeset(
 10886					drm_atomic_get_new_crtc_state(state, &acrtc->base)))
 10887				continue;
 10888	
 10889			/* Skip any thing not scale or underscan changes */
 10890			if (!is_scaling_state_different(dm_new_con_state, dm_old_con_state))
 10891				continue;
 10892	
 10893			lock_and_validation_needed = true;
 10894		}
 10895	
 10896	#if defined(CONFIG_DRM_AMD_DC_DCN)
 10897		for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
 10898			struct amdgpu_dm_connector *aconnector;
 10899			struct drm_connector *connector;
 10900			struct drm_connector_list_iter iter;
 10901			u8 link_coding_cap;
 10902	
 10903			if (!mgr->mst_state )
 10904				continue;
 10905	
 10906			drm_connector_list_iter_begin(dev, &iter);
 10907			drm_for_each_connector_iter(connector, &iter) {
 10908				int id = connector->index;
 10909	
 10910				if (id == mst_state->mgr->conn_base_id) {
 10911					aconnector = to_amdgpu_dm_connector(connector);
 10912					link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
 10913					drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);
 10914	
 10915					break;
 10916				}
 10917			}
 10918			drm_connector_list_iter_end(&iter);
 10919	
 10920		}
 10921	#endif
 10922		/**
 10923		 * Streams and planes are reset when there are changes that affect
 10924		 * bandwidth. Anything that affects bandwidth needs to go through
 10925		 * DC global validation to ensure that the configuration can be applied
 10926		 * to hardware.
 10927		 *
 10928		 * We have to currently stall out here in atomic_check for outstanding
 10929		 * commits to finish in this case because our IRQ handlers reference
 10930		 * DRM state directly - we can end up disabling interrupts too early
 10931		 * if we don't.
 10932		 *
 10933		 * TODO: Remove this stall and drop DM state private objects.
 10934		 */
 10935		if (lock_and_validation_needed) {
 10936			ret = dm_atomic_get_state(state, &dm_state);
 10937			if (ret)
 10938				goto fail;
 10939	
 10940			ret = do_aquire_global_lock(dev, state);
 10941			if (ret)
 10942				goto fail;
 10943	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Lyude Paul Oct. 13, 2021, 10:25 p.m. UTC | #8

Some comments below (also, sorry again for the mixup on the last review!)

On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:
> 8b/10b encoding format requires to reserve the first slot for
> recording metadata. Real data transmission starts from the second slot,
> with a total of available 63 slots available.
> 
> In 128b/132b encoding format, metadata is transmitted separately
> in LLCP packet before MTP. Real data transmission starts from
> the first slot, with a total of 64 slots available.
> 
> v2:
> * Remove get_mst_link_encoding_cap
> * Move total/start slots to mst_state, and copy it to mst_mgr in
> atomic_check
> 
> Signed-off-by: Fangzhi Zuo <[e-mail]>
> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>  drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
>  include/drm/drm_dp_mst_helper.h               | 13 +++++++
>  3 files changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5020f2d36fe1..4ad50eb0091a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
>  #if defined(CONFIG_DRM_AMD_DC_DCN)
>         struct dsc_mst_fairness_vars vars[MAX_PIPES];
>  #endif
> +       struct drm_dp_mst_topology_state *mst_state;
> +       struct drm_dp_mst_topology_mgr *mgr;
>  
>         trace_amdgpu_dm_atomic_check_begin(state);
>  
> @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
>                 lock_and_validation_needed = true;
>         }
>  
> +#if defined(CONFIG_DRM_AMD_DC_DCN)
> +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
> +               struct amdgpu_dm_connector *aconnector;
> +               struct drm_connector *connector;
> +               struct drm_connector_list_iter iter;
> +               u8 link_coding_cap;
> +
> +               if (!mgr->mst_state )
> +                       continue;

extraneous space

> +
> +               drm_connector_list_iter_begin(dev, &iter);
> +               drm_for_each_connector_iter(connector, &iter) {
> +                       int id = connector->index;
> +
> +                       if (id == mst_state->mgr->conn_base_id) {
> +                               aconnector =
> to_amdgpu_dm_connector(connector);
> +                               link_coding_cap =
> dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
> +                               drm_dp_mst_update_coding_cap(mst_state,
> link_coding_cap);
> +
> +                               break;
> +                       }
> +               }
> +               drm_connector_list_iter_end(&iter);
> +
> +       }
> +#endif
>         /**
>          * Streams and planes are reset when there are changes that affect
>          * bandwidth. Anything that affects bandwidth needs to go through
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ad0795afc21c..fb5c47c4cb2e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
> drm_dp_mst_topology_mgr *mgr)
>         struct drm_dp_payload req_payload;
>         struct drm_dp_mst_port *port;
>         int i, j;
> -       int cur_slots = 1;
> +       int cur_slots = mgr->start_slot;
>         bool skip;
>  
>         mutex_lock(&mgr->payload_lock);
> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
> drm_dp_mst_topology_mgr *mgr,
>         num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>  
>         /* max. time slots - one slot for MTP header */
> -       if (num_slots > 63)
> +       if (num_slots > mgr->total_avail_slots)
>                 return -ENOSPC;

For reasons I will explain a little further in this email, we might want to
drop this…

>         return num_slots;
>  }
> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>         int ret;
>  
>         /* max. time slots - one slot for MTP header */
> -       if (slots > 63)
> +       if (slots > mgr->total_avail_slots)

…and this

>                 return -ENOSPC;
>  
>         vcpi->pbn = pbn;
> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
> drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>  
> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> *mst_state, uint8_t link_coding_cap)

Need some kdocs here

> +{
> +       if (link_coding_cap == DP_CAP_ANSI_128B132B) {
> +               mst_state->total_avail_slots = 64;
> +               mst_state->start_slot = 0;
> +       }
> +
> +       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
> +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ?
> "128b/132b":"8b/10b", mst_state->mgr);

need to fix indenting here, and wrap this to 100 chars

> +}
> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
> +
>  /**
>   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>   * @mgr: manager for this port
> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>  
>         ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>         if (ret) {
> -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63
> ret=%d\n",
> -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d
> ret=%d\n",
> +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
> >total_avail_slots, ret);
>                 drm_dp_mst_topology_put_port(port);
>                 goto out;
>         }
> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> drm_dp_mst_topology_mgr *mgr,
>                                          struct drm_dp_mst_topology_state
> *mst_state)
>  {
>         struct drm_dp_vcpi_allocation *vcpi;
> -       int avail_slots = 63, payload_count = 0;
> +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
>  
>         list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>                 /* Releasing VCPI is always OK-even if the port is gone */
> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> drm_dp_mst_topology_mgr *mgr,
>                 }
>         }
>         drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d
> used=%d\n",
> -                      mgr, mst_state, avail_slots, 63 - avail_slots);
> +                      mgr, mst_state, avail_slots, mgr->total_avail_slots -
> avail_slots);
>  
>         return 0;
>  }
> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state
> *state)
>                         break;
>  
>                 mutex_lock(&mgr->lock);
> +
> +               mgr->start_slot = mst_state->start_slot;
> +               mgr->total_avail_slots = mst_state->total_avail_slots;
> +

this isn't correct - atomic checks aren't allowed to mutate anything besides
the atomic state structure that we're checking since we don't know whether or
not the display state is going to be committed when checking it. If we're
storing state in mgr, that state needs to be written to mgr during the atomic
commit instead of the atomic check.

...but, coming back to this MST code after being gone for a while, I think it
might be time for us to stop worrying about the non-atomic state. Especially
since there's only one driver using it; the legacy radeon.ko; and right now
the plan is either to just drop MST support on radeon.ko or move the MST code
it uses into radeon.ko.Which brings me to say - I think we can drop some of
the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and
drm_dp_find_vcpi_slots I mentioned above). We can then just update the kdocs
for these functions in a separate patch to clarify that now in addition to
being deprecated, these functions are just broken and will eventually be
removed.

So - doing that allows us to get rid of mgr->total_avail_slots and mgr-
>start_slot entirely, just set the slot info in the atomic state during atomic
check, and then just rely on the atomic state for
drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler to me,
does this sound alrght with you?

>                 ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> >mst_primary,
>                                                             mst_state);
>                 mutex_unlock(&mgr->lock);
> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
>         if (!mgr->proposed_vcpis)
>                 return -ENOMEM;
>         set_bit(0, &mgr->payload_mask);
> +       mgr->total_avail_slots = 63;
> +       mgr->start_slot = 1;
>  
>         mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>         if (mst_state == NULL)
>                 return -ENOMEM;
>  
> +       mst_state->total_avail_slots = 63;
> +       mst_state->start_slot = 1;
> +
>         mst_state->mgr = mgr;
>         INIT_LIST_HEAD(&mst_state->vcpis);
>  
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index ddb9231d0309..f8152dfb34ed 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>         struct drm_private_state base;
>         struct list_head vcpis;
>         struct drm_dp_mst_topology_mgr *mgr;
> +       u8 total_avail_slots;
> +       u8 start_slot;
>  };
>  
>  #define to_dp_mst_topology_mgr(x) container_of(x, struct
> drm_dp_mst_topology_mgr, base)
> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>          */
>         int pbn_div;
>  
> +       /**
> +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> +        */
> +       u8 total_avail_slots;
> +
> +       /**
> +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> +        */
> +       u8 start_slot;
> +
>         /**
>          * @funcs: Atomic helper callbacks
>          */
> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>  
>  void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port);
>  
> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> *mst_state, uint8_t link_coding_cap);
>  
>  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>                                 struct drm_dp_mst_port *port);

Bhawanpreet Lakha Oct. 14, 2021, 8:21 p.m. UTC | #9

On 2021-10-13 6:25 p.m., Lyude Paul wrote:
> Some comments below (also, sorry again for the mixup on the last review!)
>
> On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:
>> 8b/10b encoding format requires to reserve the first slot for
>> recording metadata. Real data transmission starts from the second slot,
>> with a total of available 63 slots available.
>>
>> In 128b/132b encoding format, metadata is transmitted separately
>> in LLCP packet before MTP. Real data transmission starts from
>> the first slot, with a total of 64 slots available.
>>
>> v2:
>> * Remove get_mst_link_encoding_cap
>> * Move total/start slots to mst_state, and copy it to mst_mgr in
>> atomic_check
>>
>> Signed-off-by: Fangzhi Zuo <[e-mail]>
>> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>>   drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
>>   include/drm/drm_dp_mst_helper.h               | 13 +++++++
>>   3 files changed, 69 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 5020f2d36fe1..4ad50eb0091a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device
>> *dev,
>>   #if defined(CONFIG_DRM_AMD_DC_DCN)
>>          struct dsc_mst_fairness_vars vars[MAX_PIPES];
>>   #endif
>> +       struct drm_dp_mst_topology_state *mst_state;
>> +       struct drm_dp_mst_topology_mgr *mgr;
>>   
>>          trace_amdgpu_dm_atomic_check_begin(state);
>>   
>> @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device
>> *dev,
>>                  lock_and_validation_needed = true;
>>          }
>>   
>> +#if defined(CONFIG_DRM_AMD_DC_DCN)
>> +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
>> +               struct amdgpu_dm_connector *aconnector;
>> +               struct drm_connector *connector;
>> +               struct drm_connector_list_iter iter;
>> +               u8 link_coding_cap;
>> +
>> +               if (!mgr->mst_state )
>> +                       continue;
> extraneous space
>
>> +
>> +               drm_connector_list_iter_begin(dev, &iter);
>> +               drm_for_each_connector_iter(connector, &iter) {
>> +                       int id = connector->index;
>> +
>> +                       if (id == mst_state->mgr->conn_base_id) {
>> +                               aconnector =
>> to_amdgpu_dm_connector(connector);
>> +                               link_coding_cap =
>> dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
>> +                               drm_dp_mst_update_coding_cap(mst_state,
>> link_coding_cap);
>> +
>> +                               break;
>> +                       }
>> +               }
>> +               drm_connector_list_iter_end(&iter);
>> +
>> +       }
>> +#endif
>>          /**
>>           * Streams and planes are reset when there are changes that affect
>>           * bandwidth. Anything that affects bandwidth needs to go through
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index ad0795afc21c..fb5c47c4cb2e 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
>> drm_dp_mst_topology_mgr *mgr)
>>          struct drm_dp_payload req_payload;
>>          struct drm_dp_mst_port *port;
>>          int i, j;
>> -       int cur_slots = 1;
>> +       int cur_slots = mgr->start_slot;
>>          bool skip;
>>   
>>          mutex_lock(&mgr->payload_lock);
>> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
>> drm_dp_mst_topology_mgr *mgr,
>>          num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>>   
>>          /* max. time slots - one slot for MTP header */
>> -       if (num_slots > 63)
>> +       if (num_slots > mgr->total_avail_slots)
>>                  return -ENOSPC;
> For reasons I will explain a little further in this email, we might want to
> drop this…
>
>>          return num_slots;
>>   }
>> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>>          int ret;
>>   
>>          /* max. time slots - one slot for MTP header */
>> -       if (slots > 63)
>> +       if (slots > mgr->total_avail_slots)
> …and this
>
>>                  return -ENOSPC;
>>   
>>          vcpi->pbn = pbn;
>> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
>> drm_atomic_state *state,
>>   }
>>   EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>>   
>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>> *mst_state, uint8_t link_coding_cap)
> Need some kdocs here
>
>> +{
>> +       if (link_coding_cap == DP_CAP_ANSI_128B132B) {
>> +               mst_state->total_avail_slots = 64;
>> +               mst_state->start_slot = 0;
>> +       }
>> +
>> +       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
>> +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ?
>> "128b/132b":"8b/10b", mst_state->mgr);
> need to fix indenting here, and wrap this to 100 chars
>
>> +}
>> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
>> +
>>   /**
>>    * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>>    * @mgr: manager for this port
>> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>>   
>>          ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>>          if (ret) {
>> -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63
>> ret=%d\n",
>> -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
>> +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d
>> ret=%d\n",
>> +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
>>> total_avail_slots, ret);
>>                  drm_dp_mst_topology_put_port(port);
>>                  goto out;
>>          }
>> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
>> drm_dp_mst_topology_mgr *mgr,
>>                                           struct drm_dp_mst_topology_state
>> *mst_state)
>>   {
>>          struct drm_dp_vcpi_allocation *vcpi;
>> -       int avail_slots = 63, payload_count = 0;
>> +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
>>   
>>          list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>>                  /* Releasing VCPI is always OK-even if the port is gone */
>> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
>> drm_dp_mst_topology_mgr *mgr,
>>                  }
>>          }
>>          drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d
>> used=%d\n",
>> -                      mgr, mst_state, avail_slots, 63 - avail_slots);
>> +                      mgr, mst_state, avail_slots, mgr->total_avail_slots -
>> avail_slots);
>>   
>>          return 0;
>>   }
>> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state
>> *state)
>>                          break;
>>   
>>                  mutex_lock(&mgr->lock);
>> +
>> +               mgr->start_slot = mst_state->start_slot;
>> +               mgr->total_avail_slots = mst_state->total_avail_slots;
>> +
> this isn't correct - atomic checks aren't allowed to mutate anything besides
> the atomic state structure that we're checking since we don't know whether or
> not the display state is going to be committed when checking it. If we're
> storing state in mgr, that state needs to be written to mgr during the atomic
> commit instead of the atomic check.
>
> ...but, coming back to this MST code after being gone for a while, I think it
> might be time for us to stop worrying about the non-atomic state. Especially
> since there's only one driver using it; the legacy radeon.ko; and right now
> the plan is either to just drop MST support on radeon.ko or move the MST code
> it uses into radeon.ko.Which brings me to say - I think we can drop some of
> the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and
> drm_dp_find_vcpi_slots I mentioned above). We can then just update the kdocs
> for these functions in a separate patch to clarify that now in addition to
> being deprecated, these functions are just broken and will eventually be
> removed.
>
> So - doing that allows us to get rid of mgr->total_avail_slots and mgr-
>> start_slot entirely, just set the slot info in the atomic state during atomic
> check, and then just rely on the atomic state for
> drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler to me,
> does this sound alrght with you?

Thanks for the response,

That function is per port (drm_dp_atomic_find_vcpi_slots) so not sure 
how that will work, maybe I don't understand what you mean?

Also we only need to check this inside 
drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a state, 
so we still need to have this on the mgr somehow.

I was thinking we could add the slots(or some DP version indicator) 
inside the drm_connector, and add a parameter to 
drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots) and call it with 
this info via drm_dp_mst_atomic_check().


Bhawan

>
>>                  ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
>>> mst_primary,
>>                                                              mst_state);
>>                  mutex_unlock(&mgr->lock);
>> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>>          if (!mgr->proposed_vcpis)
>>                  return -ENOMEM;
>>          set_bit(0, &mgr->payload_mask);
>> +       mgr->total_avail_slots = 63;
>> +       mgr->start_slot = 1;
>>   
>>          mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>>          if (mst_state == NULL)
>>                  return -ENOMEM;
>>   
>> +       mst_state->total_avail_slots = 63;
>> +       mst_state->start_slot = 1;
>> +
>>          mst_state->mgr = mgr;
>>          INIT_LIST_HEAD(&mst_state->vcpis);
>>   
>> diff --git a/include/drm/drm_dp_mst_helper.h
>> b/include/drm/drm_dp_mst_helper.h
>> index ddb9231d0309..f8152dfb34ed 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>>          struct drm_private_state base;
>>          struct list_head vcpis;
>>          struct drm_dp_mst_topology_mgr *mgr;
>> +       u8 total_avail_slots;
>> +       u8 start_slot;
>>   };
>>   
>>   #define to_dp_mst_topology_mgr(x) container_of(x, struct
>> drm_dp_mst_topology_mgr, base)
>> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>>           */
>>          int pbn_div;
>>   
>> +       /**
>> +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
>> +        */
>> +       u8 total_avail_slots;
>> +
>> +       /**
>> +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
>> +        */
>> +       u8 start_slot;
>> +
>>          /**
>>           * @funcs: Atomic helper callbacks
>>           */
>> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
>> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>>   
>>   void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>> struct drm_dp_mst_port *port);
>>   
>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>> *mst_state, uint8_t link_coding_cap);
>>   
>>   void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>>                                  struct drm_dp_mst_port *port);

Bhawanpreet Lakha Oct. 14, 2021, 8:22 p.m. UTC | #10

Adding Mikita aswell

On 2021-10-14 4:21 p.m., Bhawanpreet Lakha wrote:
>
> On 2021-10-13 6:25 p.m., Lyude Paul wrote:
>> Some comments below (also, sorry again for the mixup on the last 
>> review!)
>>
>> On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:
>>> 8b/10b encoding format requires to reserve the first slot for
>>> recording metadata. Real data transmission starts from the second slot,
>>> with a total of available 63 slots available.
>>>
>>> In 128b/132b encoding format, metadata is transmitted separately
>>> in LLCP packet before MTP. Real data transmission starts from
>>> the first slot, with a total of 64 slots available.
>>>
>>> v2:
>>> * Remove get_mst_link_encoding_cap
>>> * Move total/start slots to mst_state, and copy it to mst_mgr in
>>> atomic_check
>>>
>>> Signed-off-by: Fangzhi Zuo <[e-mail]>
>>> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
>>> ---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>>>   drivers/gpu/drm/drm_dp_mst_topology.c         | 35 
>>> +++++++++++++++----
>>>   include/drm/drm_dp_mst_helper.h               | 13 +++++++
>>>   3 files changed, 69 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 5020f2d36fe1..4ad50eb0091a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct 
>>> drm_device
>>> *dev,
>>>   #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>          struct dsc_mst_fairness_vars vars[MAX_PIPES];
>>>   #endif
>>> +       struct drm_dp_mst_topology_state *mst_state;
>>> +       struct drm_dp_mst_topology_mgr *mgr;
>>>            trace_amdgpu_dm_atomic_check_begin(state);
>>>   @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct 
>>> drm_device
>>> *dev,
>>>                  lock_and_validation_needed = true;
>>>          }
>>>   +#if defined(CONFIG_DRM_AMD_DC_DCN)
>>> +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
>>> +               struct amdgpu_dm_connector *aconnector;
>>> +               struct drm_connector *connector;
>>> +               struct drm_connector_list_iter iter;
>>> +               u8 link_coding_cap;
>>> +
>>> +               if (!mgr->mst_state )
>>> +                       continue;
>> extraneous space
>>
>>> +
>>> +               drm_connector_list_iter_begin(dev, &iter);
>>> +               drm_for_each_connector_iter(connector, &iter) {
>>> +                       int id = connector->index;
>>> +
>>> +                       if (id == mst_state->mgr->conn_base_id) {
>>> +                               aconnector =
>>> to_amdgpu_dm_connector(connector);
>>> +                               link_coding_cap =
>>> dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
>>> +                               drm_dp_mst_update_coding_cap(mst_state,
>>> link_coding_cap);
>>> +
>>> +                               break;
>>> +                       }
>>> +               }
>>> +               drm_connector_list_iter_end(&iter);
>>> +
>>> +       }
>>> +#endif
>>>          /**
>>>           * Streams and planes are reset when there are changes that 
>>> affect
>>>           * bandwidth. Anything that affects bandwidth needs to go 
>>> through
>>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>>> index ad0795afc21c..fb5c47c4cb2e 100644
>>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>>> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
>>> drm_dp_mst_topology_mgr *mgr)
>>>          struct drm_dp_payload req_payload;
>>>          struct drm_dp_mst_port *port;
>>>          int i, j;
>>> -       int cur_slots = 1;
>>> +       int cur_slots = mgr->start_slot;
>>>          bool skip;
>>>            mutex_lock(&mgr->payload_lock);
>>> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
>>> drm_dp_mst_topology_mgr *mgr,
>>>          num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>>>            /* max. time slots - one slot for MTP header */
>>> -       if (num_slots > 63)
>>> +       if (num_slots > mgr->total_avail_slots)
>>>                  return -ENOSPC;
>> For reasons I will explain a little further in this email, we might 
>> want to
>> drop this…
>>
>>>          return num_slots;
>>>   }
>>> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
>>> drm_dp_mst_topology_mgr *mgr,
>>>          int ret;
>>>            /* max. time slots - one slot for MTP header */
>>> -       if (slots > 63)
>>> +       if (slots > mgr->total_avail_slots)
>> …and this
>>
>>>                  return -ENOSPC;
>>>            vcpi->pbn = pbn;
>>> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
>>> drm_atomic_state *state,
>>>   }
>>>   EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>>>   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>>> *mst_state, uint8_t link_coding_cap)
>> Need some kdocs here
>>
>>> +{
>>> +       if (link_coding_cap == DP_CAP_ANSI_128B132B) {
>>> +               mst_state->total_avail_slots = 64;
>>> +               mst_state->start_slot = 0;
>>> +       }
>>> +
>>> +       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
>>> +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ?
>>> "128b/132b":"8b/10b", mst_state->mgr);
>> need to fix indenting here, and wrap this to 100 chars
>>
>>> +}
>>> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
>>> +
>>>   /**
>>>    * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>>>    * @mgr: manager for this port
>>> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
>>> drm_dp_mst_topology_mgr *mgr,
>>>            ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>>>          if (ret) {
>>> -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d 
>>> max=63
>>> ret=%d\n",
>>> -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
>>> +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d 
>>> max=%d
>>> ret=%d\n",
>>> +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
>>>> total_avail_slots, ret);
>>>                  drm_dp_mst_topology_put_port(port);
>>>                  goto out;
>>>          }
>>> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
>>> drm_dp_mst_topology_mgr *mgr,
>>>                                           struct 
>>> drm_dp_mst_topology_state
>>> *mst_state)
>>>   {
>>>          struct drm_dp_vcpi_allocation *vcpi;
>>> -       int avail_slots = 63, payload_count = 0;
>>> +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
>>>            list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>>>                  /* Releasing VCPI is always OK-even if the port is 
>>> gone */
>>> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
>>> drm_dp_mst_topology_mgr *mgr,
>>>                  }
>>>          }
>>>          drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI 
>>> avail=%d
>>> used=%d\n",
>>> -                      mgr, mst_state, avail_slots, 63 - avail_slots);
>>> +                      mgr, mst_state, avail_slots, 
>>> mgr->total_avail_slots -
>>> avail_slots);
>>>            return 0;
>>>   }
>>> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct 
>>> drm_atomic_state
>>> *state)
>>>                          break;
>>>                    mutex_lock(&mgr->lock);
>>> +
>>> +               mgr->start_slot = mst_state->start_slot;
>>> +               mgr->total_avail_slots = mst_state->total_avail_slots;
>>> +
>> this isn't correct - atomic checks aren't allowed to mutate anything 
>> besides
>> the atomic state structure that we're checking since we don't know 
>> whether or
>> not the display state is going to be committed when checking it. If 
>> we're
>> storing state in mgr, that state needs to be written to mgr during 
>> the atomic
>> commit instead of the atomic check.
>>
>> ...but, coming back to this MST code after being gone for a while, I 
>> think it
>> might be time for us to stop worrying about the non-atomic state. 
>> Especially
>> since there's only one driver using it; the legacy radeon.ko; and 
>> right now
>> the plan is either to just drop MST support on radeon.ko or move the 
>> MST code
>> it uses into radeon.ko.Which brings me to say - I think we can drop 
>> some of
>> the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and
>> drm_dp_find_vcpi_slots I mentioned above). We can then just update 
>> the kdocs
>> for these functions in a separate patch to clarify that now in 
>> addition to
>> being deprecated, these functions are just broken and will eventually be
>> removed.
>>
>> So - doing that allows us to get rid of mgr->total_avail_slots and mgr-
>>> start_slot entirely, just set the slot info in the atomic state 
>>> during atomic
>> check, and then just rely on the atomic state for
>> drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler 
>> to me,
>> does this sound alrght with you?
>
> Thanks for the response,
>
> That function is per port (drm_dp_atomic_find_vcpi_slots) so not sure 
> how that will work, maybe I don't understand what you mean?
>
> Also we only need to check this inside 
> drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a 
> state, so we still need to have this on the mgr somehow.
>
> I was thinking we could add the slots(or some DP version indicator) 
> inside the drm_connector, and add a parameter to 
> drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots) and call it with 
> this info via drm_dp_mst_atomic_check().
>
>
> Bhawan
>
>>
>>>                  ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
>>>> mst_primary,
>>> mst_state);
>>>                  mutex_unlock(&mgr->lock);
>>> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
>>> drm_dp_mst_topology_mgr *mgr,
>>>          if (!mgr->proposed_vcpis)
>>>                  return -ENOMEM;
>>>          set_bit(0, &mgr->payload_mask);
>>> +       mgr->total_avail_slots = 63;
>>> +       mgr->start_slot = 1;
>>>            mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>>>          if (mst_state == NULL)
>>>                  return -ENOMEM;
>>>   +       mst_state->total_avail_slots = 63;
>>> +       mst_state->start_slot = 1;
>>> +
>>>          mst_state->mgr = mgr;
>>>          INIT_LIST_HEAD(&mst_state->vcpis);
>>>   diff --git a/include/drm/drm_dp_mst_helper.h
>>> b/include/drm/drm_dp_mst_helper.h
>>> index ddb9231d0309..f8152dfb34ed 100644
>>> --- a/include/drm/drm_dp_mst_helper.h
>>> +++ b/include/drm/drm_dp_mst_helper.h
>>> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>>>          struct drm_private_state base;
>>>          struct list_head vcpis;
>>>          struct drm_dp_mst_topology_mgr *mgr;
>>> +       u8 total_avail_slots;
>>> +       u8 start_slot;
>>>   };
>>>     #define to_dp_mst_topology_mgr(x) container_of(x, struct
>>> drm_dp_mst_topology_mgr, base)
>>> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>>>           */
>>>          int pbn_div;
>>>   +       /**
>>> +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
>>> +        */
>>> +       u8 total_avail_slots;
>>> +
>>> +       /**
>>> +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
>>> +        */
>>> +       u8 start_slot;
>>> +
>>>          /**
>>>           * @funcs: Atomic helper callbacks
>>>           */
>>> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
>>> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>>>     void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr 
>>> *mgr,
>>> struct drm_dp_mst_port *port);
>>>   +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>>> *mst_state, uint8_t link_coding_cap);
>>>     void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr 
>>> *mgr,
>>>                                  struct drm_dp_mst_port *port);

Bhawanpreet Lakha Oct. 15, 2021, 7:43 p.m. UTC | #11

On 2021-10-13 6:25 p.m., Lyude Paul wrote:
> Some comments below (also, sorry again for the mixup on the last review!)
>
> On Tue, 2021-10-12 at 17:58 -0400, Bhawanpreet Lakha wrote:
>> 8b/10b encoding format requires to reserve the first slot for
>> recording metadata. Real data transmission starts from the second slot,
>> with a total of available 63 slots available.
>>
>> In 128b/132b encoding format, metadata is transmitted separately
>> in LLCP packet before MTP. Real data transmission starts from
>> the first slot, with a total of 64 slots available.
>>
>> v2:
>> * Remove get_mst_link_encoding_cap
>> * Move total/start slots to mst_state, and copy it to mst_mgr in
>> atomic_check
>>
>> Signed-off-by: Fangzhi Zuo <[e-mail]>
>> Signed-off-by: Bhawanpreet Lakha <[e-mail]>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++
>>   drivers/gpu/drm/drm_dp_mst_topology.c         | 35 +++++++++++++++----
>>   include/drm/drm_dp_mst_helper.h               | 13 +++++++
>>   3 files changed, 69 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 5020f2d36fe1..4ad50eb0091a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device
>> *dev,
>>   #if defined(CONFIG_DRM_AMD_DC_DCN)
>>          struct dsc_mst_fairness_vars vars[MAX_PIPES];
>>   #endif
>> +       struct drm_dp_mst_topology_state *mst_state;
>> +       struct drm_dp_mst_topology_mgr *mgr;
>>   
>>          trace_amdgpu_dm_atomic_check_begin(state);
>>   
>> @@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device
>> *dev,
>>                  lock_and_validation_needed = true;
>>          }
>>   
>> +#if defined(CONFIG_DRM_AMD_DC_DCN)
>> +       for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
>> +               struct amdgpu_dm_connector *aconnector;
>> +               struct drm_connector *connector;
>> +               struct drm_connector_list_iter iter;
>> +               u8 link_coding_cap;
>> +
>> +               if (!mgr->mst_state )
>> +                       continue;
> extraneous space
>
>> +
>> +               drm_connector_list_iter_begin(dev, &iter);
>> +               drm_for_each_connector_iter(connector, &iter) {
>> +                       int id = connector->index;
>> +
>> +                       if (id == mst_state->mgr->conn_base_id) {
>> +                               aconnector =
>> to_amdgpu_dm_connector(connector);
>> +                               link_coding_cap =
>> dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
>> +                               drm_dp_mst_update_coding_cap(mst_state,
>> link_coding_cap);
>> +
>> +                               break;
>> +                       }
>> +               }
>> +               drm_connector_list_iter_end(&iter);
>> +
>> +       }
>> +#endif
>>          /**
>>           * Streams and planes are reset when there are changes that affect
>>           * bandwidth. Anything that affects bandwidth needs to go through
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index ad0795afc21c..fb5c47c4cb2e 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
>> drm_dp_mst_topology_mgr *mgr)
>>          struct drm_dp_payload req_payload;
>>          struct drm_dp_mst_port *port;
>>          int i, j;
>> -       int cur_slots = 1;
>> +       int cur_slots = mgr->start_slot;
>>          bool skip;
>>   
>>          mutex_lock(&mgr->payload_lock);
>> @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
>> drm_dp_mst_topology_mgr *mgr,
>>          num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>>   
>>          /* max. time slots - one slot for MTP header */
>> -       if (num_slots > 63)
>> +       if (num_slots > mgr->total_avail_slots)
>>                  return -ENOSPC;
> For reasons I will explain a little further in this email, we might want to
> drop this…
>
>>          return num_slots;
>>   }
>> @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>>          int ret;
>>   
>>          /* max. time slots - one slot for MTP header */
>> -       if (slots > 63)
>> +       if (slots > mgr->total_avail_slots)
> …and this
>
>>                  return -ENOSPC;
>>   
>>          vcpi->pbn = pbn;
>> @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
>> drm_atomic_state *state,
>>   }
>>   EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>>   
>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>> *mst_state, uint8_t link_coding_cap)
> Need some kdocs here
>
>> +{
>> +       if (link_coding_cap == DP_CAP_ANSI_128B132B) {
>> +               mst_state->total_avail_slots = 64;
>> +               mst_state->start_slot = 0;
>> +       }
>> +
>> +       DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
>> +                       (link_coding_cap == DP_CAP_ANSI_128B132B) ?
>> "128b/132b":"8b/10b", mst_state->mgr);
> need to fix indenting here, and wrap this to 100 chars
>
>> +}
>> +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
>> +
>>   /**
>>    * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>>    * @mgr: manager for this port
>> @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>>   
>>          ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
>>          if (ret) {
>> -               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63
>> ret=%d\n",
>> -                           DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
>> +               drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d
>> ret=%d\n",
>> +                           DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
>>> total_avail_slots, ret);
>>                  drm_dp_mst_topology_put_port(port);
>>                  goto out;
>>          }
>> @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
>> drm_dp_mst_topology_mgr *mgr,
>>                                           struct drm_dp_mst_topology_state
>> *mst_state)
>>   {
>>          struct drm_dp_vcpi_allocation *vcpi;
>> -       int avail_slots = 63, payload_count = 0;
>> +       int avail_slots = mgr->total_avail_slots, payload_count = 0;
>>   
>>          list_for_each_entry(vcpi, &mst_state->vcpis, next) {
>>                  /* Releasing VCPI is always OK-even if the port is gone */
>> @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
>> drm_dp_mst_topology_mgr *mgr,
>>                  }
>>          }
>>          drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d
>> used=%d\n",
>> -                      mgr, mst_state, avail_slots, 63 - avail_slots);
>> +                      mgr, mst_state, avail_slots, mgr->total_avail_slots -
>> avail_slots);
>>   
>>          return 0;
>>   }
>> @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state
>> *state)
>>                          break;
>>   
>>                  mutex_lock(&mgr->lock);
>> +
>> +               mgr->start_slot = mst_state->start_slot;
>> +               mgr->total_avail_slots = mst_state->total_avail_slots;
>> +
> this isn't correct - atomic checks aren't allowed to mutate anything besides
> the atomic state structure that we're checking since we don't know whether or
> not the display state is going to be committed when checking it. If we're
> storing state in mgr, that state needs to be written to mgr during the atomic
> commit instead of the atomic check.
>
> ...but, coming back to this MST code after being gone for a while, I think it
> might be time for us to stop worrying about the non-atomic state. Especially
> since there's only one driver using it; the legacy radeon.ko; and right now
> the plan is either to just drop MST support on radeon.ko or move the MST code
> it uses into radeon.ko.Which brings me to say - I think we can drop some of
> the hunks I mentioned above (e.g. the changes to drm_dp_init_vcpi and
> drm_dp_find_vcpi_slots I mentioned above). We can then just update the kdocs
> for these functions in a separate patch to clarify that now in addition to
> being deprecated, these functions are just broken and will eventually be
> removed.
>
> So - doing that allows us to get rid of mgr->total_avail_slots and mgr-
>> start_slot entirely, just set the slot info in the atomic state during atomic
> check, and then just rely on the atomic state for
> drm_dp_atomic_find_vcpi_slots() and friends. Which seems much simpler to me,
> does this sound alrght with you?
>
Thanks for the response,

That function is per port so not sure how that will work. Also we only 
need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(), 
which doesn't have a state.

We could add the slots(or some DP version indicator) inside the 
drm_connector, and add a parameter to 
drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with 
this info via drm_dp_mst_atomic_check() and then update the mgr->slot in 
commit.


Bhawan

>>                  ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
>>> mst_primary,
>>                                                              mst_state);
>>                  mutex_unlock(&mgr->lock);
>> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>>          if (!mgr->proposed_vcpis)
>>                  return -ENOMEM;
>>          set_bit(0, &mgr->payload_mask);
>> +       mgr->total_avail_slots = 63;
>> +       mgr->start_slot = 1;
>>   
>>          mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>>          if (mst_state == NULL)
>>                  return -ENOMEM;
>>   
>> +       mst_state->total_avail_slots = 63;
>> +       mst_state->start_slot = 1;
>> +
>>          mst_state->mgr = mgr;
>>          INIT_LIST_HEAD(&mst_state->vcpis);
>>   
>> diff --git a/include/drm/drm_dp_mst_helper.h
>> b/include/drm/drm_dp_mst_helper.h
>> index ddb9231d0309..f8152dfb34ed 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>>          struct drm_private_state base;
>>          struct list_head vcpis;
>>          struct drm_dp_mst_topology_mgr *mgr;
>> +       u8 total_avail_slots;
>> +       u8 start_slot;
>>   };
>>   
>>   #define to_dp_mst_topology_mgr(x) container_of(x, struct
>> drm_dp_mst_topology_mgr, base)
>> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>>           */
>>          int pbn_div;
>>   
>> +       /**
>> +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
>> +        */
>> +       u8 total_avail_slots;
>> +
>> +       /**
>> +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
>> +        */
>> +       u8 start_slot;
>> +
>>          /**
>>           * @funcs: Atomic helper callbacks
>>           */
>> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
>> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>>   
>>   void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>> struct drm_dp_mst_port *port);
>>   
>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>> *mst_state, uint8_t link_coding_cap);
>>   
>>   void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>>                                  struct drm_dp_mst_port *port);

Lyude Paul Oct. 15, 2021, 8:04 p.m. UTC | #12

[snip]

On Thu, 2021-10-14 at 16:21 -0400, Bhawanpreet Lakha wrote:
> 
> Thanks for the response,
> 
> That function is per port (drm_dp_atomic_find_vcpi_slots) so not sure 
> how that will work, maybe I don't understand what you mean?

Yeah - drm_dp_atomic_find_vcpi_slots() is just one part of the atomic helpers
though, we can always add more. JFYI, the main atomic check function for MST
is drm_dp_mst_atomic_check(). So we'd probably just want to add something into
drm_dp_mst_topology_state that handles making sure we go through
drm_dp_vcpi_allocation struct and recalculates everything in it. We might also
want to add an atomic helper to set the new starting slot and slot count in
the atomic state, then go through the current atomic topology state and ensure
that we add any CRTCs that would need full modesets as a result of that info
changing.

> 
> Also we only need to check this inside 
> drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a state, 
> so we still need to have this on the mgr somehow.

It does, we pass drm_dp_mst_topology_state to it. So we could just add these
fields there.

> 
> I was thinking we could add the slots(or some DP version indicator) 
> inside the drm_connector, and add a parameter to 
> drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots) and call it with 
> this info via drm_dp_mst_atomic_check().

So before I continue: I should note that some of the code in MST goes against
what I'm about to say, in particular a lot of the payload updating functions
in MST that keep their payload state outside of drm_dp_mst_topology_state and
friends, but that's because some of this code is old and needs to be updated
anyway (and some of it was actually just being kept around because we were
worried about breaking radeon.ko, the only driver relying on behavior from the
non-atomic paths in our topology helper). Also - I'm not sure what your prior
experience is with modesetting in the linux kernel so my apologies if I'm
explaining anything you already understand here.

Anyway-drm_connector wouldn't be the right place to put it because it's not
part of the atomic state. The reason we have atomic modesetting in the first
place is so that we can come up with a new display state, and then have the
kernel verify the configurations in that new state and potentially reject it
if we tried to program something that wouldn't have worked in hardware. As
well, having it in drm_connector would mean that it wouldn't be safe to access
unless we've somehow locked the drm_connector. drm_connector_state would be
'safe' to have this data in, but considering that we already have a private
atomic state object for MST (drm_dp_mst_topology_state) it doesn't make much
sense to keep MST info elsewhere.

> 
> 
> Bhawan
> 
> > 
> > >                  ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> > > > mst_primary,
> > >                                                              mst_state);
> > >                  mutex_unlock(&mgr->lock);
> > > @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >          if (!mgr->proposed_vcpis)
> > >                  return -ENOMEM;
> > >          set_bit(0, &mgr->payload_mask);
> > > +       mgr->total_avail_slots = 63;
> > > +       mgr->start_slot = 1;
> > >   
> > >          mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> > >          if (mst_state == NULL)
> > >                  return -ENOMEM;
> > >   
> > > +       mst_state->total_avail_slots = 63;
> > > +       mst_state->start_slot = 1;
> > > +
> > >          mst_state->mgr = mgr;
> > >          INIT_LIST_HEAD(&mst_state->vcpis);
> > >   
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index ddb9231d0309..f8152dfb34ed 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
> > >          struct drm_private_state base;
> > >          struct list_head vcpis;
> > >          struct drm_dp_mst_topology_mgr *mgr;
> > > +       u8 total_avail_slots;
> > > +       u8 start_slot;
> > >   };
> > >   
> > >   #define to_dp_mst_topology_mgr(x) container_of(x, struct
> > > drm_dp_mst_topology_mgr, base)
> > > @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
> > >           */
> > >          int pbn_div;
> > >   
> > > +       /**
> > > +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> > > +        */
> > > +       u8 total_avail_slots;
> > > +
> > > +       /**
> > > +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> > > +        */
> > > +       u8 start_slot;
> > > +
> > >          /**
> > >           * @funcs: Atomic helper callbacks
> > >           */
> > > @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> > > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > >   
> > >   void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> > > struct drm_dp_mst_port *port);
> > >   
> > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > *mst_state, uint8_t link_coding_cap);
> > >   
> > >   void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > >                                  struct drm_dp_mst_port *port);
>

Lyude Paul Oct. 15, 2021, 8:41 p.m. UTC | #13

[more snip]

On Fri, 2021-10-15 at 15:43 -0400, Bhawanpreet Lakha wrote:
> > 
> Thanks for the response,
> 
> That function is per port so not sure how that will work. Also we only 
> need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(), 
> which doesn't have a state.
> 
> We could add the slots(or some DP version indicator) inside the 
> drm_connector, and add a parameter to 
> drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with 
> this info via drm_dp_mst_atomic_check() and then update the mgr->slot in 
> commit.

TBH - I think we can actually just get away with having all of this info in
drm_dp_mst_topology_state

> 
> 
> Bhawan
> 
> > >                  ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> > > > mst_primary,
> > >                                                              mst_state);
> > >                  mutex_unlock(&mgr->lock);
> > > @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >          if (!mgr->proposed_vcpis)
> > >                  return -ENOMEM;
> > >          set_bit(0, &mgr->payload_mask);
> > > +       mgr->total_avail_slots = 63;
> > > +       mgr->start_slot = 1;
> > >   
> > >          mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> > >          if (mst_state == NULL)
> > >                  return -ENOMEM;
> > >   
> > > +       mst_state->total_avail_slots = 63;
> > > +       mst_state->start_slot = 1;
> > > +
> > >          mst_state->mgr = mgr;
> > >          INIT_LIST_HEAD(&mst_state->vcpis);
> > >   
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index ddb9231d0309..f8152dfb34ed 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
> > >          struct drm_private_state base;
> > >          struct list_head vcpis;
> > >          struct drm_dp_mst_topology_mgr *mgr;
> > > +       u8 total_avail_slots;
> > > +       u8 start_slot;
> > >   };
> > >   
> > >   #define to_dp_mst_topology_mgr(x) container_of(x, struct
> > > drm_dp_mst_topology_mgr, base)
> > > @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
> > >           */
> > >          int pbn_div;
> > >   
> > > +       /**
> > > +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> > > +        */
> > > +       u8 total_avail_slots;
> > > +
> > > +       /**
> > > +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> > > +        */
> > > +       u8 start_slot;
> > > +
> > >          /**
> > >           * @funcs: Atomic helper callbacks
> > >           */
> > > @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> > > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > >   
> > >   void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> > > struct drm_dp_mst_port *port);
> > >   
> > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > *mst_state, uint8_t link_coding_cap);
> > >   
> > >   void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > >                                  struct drm_dp_mst_port *port);
>

Bhawanpreet Lakha Oct. 18, 2021, 7:47 p.m. UTC | #14

I understand the mst_state argument its just that most of the mst 
functions are using mst_mgr/port structs and there is no easy way to 
extract the mst_state using mgr/port in these places 
(drm_dp_update_payload_part1, drm_dp_mst_allocate_vcpi, drm_dp_init_vcpi 
etc) where we need the slot info.

So either we need to keep a copy of the slots in the mgr because that's 
what most of the code is using right now or pass around the atomic state 
to get the mgr->state mapping. (I don't have much experience with the 
mst code so maybe I am missing some key detail here?)


Thanks,

Bhawan


On 2021-10-15 4:41 p.m., Lyude Paul wrote:
> [more snip]
>
> On Fri, 2021-10-15 at 15:43 -0400, Bhawanpreet Lakha wrote:
>> Thanks for the response,
>>
>> That function is per port so not sure how that will work. Also we only
>> need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(),
>> which doesn't have a state.
>>
>> We could add the slots(or some DP version indicator) inside the
>> drm_connector, and add a parameter to
>> drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with
>> this info via drm_dp_mst_atomic_check() and then update the mgr->slot in
>> commit.
> TBH - I think we can actually just get away with having all of this info in
> drm_dp_mst_topology_state
>
>>
>> Bhawan
>>
>>>>                   ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
>>>>> mst_primary,
>>>>                                                               mst_state);
>>>>                   mutex_unlock(&mgr->lock);
>>>> @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
>>>> drm_dp_mst_topology_mgr *mgr,
>>>>           if (!mgr->proposed_vcpis)
>>>>                   return -ENOMEM;
>>>>           set_bit(0, &mgr->payload_mask);
>>>> +       mgr->total_avail_slots = 63;
>>>> +       mgr->start_slot = 1;
>>>>    
>>>>           mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>>>>           if (mst_state == NULL)
>>>>                   return -ENOMEM;
>>>>    
>>>> +       mst_state->total_avail_slots = 63;
>>>> +       mst_state->start_slot = 1;
>>>> +
>>>>           mst_state->mgr = mgr;
>>>>           INIT_LIST_HEAD(&mst_state->vcpis);
>>>>    
>>>> diff --git a/include/drm/drm_dp_mst_helper.h
>>>> b/include/drm/drm_dp_mst_helper.h
>>>> index ddb9231d0309..f8152dfb34ed 100644
>>>> --- a/include/drm/drm_dp_mst_helper.h
>>>> +++ b/include/drm/drm_dp_mst_helper.h
>>>> @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
>>>>           struct drm_private_state base;
>>>>           struct list_head vcpis;
>>>>           struct drm_dp_mst_topology_mgr *mgr;
>>>> +       u8 total_avail_slots;
>>>> +       u8 start_slot;
>>>>    };
>>>>    
>>>>    #define to_dp_mst_topology_mgr(x) container_of(x, struct
>>>> drm_dp_mst_topology_mgr, base)
>>>> @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
>>>>            */
>>>>           int pbn_div;
>>>>    
>>>> +       /**
>>>> +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
>>>> +        */
>>>> +       u8 total_avail_slots;
>>>> +
>>>> +       /**
>>>> +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
>>>> +        */
>>>> +       u8 start_slot;
>>>> +
>>>>           /**
>>>>            * @funcs: Atomic helper callbacks
>>>>            */
>>>> @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
>>>> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>>>>    
>>>>    void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>>>> struct drm_dp_mst_port *port);
>>>>    
>>>> +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
>>>> *mst_state, uint8_t link_coding_cap);
>>>>    
>>>>    void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>>>>                                   struct drm_dp_mst_port *port);

Lyude Paul Oct. 18, 2021, 11:17 p.m. UTC | #15

First off, sidenote: I wonder if we even need total_avail_slots and
could just use start_slot. Anyway, more productive comments below:

On Mon, 2021-10-18 at 15:47 -0400, Bhawanpreet Lakha wrote:
> I understand the mst_state argument its just that most of the mst
> functions are using mst_mgr/port structs and there is no easy way to
> extract the mst_state using mgr/port in these places
> (drm_dp_update_payload_part1, drm_dp_mst_allocate_vcpi, drm_dp_init_vcpi
> etc) where we need the slot info.
>
Ahh, I see why this might be confusing. I think surprisingly though,
this actually should probably be OK. Mostly, two of these functions
don't actually need the slot count and one I think I have a workaround
for:

* drm_dp_init_vcpi() - This function does use the total slot count here:

    if (slots > 63)
            return -ENOSPC;

  However, drm_dp_init_vcpi() assigns the current payload which means
  it's called by the driver at commit time, not atomic check time. Since
  atomic commits are only allowed to happen after we've successfully
  tested the state in question, we aren't allowed to fail them in the
  middle of a commit - which is definitely what we're doing in
  drm_dp_init_vcpi(). So, I'd actually say we should either totally
  ignore this bit of drm_dp_init_vcpi() or preferably, just entirely
  drop it in a prerequisite patch.
  (If you aren't familiar with atomic modesetting, the reason we "can't"
  just fail in the middle of committing a new atomic state is because we
  may very well have already committed that state to hardware partially.
  So, there's no nice way of aborting at that point without seriously
  complicating things - hence the need for having an atomic check before
  commits)

* drm_dp_mst_allocate_vcpi() - this seems to only be an issue because of
  drm_dp_init_vcpi(), and we additionally print the maximum number of
  slots in a drm_dbg_kms() message:

    drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
                …

  Since we should have already decided
  all of the new payloads by the time we're in drm_dp_mst_allocate_vcpi
  (again, since we're in atomic commit), that's probably not the best
  place for us to print that anyway. So, I think we'd be fine just
  dropping the "max=63" from that string.

* drm_dp_update_payload_part1() - This one does need the current slot
  count, you're right. I would normally say we should just fix this
  function and move the payload info over to the atomic state, which is
  the eventual plan, but doing that would require dealing with the
  radeon.ko MST mess which is probably too much to ask for with
  something as simple as this patch. I think I know a workaround though:
  Let's keep this new slot info (e.g. num_slots, and possibly
  total_avail_slots if we keep that) in the MST atomic state, _but_ as a
  temporary solution we can instead add a start_slot argument to
  drm_dp_update_payload_part1(). That way we have an easy solution for
  making sure radeon still compiles, and atomic drivers can just extract
  the start_slot info themselves from the topology state and pass it
  into drm_dp_update_payload_part1(). Then I can get rid of that
  start_slots argument at a later date when I've started moving all of
  the payload info for MST into the atomic state.
  If we do this solution though, we should definitely document in the
  patch and in the kdocs for drm_dp_update_payload_part1() that passing
  the start slot is a temporary workaround for non-atomic drivers, and
  will be removed when non-atomic portions of the MST helpers have been
  moved out of helpers and into atomic state.

Does this sound good? There's a lot of moving pieces here so hopefully I
didn't miss anything

> So either we need to keep a copy of the slots in the mgr because that's
> what most of the code is using right now or pass around the atomic state
> to get the mgr->state mapping. (I don't have much experience with the
> mst code so maybe I am missing some key detail here?)

Worst case scenario, if there was something I missed that implies we DO
need access to a mgr->state mapping, I might be OK with us using that in
the interim for these patches. I don't think we need that quite yet as
far as I can tell though.

>
>
> Thanks,
>
> Bhawan
>
>
> On 2021-10-15 4:41 p.m., Lyude Paul wrote:
> > [more snip]
> >
> > On Fri, 2021-10-15 at 15:43 -0400, Bhawanpreet Lakha wrote:
> > > Thanks for the response,
> > >
> > > That function is per port so not sure how that will work. Also we only
> > > need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(),
> > > which doesn't have a state.
> > >
> > > We could add the slots(or some DP version indicator) inside the
> > > drm_connector, and add a parameter to
> > > drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots)? and call it with
> > > this info via drm_dp_mst_atomic_check() and then update the mgr->slot in
> > > commit.
> > TBH - I think we can actually just get away with having all of this info
> > in
> > drm_dp_mst_topology_state
> >
> > >
> > > Bhawan
> > >
> > > > >                   ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> > > > > > mst_primary,
> > > > >                                                              
> > > > > mst_state);
> > > > >                   mutex_unlock(&mgr->lock);
> > > > > @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> > > > > drm_dp_mst_topology_mgr *mgr,
> > > > >           if (!mgr->proposed_vcpis)
> > > > >                   return -ENOMEM;
> > > > >           set_bit(0, &mgr->payload_mask);
> > > > > +       mgr->total_avail_slots = 63;
> > > > > +       mgr->start_slot = 1;
> > > > >   
> > > > >           mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> > > > >           if (mst_state == NULL)
> > > > >                   return -ENOMEM;
> > > > >   
> > > > > +       mst_state->total_avail_slots = 63;
> > > > > +       mst_state->start_slot = 1;
> > > > > +
> > > > >           mst_state->mgr = mgr;
> > > > >           INIT_LIST_HEAD(&mst_state->vcpis);
> > > > >   
> > > > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > > > b/include/drm/drm_dp_mst_helper.h
> > > > > index ddb9231d0309..f8152dfb34ed 100644
> > > > > --- a/include/drm/drm_dp_mst_helper.h
> > > > > +++ b/include/drm/drm_dp_mst_helper.h
> > > > > @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
> > > > >           struct drm_private_state base;
> > > > >           struct list_head vcpis;
> > > > >           struct drm_dp_mst_topology_mgr *mgr;
> > > > > +       u8 total_avail_slots;
> > > > > +       u8 start_slot;
> > > > >    };
> > > > >   
> > > > >    #define to_dp_mst_topology_mgr(x) container_of(x, struct
> > > > > drm_dp_mst_topology_mgr, base)
> > > > > @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
> > > > >            */
> > > > >           int pbn_div;
> > > > >   
> > > > > +       /**
> > > > > +        * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> > > > > +        */
> > > > > +       u8 total_avail_slots;
> > > > > +
> > > > > +       /**
> > > > > +        * @start_slot: 1 for 8b/10b, 0 for 128/132b
> > > > > +        */
> > > > > +       u8 start_slot;
> > > > > +
> > > > >           /**
> > > > >            * @funcs: Atomic helper callbacks
> > > > >            */
> > > > > @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> > > > > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > > > >   
> > > > >    void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr
> > > > > *mgr,
> > > > > struct drm_dp_mst_port *port);
> > > > >   
> > > > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > > > *mst_state, uint8_t link_coding_cap);
> > > > >   
> > > > >    void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr
> > > > > *mgr,
> > > > >                                   struct drm_dp_mst_port *port);
>

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

Patch

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 5020f2d36fe1..4ad50eb0091a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10612,6 +10612,8 @@  static int amdgpu_dm_atomic_check(struct drm_device *dev,
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 	struct dsc_mst_fairness_vars vars[MAX_PIPES];
 #endif
+	struct drm_dp_mst_topology_state *mst_state;
+	struct drm_dp_mst_topology_mgr *mgr;
 
 	trace_amdgpu_dm_atomic_check_begin(state);
 
@@ -10819,6 +10821,32 @@  static int amdgpu_dm_atomic_check(struct drm_device *dev,
 		lock_and_validation_needed = true;
 	}
 
+#if defined(CONFIG_DRM_AMD_DC_DCN)
+	for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+		struct amdgpu_dm_connector *aconnector;
+		struct drm_connector *connector;
+		struct drm_connector_list_iter iter;
+		u8 link_coding_cap;
+
+		if (!mgr->mst_state )
+			continue;
+
+		drm_connector_list_iter_begin(dev, &iter);
+		drm_for_each_connector_iter(connector, &iter) {
+			int id = connector->index;
+
+			if (id == mst_state->mgr->conn_base_id) {
+				aconnector = to_amdgpu_dm_connector(connector);
+				link_coding_cap = dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
+				drm_dp_mst_update_coding_cap(mst_state, link_coding_cap);
+
+				break;
+			}
+		}
+		drm_connector_list_iter_end(&iter);
+
+	}
+#endif
 	/**
 	 * Streams and planes are reset when there are changes that affect
 	 * bandwidth. Anything that affects bandwidth needs to go through
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index ad0795afc21c..fb5c47c4cb2e 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3368,7 +3368,7 @@  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
 	struct drm_dp_payload req_payload;
 	struct drm_dp_mst_port *port;
 	int i, j;
-	int cur_slots = 1;
+	int cur_slots = mgr->start_slot;
 	bool skip;
 
 	mutex_lock(&mgr->payload_lock);
@@ -4321,7 +4321,7 @@  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
 	num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
 	/* max. time slots - one slot for MTP header */
-	if (num_slots > 63)
+	if (num_slots > mgr->total_avail_slots)
 		return -ENOSPC;
 	return num_slots;
 }
@@ -4333,7 +4333,7 @@  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 	int ret;
 
 	/* max. time slots - one slot for MTP header */
-	if (slots > 63)
+	if (slots > mgr->total_avail_slots)
 		return -ENOSPC;
 
 	vcpi->pbn = pbn;
@@ -4507,6 +4507,18 @@  int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
 
+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap)
+{
+	if (link_coding_cap == DP_CAP_ANSI_128B132B) {
+		mst_state->total_avail_slots = 64;
+		mst_state->start_slot = 0;
+	}
+
+	DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
+			(link_coding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b", mst_state->mgr);
+}
+EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
+
 /**
  * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
  * @mgr: manager for this port
@@ -4538,8 +4550,8 @@  bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 
 	ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
 	if (ret) {
-		drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
-			    DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
+		drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=%d ret=%d\n",
+			    DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->total_avail_slots, ret);
 		drm_dp_mst_topology_put_port(port);
 		goto out;
 	}
@@ -5226,7 +5238,7 @@  drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,
 					 struct drm_dp_mst_topology_state *mst_state)
 {
 	struct drm_dp_vcpi_allocation *vcpi;
-	int avail_slots = 63, payload_count = 0;
+	int avail_slots = mgr->total_avail_slots, payload_count = 0;
 
 	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
 		/* Releasing VCPI is always OK-even if the port is gone */
@@ -5255,7 +5267,7 @@  drm_dp_mst_atomic_check_vcpi_alloc_limit(struct drm_dp_mst_topology_mgr *mgr,
 		}
 	}
 	drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
-		       mgr, mst_state, avail_slots, 63 - avail_slots);
+		       mgr, mst_state, avail_slots, mgr->total_avail_slots - avail_slots);
 
 	return 0;
 }
@@ -5421,6 +5433,10 @@  int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
 			break;
 
 		mutex_lock(&mgr->lock);
+
+		mgr->start_slot = mst_state->start_slot;
+		mgr->total_avail_slots = mst_state->total_avail_slots;
+
 		ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr->mst_primary,
 							    mst_state);
 		mutex_unlock(&mgr->lock);
@@ -5527,11 +5543,16 @@  int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
 	if (!mgr->proposed_vcpis)
 		return -ENOMEM;
 	set_bit(0, &mgr->payload_mask);
+	mgr->total_avail_slots = 63;
+	mgr->start_slot = 1;
 
 	mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
 	if (mst_state == NULL)
 		return -ENOMEM;
 
+	mst_state->total_avail_slots = 63;
+	mst_state->start_slot = 1;
+
 	mst_state->mgr = mgr;
 	INIT_LIST_HEAD(&mst_state->vcpis);
 
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index ddb9231d0309..f8152dfb34ed 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -554,6 +554,8 @@  struct drm_dp_mst_topology_state {
 	struct drm_private_state base;
 	struct list_head vcpis;
 	struct drm_dp_mst_topology_mgr *mgr;
+	u8 total_avail_slots;
+	u8 start_slot;
 };
 
 #define to_dp_mst_topology_mgr(x) container_of(x, struct drm_dp_mst_topology_mgr, base)
@@ -661,6 +663,16 @@  struct drm_dp_mst_topology_mgr {
 	 */
 	int pbn_div;
 
+	/**
+	 * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
+	 */
+	u8 total_avail_slots;
+
+	/**
+	 * @start_slot: 1 for 8b/10b, 0 for 128/132b
+	 */
+	u8 start_slot;
+
 	/**
 	 * @funcs: Atomic helper callbacks
 	 */
@@ -806,6 +818,7 @@  int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
 
 void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
 
+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state, uint8_t link_coding_cap);
 
 void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 				struct drm_dp_mst_port *port);