Re: [PATCH] drm: Update MST First Link Slot Information Based on Encoding Format

[Lyude Paul <lyude@redhat.com>]

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 <Jerry.Zuo@amd.com>
> ---
>  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(&mgr->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, &port->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, &mst_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, &mgr->payload_mask);
> +       mgr->first_link_total_avail_slots = 63;
> +       mgr->first_link_start_slot = 1;
>  
>         mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>         if (mst_state == NULL)
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index bd1c39907b92..f4310b3705e7 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -653,6 +653,13 @@ struct drm_dp_mst_topology_mgr {
>          */
>         int pbn_div;
>  
> +       /**
> +        * @first_link_total_avail_slots: frist link total slots available.
> +        * @first_link_start_slot: start slot index for real data
> transmission.
> +        */

These kernel docs are also broken, you can't define two variables in the same
kernel doc comment if it's embedded in the struct, there has to be one kernel
doc comment per-member. Attempting to generate documentation with make htmldocs
even mentions this:

./include/drm/drm_dp_mst_helper.h:770: warning: Function parameter or member
'first_link_start_slot' not described in 'drm_dp_mst_topology_mgr'

> +       u8 first_link_total_avail_slots;
> +       u8 first_link_start_slot;
> +
>         /**
>          * @funcs: Atomic helper callbacks
>          */
> @@ -795,6 +802,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_first_link_slot_info(struct drm_dp_mst_topology_mgr
> *mgr, uint8_t encoding_format);
>  
>  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>                                 struct drm_dp_mst_port *port);

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel