diff mbox series

[1/2] drm: Update MST First Link Slot Information Based on Encoding Format

Message ID 20210827234322.2740301-2-Jerry.Zuo@amd.com (mailing list archive)
State New, archived
Headers show
Series Update 128b/132b MST Slot Information | expand

Commit Message

Zuo, Jerry Aug. 27, 2021, 11:43 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.

Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 27 ++++++++++++++++++++-------
 include/drm/drm_dp_mst_helper.h       |  9 +++++++++
 2 files changed, 29 insertions(+), 7 deletions(-)

Comments

kernel test robot Aug. 28, 2021, 4:09 a.m. UTC | #1
Hi Fangzhi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master v5.14-rc7 next-20210827]
[cannot apply to drm/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/Fangzhi-Zuo/Update-128b-132b-MST-Slot-Information/20210828-082044
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: parisc-randconfig-r002-20210827 (attached as .config)
compiler: hppa-linux-gcc (GCC) 11.2.0
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/8232240b519fadd6ad9eb814925e513a15407474
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Fangzhi-Zuo/Update-128b-132b-MST-Slot-Information/20210828-082044
        git checkout 8232240b519fadd6ad9eb814925e513a15407474
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_dp_mst_topology.c:4512:6: warning: no previous prototype for 'drm_dp_mst_update_encoding_cap' [-Wmissing-prototypes]
    4512 | void drm_dp_mst_update_encoding_cap(struct drm_dp_mst_topology_mgr *mgr, uint8_t link_encoding_cap)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/drm_dp_mst_update_encoding_cap +4512 drivers/gpu/drm/drm_dp_mst_topology.c

  4511	
> 4512	void drm_dp_mst_update_encoding_cap(struct drm_dp_mst_topology_mgr *mgr, uint8_t link_encoding_cap)
  4513	{
  4514		if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
  4515			mgr->total_avail_slots = 64;
  4516			mgr->start_slot = 0;
  4517		}
  4518		DRM_DEBUG_KMS("%s encoding format determined\n",
  4519			      (link_encoding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b" : "8b/10b");
  4520	}
  4521	EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
  4522	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Lyude Paul Aug. 30, 2021, 8 p.m. UTC | #2
On Fri, 2021-08-27 at 19:43 -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.
> 
> Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 27 ++++++++++++++++++++-------
>  include/drm/drm_dp_mst_helper.h       |  9 +++++++++
>  2 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 86d13d6bc463..30544801d2b8 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3370,7 +3370,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);
> @@ -4323,7 +4323,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;
>  }
> @@ -4335,7 +4335,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;
> @@ -4509,6 +4509,17 @@ 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_encoding_cap(struct drm_dp_mst_topology_mgr *mgr,
> uint8_t link_encoding_cap)
> +{
> +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
> +               mgr->total_avail_slots = 64;
> +               mgr->start_slot = 0;
> +       }
> +       DRM_DEBUG_KMS("%s encoding format determined\n",
> +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ?
> "128b/132b" : "8b/10b");
> +}
> +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
> +

This seems to be missing kdocs, can you fix that?

Also - I'm not convinced this is all of the work we have to do, as there's no
locking taking place here in this function. If we're changing the number of
available VCPI slots that we have, we need to be able to factor that into the
atomic check logic which means that we can't rely on mgr->* for any kind of
data that isn't guaranteed to remain consistent throughout the lifetime of the
driver or topology. (Note that some of the old MST code didn't follow this
logic, so I wouldn't be surprised if there's still exceptions to this we need
to clean up).

Note that I still expect we'll have to keep some sort of track of the current
total slot count in the topology mgr, but that should be reflecting the
currently programmed state and not be relied on from our atomic check.

IMHO - the correct way we should go about adding support for this is to add
something into drm_dp_mst_topology_state and integrate this into the atomic
check helpers.

