Re: [PATCH] drm: Update MST First Link Slot Information Based on Encoding Format
On Thu, 2021-03-25 at 23:14 -0400, Fangzhi Zuo 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.
>
> Update the slot information after link detect.
>
> Signed-off-by: Fangzhi Zuo
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 48 ++-
> include/drm/drm_dp_mst_helper.h | 8 +
> 2 files changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 42a0c6888c33..577ed4224778 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3382,7 +3382,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->first_link_start_slot;
>
> mutex_lock(>payload_lock);
> for (i = 0; i < mgr->max_payloads; i++) {
> @@ -4302,8 +4302,13 @@ 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)
> + /**
> + * first_link_total_avail_slots: max. time slots
> + * first slot reserved for MTP header in 8b/10b,
> + * but not required for 128b/132b
> + */
> +
> + if (num_slots > mgr->first_link_total_avail_slots)
This is deprecated code, as indicated in the comments for this function. We
really shouldn't be updating this imho.
> return -ENOSPC;
> return num_slots;
> }
> @@ -4314,8 +4319,12 @@ 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)
> + /**
> + * first_link_total_avail_slots: max. time slots
> + * first slot reserved for MTP header in 8b/10b,
> + * but not required for 128b/132b
> + */
> + if (slots > mgr->first_link_total_avail_slots)
why are these kernel docs? putting kdocs in the middle of a function doesn't do
anything
> return -ENOSPC;
>
> vcpi->pbn = pbn;
> @@ -4488,6 +4497,25 @@ int drm_dp_atomic_release_vcpi_slots(struct
> drm_atomic_state *state,
> }
> EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
>
> +/*
> + * drm_dp_mst_update_first_link_slot_info()
> + * update the first link's total available slots and starting slot
> + * @mgr: manager to store the slot info.
> + * @encoding_format: detected link encoding format
These kernel docs aren't properly formatted, follow the examples in the rest of
the file:
/**
* function() - summary
* @mgr: something something
* …
*/
> + */
> +void drm_dp_mst_update_first_link_slot_info(
> + struct drm_dp_mst_topology_mgr *mgr, uint8_t encoding_format)
Use u8, the kernel discourages using the longer typenames outside of uapi
headers. Also this isn't indented correctly - the kernel doesn't break at the
starting parenthesis for function definitions. This should be something like
drm_dp_mst_update_first_link_slot_info(struct drm_dp_mst_topology_mgr *mgr,
u8 encoding_format)
Also, where is this function actually used? If amdgpu is intending to use this,
it really should be in the same series that this is introduced in so that we can
tell if these helpers are what we really want here or not
> +{
> + if (encoding_format == DP_CAP_ANSI_128B132B) {
> + mgr->first_link_total_avail_slots = 64;
> + mgr->first_link_start_slot = 0;
> + }
> + DRM_DEBUG_KMS("%s encoding format on 0x%p -> total %d slots, start at
> slot %d\n",
> + (encoding_format == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b",
> + mgr, mgr->first_link_total_avail_slots, mgr-
> >first_link_start_slot);
the indenting here is entirely broken - please follow the same style as the rest
of the file. Line continuations need to start at the beginning parenthesis, but
the lines after DRM_DEBUG_KMS() don't have any additional indenting here.
> +}
> +EXPORT_SYMBOL(drm_dp_mst_update_first_link_slot_info);
> +
> /**
> * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> * @mgr: manager for this port
> @@ -4518,8 +4546,8 @@ bool drm_dp_mst_allocate_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>
> ret = drm_dp_init_vcpi(mgr, >vcpi, pbn, slots);
> if (ret) {
> - DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n",
> -
[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.
Update the slot information after link detect.
Signed-off-by: Fangzhi Zuo
---
drivers/gpu/drm/drm_dp_mst_topology.c | 48 ++-
include/drm/drm_dp_mst_helper.h | 8 +
2 files changed, 47 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 42a0c6888c33..577ed4224778 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3382,7 +3382,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->first_link_start_slot;
mutex_lock(>payload_lock);
for (i = 0; i < mgr->max_payloads; i++) {
@@ -4302,8 +4302,13 @@ 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)
+ /**
+* first_link_total_avail_slots: max. time slots
+* first slot reserved for MTP header in 8b/10b,
+* but not required for 128b/132b
+*/
+
+ if (num_slots > mgr->first_link_total_avail_slots)
return -ENOSPC;
return num_slots;
}
@@ -4314,8 +4319,12 @@ 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)
+ /**
+* first_link_total_avail_slots: max. time slots
+* first slot reserved for MTP header in 8b/10b,
+* but not required for 128b/132b
+*/
+ if (slots > mgr->first_link_total_avail_slots)
return -ENOSPC;
vcpi->pbn = pbn;
@@ -4488,6 +4497,25 @@ int drm_dp_atomic_release_vcpi_slots(struct
drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
+/*
+ * drm_dp_mst_update_first_link_slot_info()
+ * update the first link's total available slots and starting slot
+ * @mgr: manager to store the slot info.
+ * @encoding_format: detected link encoding format
+ */
+void drm_dp_mst_update_first_link_slot_info(
+ struct drm_dp_mst_topology_mgr *mgr, uint8_t encoding_format)
+{
+ if (encoding_format == DP_CAP_ANSI_128B132B) {
+ mgr->first_link_total_avail_slots = 64;
+ mgr->first_link_start_slot = 0;
+ }
+ DRM_DEBUG_KMS("%s encoding format on 0x%p -> total %d slots, start at
slot %d\n",
+ (encoding_format == DP_CAP_ANSI_128B132B) ? "128b/132b":"8b/10b",
+ mgr, mgr->first_link_total_avail_slots,
mgr->first_link_start_slot);
+}
+EXPORT_SYMBOL(drm_dp_mst_update_first_link_slot_info);
+
/**
* drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
* @mgr: manager for this port
@@ -4518,8 +4546,8 @@ bool drm_dp_mst_allocate_vcpi(struct
drm_dp_mst_topology_mgr *mgr,
ret = drm_dp_init_vcpi(mgr, >vcpi, pbn, slots);
if (ret) {
- DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n",
- DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
+ DRM_DEBUG_KMS("failed to init vcpi slots=%d max=%d ret=%d\n",
+ DIV_ROUND_UP(pbn, mgr->pbn_div),
mgr->first_link_total_avail_slots, ret);
drm_dp_mst_topology_put_port(port);
goto out;
}
@@ -5162,7 +5190,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->first_link_total_avail_slots, payload_count = 0;
list_for_each_entry(vcpi, _state->vcpis, next) {
/* Releasing VCPI is always OK-even if the port is gone */
@@ -5191,7 +5219,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
drm_dp_mst_topology_mgr *mgr,
}
DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d used=%d\n",
mgr, mst_state, avail_slots,
-63 - avail_slots);
+mgr->first_link_total_avail_slots - avail_slots);
return 0;
}
@@ -5455,6 +5483,8 @@ int drm_dp_mst_topology_mgr_init(struct
drm_dp_mst_topology_mgr *mgr,
if (!mgr->proposed_vcpis)
return -ENOMEM;
set_bit(0, >payload_mask);
+ mgr->first_link_total_avail_slots = 63;
+