>  /**
>   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
>   * @mgr: manager for this port
> @@ -4540,8 +4551,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;
>         }
> @@ -5228,7 +5239,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 */
> @@ -5257,7 +5268,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;
>  }
> @@ -5529,6 +5540,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->total_avail_slots = 63;
> +       mgr->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 ddb9231d0309..eac5ce48f214 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {
>          */
>         int pbn_div;
>  
> +       /**
> +        * @total_avail_slots: available slots for data transmission
> +        */
> +       u8 total_avail_slots;
> +       /**
> +        * @start_slot: first slot index for data transmission
> +        */
> +       u8 start_slot;
> +
>         /**
>          * @funcs: Atomic helper callbacks
>          */
Zuo, Jerry Aug. 31, 2021, 7:44 p.m. UTC | #3
[AMD Official Use Only]

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: August 30, 2021 4:00 PM
> To: Zuo, Jerry <Jerry.Zuo@amd.com>; dri-devel@lists.freedesktop.org
> Cc: Wentland, Harry <Harry.Wentland@amd.com>; Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>
> Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based
> on Encoding Format
>
> On Fri, 2021-08-27 at 19:43 -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.
> >
> > Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 27
> > ++++++++++++++++++++-------
> >  include/drm/drm_dp_mst_helper.h       |  9 +++++++++
> >  2 files changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 86d13d6bc463..30544801d2b8 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -3370,7 +3370,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); @@ -4323,7 +4323,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;
> >  }
> > @@ -4335,7 +4335,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;
> > @@ -4509,6 +4509,17 @@ 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_encoding_cap(struct
> drm_dp_mst_topology_mgr
> > +*mgr,
> > uint8_t link_encoding_cap)
> > +{
> > +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
> > +               mgr->total_avail_slots = 64;
> > +               mgr->start_slot = 0;
> > +       }
> > +       DRM_DEBUG_KMS("%s encoding format determined\n",
> > +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ?
> > "128b/132b" : "8b/10b");
> > +}
> > +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
> > +
>
> This seems to be missing kdocs, can you fix that?
>
> Also - I'm not convinced this is all of the work we have to do, as there's no
> locking taking place here in this function. If we're changing the number of
> available VCPI slots that we have, we need to be able to factor that into the
> atomic check logic which means that we can't rely on mgr->* for any kind of
> data that isn't guaranteed to remain consistent throughout the lifetime of the
> driver or topology. (Note that some of the old MST code didn't follow this
> logic, so I wouldn't be surprised if there's still exceptions to this we need to
> clean up).
>
> Note that I still expect we'll have to keep some sort of track of the current
> total slot count in the topology mgr, but that should be reflecting the
> currently programmed state and not be relied on from our atomic check.
>

Thanks Lyude for your comments.

Seems I should keep existing code to keep track of current slot status in mgr.
That information is getting updated each time when topology change detected.
That slot information saved in mgr is a sort of static, and could only be used
for debug purpose to track what is the current encoding format.

> IMHO - the correct way we should go about adding support for this is to add
> something into drm_dp_mst_topology_state and integrate this into the
> atomic check helpers.

The slot information should also be added into drm_dp_mst_topology_state to
reflect the real-time slot status.

I'd like to confirm the best place to get slot count info. updated.
Should the update be done within &drm_mode_config_funcs. atomic_check(),
before new stream is created, OR
should be updated within drm_dp_mst_atomic_check() ?

The updated slot count will be used in drm_dp_mst_atomic_check() to check slot limit,
and in drm_dp_update_payload_part1() as initial cur_slots.

>
> >  /**
> >   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> >   * @mgr: manager for this port
> > @@ -4540,8 +4551,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;
> >         }
> > @@ -5228,7 +5239,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 */ @@ -5257,7 +5268,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;
> >  }
> > @@ -5529,6 +5540,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->total_avail_slots = 63;
> > +       mgr->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 ddb9231d0309..eac5ce48f214
> > 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {
> >          */
> >         int pbn_div;
> >
> > +       /**
> > +        * @total_avail_slots: available slots for data transmission
> > +        */
> > +       u8 total_avail_slots;
> > +       /**
> > +        * @start_slot: first slot index for data transmission
> > +        */
> > +       u8 start_slot;
> > +
> >         /**
> >          * @funcs: Atomic helper callbacks
> >          */
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
Lyude Paul Aug. 31, 2021, 8:19 p.m. UTC | #4
On Tue, 2021-08-31 at 19:44 +0000, Zuo, Jerry wrote:
> [AMD Official Use Only]
> 
> > -----Original Message-----
> > From: Lyude Paul <lyude@redhat.com>
> > Sent: August 30, 2021 4:00 PM
> > To: Zuo, Jerry <Jerry.Zuo@amd.com>; dri-devel@lists.freedesktop.org
> > Cc: Wentland, Harry <Harry.Wentland@amd.com>; Kazlauskas, Nicholas
> > <Nicholas.Kazlauskas@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>
> > Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based
> > on Encoding Format
> > 
> > On Fri, 2021-08-27 at 19:43 -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.
> > > 
> > > Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 27
> > > ++++++++++++++++++++-------
> > >  include/drm/drm_dp_mst_helper.h       |  9 +++++++++
> > >  2 files changed, 29 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 86d13d6bc463..30544801d2b8 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3370,7 +3370,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); @@ -4323,7 +4323,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;
> > >  }
> > > @@ -4335,7 +4335,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;
> > > @@ -4509,6 +4509,17 @@ 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_encoding_cap(struct
> > drm_dp_mst_topology_mgr
> > > +*mgr,
> > > uint8_t link_encoding_cap)
> > > +{
> > > +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
> > > +               mgr->total_avail_slots = 64;
> > > +               mgr->start_slot = 0;
> > > +       }
> > > +       DRM_DEBUG_KMS("%s encoding format determined\n",
> > > +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ?
> > > "128b/132b" : "8b/10b");
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
> > > +
> > 
> > This seems to be missing kdocs, can you fix that?
> > 
> > Also - I'm not convinced this is all of the work we have to do, as there's
> > no
> > locking taking place here in this function. If we're changing the number
> > of
> > available VCPI slots that we have, we need to be able to factor that into
> > the
> > atomic check logic which means that we can't rely on mgr->* for any kind
> > of
> > data that isn't guaranteed to remain consistent throughout the lifetime of
> > the
> > driver or topology. (Note that some of the old MST code didn't follow this
> > logic, so I wouldn't be surprised if there's still exceptions to this we
> > need to
> > clean up).
> > 
> > Note that I still expect we'll have to keep some sort of track of the
> > current
> > total slot count in the topology mgr, but that should be reflecting the
> > currently programmed state and not be relied on from our atomic check.
> > 
> 
> Thanks Lyude for your comments.
> 
> Seems I should keep existing code to keep track of current slot status in
> mgr.
> That information is getting updated each time when topology change detected.
> That slot information saved in mgr is a sort of static, and could only be
> used
> for debug purpose to track what is the current encoding format.

Ahh - in that case maybe we should try getting rid of it (as we could just use
the atomic state for debugging output anyway).

> 
> > IMHO - the correct way we should go about adding support for this is to
> > add
> > something into drm_dp_mst_topology_state and integrate this into the
> > atomic check helpers.
> 
> The slot information should also be added into drm_dp_mst_topology_state to
> reflect the real-time slot status.
> 
> I'd like to confirm the best place to get slot count info. updated.
> Should the update be done within &drm_mode_config_funcs. atomic_check(),
> before new stream is created, OR
> should be updated within drm_dp_mst_atomic_check() ?

drm_dp_mst_atomic_check() would be better, since I imagine that we have to do
this for all drivers using MST
> 
> The updated slot count will be used in drm_dp_mst_atomic_check() to check
> slot limit,
> and in drm_dp_update_payload_part1() as initial cur_slots.
> 
> > 
> > >  /**
> > >   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> > >   * @mgr: manager for this port
> > > @@ -4540,8 +4551,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;
> > >         }
> > > @@ -5228,7 +5239,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 */ @@ -5257,7 +5268,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;
> > >  }
> > > @@ -5529,6 +5540,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->total_avail_slots = 63;
> > > +       mgr->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 ddb9231d0309..eac5ce48f214
> > > 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {
> > >          */
> > >         int pbn_div;
> > > 
> > > +       /**
> > > +        * @total_avail_slots: available slots for data transmission
> > > +        */
> > > +       u8 total_avail_slots;
> > > +       /**
> > > +        * @start_slot: first slot index for data transmission
> > > +        */
> > > +       u8 start_slot;
> > > +
> > >         /**
> > >          * @funcs: Atomic helper callbacks
> > >          */
> > 
> > --
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
>
Lyude Paul Aug. 31, 2021, 10:44 p.m. UTC | #5
(I am going to try responding to this tomorrow btw. I haven't been super busy
this week, but this has been a surprisingly difficult email to respond to
because I need to actually need to do a deep dive some of the MST helpers
tomorrow to figure out more of the specifics on why I realized we couldn't
just hot add/remove port->connector here).

On Tue, 2021-08-31 at 19:44 +0000, Zuo, Jerry wrote:
> [AMD Official Use Only]
> 
> > -----Original Message-----
> > From: Lyude Paul <lyude@redhat.com>
> > Sent: August 30, 2021 4:00 PM
> > To: Zuo, Jerry <Jerry.Zuo@amd.com>; dri-devel@lists.freedesktop.org
> > Cc: Wentland, Harry <Harry.Wentland@amd.com>; Kazlauskas, Nicholas
> > <Nicholas.Kazlauskas@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>
> > Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based
> > on Encoding Format
> > 
> > On Fri, 2021-08-27 at 19:43 -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.
> > > 
> > > Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 27
> > > ++++++++++++++++++++-------
> > >  include/drm/drm_dp_mst_helper.h       |  9 +++++++++
> > >  2 files changed, 29 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 86d13d6bc463..30544801d2b8 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3370,7 +3370,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); @@ -4323,7 +4323,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;
> > >  }
> > > @@ -4335,7 +4335,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;
> > > @@ -4509,6 +4509,17 @@ 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_encoding_cap(struct
> > drm_dp_mst_topology_mgr
> > > +*mgr,
> > > uint8_t link_encoding_cap)
> > > +{
> > > +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
> > > +               mgr->total_avail_slots = 64;
> > > +               mgr->start_slot = 0;
> > > +       }
> > > +       DRM_DEBUG_KMS("%s encoding format determined\n",
> > > +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ?
> > > "128b/132b" : "8b/10b");
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
> > > +
> > 
> > This seems to be missing kdocs, can you fix that?
> > 
> > Also - I'm not convinced this is all of the work we have to do, as there's
> > no
> > locking taking place here in this function. If we're changing the number
> > of
> > available VCPI slots that we have, we need to be able to factor that into
> > the
> > atomic check logic which means that we can't rely on mgr->* for any kind
> > of
> > data that isn't guaranteed to remain consistent throughout the lifetime of
> > the
> > driver or topology. (Note that some of the old MST code didn't follow this
> > logic, so I wouldn't be surprised if there's still exceptions to this we
> > need to
> > clean up).
> > 
> > Note that I still expect we'll have to keep some sort of track of the
> > current
> > total slot count in the topology mgr, but that should be reflecting the
> > currently programmed state and not be relied on from our atomic check.
> > 
> 
> Thanks Lyude for your comments.
> 
> Seems I should keep existing code to keep track of current slot status in
> mgr.
> That information is getting updated each time when topology change detected.
> That slot information saved in mgr is a sort of static, and could only be
> used
> for debug purpose to track what is the current encoding format.
> 
> > IMHO - the correct way we should go about adding support for this is to
> > add
> > something into drm_dp_mst_topology_state and integrate this into the
> > atomic check helpers.
> 
> The slot information should also be added into drm_dp_mst_topology_state to
> reflect the real-time slot status.
> 
> I'd like to confirm the best place to get slot count info. updated.
> Should the update be done within &drm_mode_config_funcs. atomic_check(),
> before new stream is created, OR
> should be updated within drm_dp_mst_atomic_check() ?
> 
> The updated slot count will be used in drm_dp_mst_atomic_check() to check
> slot limit,
> and in drm_dp_update_payload_part1() as initial cur_slots.
> 
> > 
> > >  /**
> > >   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> > >   * @mgr: manager for this port
> > > @@ -4540,8 +4551,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;
> > >         }
> > > @@ -5228,7 +5239,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 */ @@ -5257,7 +5268,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;
> > >  }
> > > @@ -5529,6 +5540,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->total_avail_slots = 63;
> > > +       mgr->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 ddb9231d0309..eac5ce48f214
> > > 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {
> > >          */
> > >         int pbn_div;
> > > 
> > > +       /**
> > > +        * @total_avail_slots: available slots for data transmission
> > > +        */
> > > +       u8 total_avail_slots;
> > > +       /**
> > > +        * @start_slot: first slot index for data transmission
> > > +        */
> > > +       u8 start_slot;
> > > +
> > >         /**
> > >          * @funcs: Atomic helper callbacks
> > >          */
> > 
> > --
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
>
Lyude Paul Aug. 31, 2021, 10:45 p.m. UTC | #6
(sorry - I just wrote a second response to your email I meant to send to your
coworker Jerry Zuo! You can ignore it)

On Tue, 2021-08-31 at 19:44 +0000, Zuo, Jerry wrote:
> [AMD Official Use Only]
> 
> > -----Original Message-----
> > From: Lyude Paul <lyude@redhat.com>
> > Sent: August 30, 2021 4:00 PM
> > To: Zuo, Jerry <Jerry.Zuo@amd.com>; dri-devel@lists.freedesktop.org
> > Cc: Wentland, Harry <Harry.Wentland@amd.com>; Kazlauskas, Nicholas
> > <Nicholas.Kazlauskas@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>
> > Subject: Re: [PATCH 1/2] drm: Update MST First Link Slot Information Based
> > on Encoding Format
> > 
> > On Fri, 2021-08-27 at 19:43 -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.
> > > 
> > > Signed-off-by: Fangzhi Zuo <Jerry.Zuo@amd.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 27
> > > ++++++++++++++++++++-------
> > >  include/drm/drm_dp_mst_helper.h       |  9 +++++++++
> > >  2 files changed, 29 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 86d13d6bc463..30544801d2b8 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3370,7 +3370,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); @@ -4323,7 +4323,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;
> > >  }
> > > @@ -4335,7 +4335,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;
> > > @@ -4509,6 +4509,17 @@ 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_encoding_cap(struct
> > drm_dp_mst_topology_mgr
> > > +*mgr,
> > > uint8_t link_encoding_cap)
> > > +{
> > > +       if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
> > > +               mgr->total_avail_slots = 64;
> > > +               mgr->start_slot = 0;
> > > +       }
> > > +       DRM_DEBUG_KMS("%s encoding format determined\n",
> > > +                     (link_encoding_cap == DP_CAP_ANSI_128B132B) ?
> > > "128b/132b" : "8b/10b");
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
> > > +
> > 
> > This seems to be missing kdocs, can you fix that?
> > 
> > Also - I'm not convinced this is all of the work we have to do, as there's
> > no
> > locking taking place here in this function. If we're changing the number
> > of
> > available VCPI slots that we have, we need to be able to factor that into
> > the
> > atomic check logic which means that we can't rely on mgr->* for any kind
> > of
> > data that isn't guaranteed to remain consistent throughout the lifetime of
> > the
> > driver or topology. (Note that some of the old MST code didn't follow this
> > logic, so I wouldn't be surprised if there's still exceptions to this we
> > need to
> > clean up).
> > 
> > Note that I still expect we'll have to keep some sort of track of the
> > current
> > total slot count in the topology mgr, but that should be reflecting the
> > currently programmed state and not be relied on from our atomic check.
> > 
> 
> Thanks Lyude for your comments.
> 
> Seems I should keep existing code to keep track of current slot status in
> mgr.
> That information is getting updated each time when topology change detected.
> That slot information saved in mgr is a sort of static, and could only be
> used
> for debug purpose to track what is the current encoding format.
> 
> > IMHO - the correct way we should go about adding support for this is to
> > add
> > something into drm_dp_mst_topology_state and integrate this into the
> > atomic check helpers.
> 
> The slot information should also be added into drm_dp_mst_topology_state to
> reflect the real-time slot status.
> 
> I'd like to confirm the best place to get slot count info. updated.
> Should the update be done within &drm_mode_config_funcs. atomic_check(),
> before new stream is created, OR
> should be updated within drm_dp_mst_atomic_check() ?
> 
> The updated slot count will be used in drm_dp_mst_atomic_check() to check
> slot limit,
> and in drm_dp_update_payload_part1() as initial cur_slots.
> 
> > 
> > >  /**
> > >   * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> > >   * @mgr: manager for this port
> > > @@ -4540,8 +4551,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;
> > >         }
> > > @@ -5228,7 +5239,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 */ @@ -5257,7 +5268,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;
> > >  }
> > > @@ -5529,6 +5540,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->total_avail_slots = 63;
> > > +       mgr->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 ddb9231d0309..eac5ce48f214
> > > 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -661,6 +661,15 @@ struct drm_dp_mst_topology_mgr {
> > >          */
> > >         int pbn_div;
> > > 
> > > +       /**
> > > +        * @total_avail_slots: available slots for data transmission
> > > +        */
> > > +       u8 total_avail_slots;
> > > +       /**
> > > +        * @start_slot: first slot index for data transmission
> > > +        */
> > > +       u8 start_slot;
> > > +
> > >         /**
> > >          * @funcs: Atomic helper callbacks
> > >          */
> > 
> > --
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
>
Lyude Paul Aug. 31, 2021, 10:46 p.m. UTC | #7
...I meant Wayne Lin, whoops. been going through a lot of emails today 
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 86d13d6bc463..30544801d2b8 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3370,7 +3370,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);
@@ -4323,7 +4323,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;
 }
@@ -4335,7 +4335,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;
@@ -4509,6 +4509,17 @@  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_encoding_cap(struct drm_dp_mst_topology_mgr *mgr, uint8_t link_encoding_cap)
+{
+	if (link_encoding_cap == DP_CAP_ANSI_128B132B) {
+		mgr->total_avail_slots = 64;
+		mgr->start_slot = 0;
+	}
+	DRM_DEBUG_KMS("%s encoding format determined\n",
+		      (link_encoding_cap == DP_CAP_ANSI_128B132B) ? "128b/132b" : "8b/10b");
+}
+EXPORT_SYMBOL(drm_dp_mst_update_encoding_cap);
+
 /**
  * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
  * @mgr: manager for this port
@@ -4540,8 +4551,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;
 	}
@@ -5228,7 +5239,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 */
@@ -5257,7 +5268,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;
 }
@@ -5529,6 +5540,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->total_avail_slots = 63;
+	mgr->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 ddb9231d0309..eac5ce48f214 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -661,6 +661,15 @@  struct drm_dp_mst_topology_mgr {
 	 */
 	int pbn_div;
 
+	/**
+	 * @total_avail_slots: available slots for data transmission
+	 */
+	u8 total_avail_slots;
+	/**
+	 * @start_slot: first slot index for data transmission
+	 */
+	u8 start_slot;
+
 	/**
 	 * @funcs: Atomic helper callbacks
 	 */