Re: [Intel-gfx] [PATCH v2 3/9] drm/dp: Split drm_dp_mst_allocate_vcpi

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 10:31 +1000, Dave Airlie wrote:
> On 25 January 2017 at 09:49, Dhinakaran Pandiyan
>  wrote:
> > drm_dp_mst_allocate_vcpi() apart from setting up the vcpi structure,
> > also finds if there are enough slots available. This check is a duplicate
> > of that implemented in drm_dp_mst_find_vcpi_slots(). Let's move this check
> > out and reuse the existing drm_dp_mst_find_vcpi_slots() function to check
> > if there are enough vcpi slots before allocating them.
> >
> > This brings the check to one place. Additionally drivers that will use MST
> > state tracking for atomic modesets can use the atomic version of
> > find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()
> >
> 
> Also seem sane, at least for the core bits,
> 
> Reviewed-by: Dave Airlie 
> 

Thanks for the review.

-DK


> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c  | 20 +---
> >  drivers/gpu/drm/i915/intel_dp_mst.c|  4 ++--
> >  drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
> >  drivers/gpu/drm/radeon/radeon_dp_mst.c |  4 +++-
> >  include/drm/drm_dp_mst_helper.h|  2 +-
> >  5 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d9edd84..b871d4e 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2479,20 +2479,17 @@ int drm_dp_find_vcpi_slots(struct 
> > drm_dp_mst_topology_mgr *mgr,
> >  EXPORT_SYMBOL(drm_dp_find_vcpi_slots);
> >
> >  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > -   struct drm_dp_vcpi *vcpi, int pbn)
> > +   struct drm_dp_vcpi *vcpi, int pbn, int slots)
> >  {
> > -   int num_slots;
> > int ret;
> >
> > -   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > -
> > /* max. time slots - one slot for MTP header */
> > -   if (num_slots > 63)
> > +   if (slots > 63)
> > return -ENOSPC;
> >
> > vcpi->pbn = pbn;
> > -   vcpi->aligned_pbn = num_slots * mgr->pbn_div;
> > -   vcpi->num_slots = num_slots;
> > +   vcpi->aligned_pbn = slots * mgr->pbn_div;
> > +   vcpi->num_slots = slots;
> >
> > ret = drm_dp_mst_assign_payload_id(mgr, vcpi);
> > if (ret < 0)
> > @@ -2507,7 +2504,7 @@ static int drm_dp_init_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr,
> >   * @pbn: payload bandwidth number to request
> >   * @slots: returned number of slots for this PBN.
> >   */
> > -bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> > drm_dp_mst_port *port, int pbn, int *slots)
> > +bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> > drm_dp_mst_port *port, int pbn, int slots)
> >  {
> > int ret;
> >
> > @@ -2515,16 +2512,18 @@ bool drm_dp_mst_allocate_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > if (!port)
> > return false;
> >
> > +   if (slots < 0)
> > +   return false;
> > +
> > if (port->vcpi.vcpi > 0) {
> > DRM_DEBUG_KMS("payload: vcpi %d already allocated for pbn 
> > %d - requested pbn %d\n", port->vcpi.vcpi, port->vcpi.pbn, pbn);
> > if (pbn == port->vcpi.pbn) {
> > -   *slots = port->vcpi.num_slots;
> > drm_dp_put_port(port);
> > return true;
> > }
> > }
> >
> > -   ret = drm_dp_init_vcpi(mgr, >vcpi, pbn);
> > +   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);
> > @@ -2532,7 +2531,6 @@ bool drm_dp_mst_allocate_vcpi(struct 
> > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > }
> > DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
> > pbn, port->vcpi.num_slots);
> > -   *slots = port->vcpi.num_slots;
> >
> > drm_dp_put_port(port);
> > return true;
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 38e3ca2..f51574f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -147,7 +147,6 @@ static void intel_mst_pre_enable_dp(struct 
> > intel_encoder *encoder,
> > to_intel_connector(conn_state->connector);
> > int ret;
> > uint32_t temp;
> > -   int slots;
> >
> > /* MST encoders are bound to a crtc, not to a connector,
> >  * force the mapping here for get_hw_state.
> > @@ -177,7 +176,8 @@ static void intel_mst_pre_enable_dp(struct 
> > intel_encoder *encoder,
> >
> > ret = 

Re: [Intel-gfx] [PATCH v2 3/9] drm/dp: Split drm_dp_mst_allocate_vcpi

[Dave Airlie]

On 25 January 2017 at 09:49, Dhinakaran Pandiyan
 wrote:
> drm_dp_mst_allocate_vcpi() apart from setting up the vcpi structure,
> also finds if there are enough slots available. This check is a duplicate
> of that implemented in drm_dp_mst_find_vcpi_slots(). Let's move this check
> out and reuse the existing drm_dp_mst_find_vcpi_slots() function to check
> if there are enough vcpi slots before allocating them.
>
> This brings the check to one place. Additionally drivers that will use MST
> state tracking for atomic modesets can use the atomic version of
> find_vcpi_slots() and reuse drm_dp_mst_allocate_vcpi()
>

Also seem sane, at least for the core bits,

Reviewed-by: Dave Airlie 

> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c  | 20 +---
>  drivers/gpu/drm/i915/intel_dp_mst.c|  4 ++--
>  drivers/gpu/drm/nouveau/nv50_display.c |  3 ++-
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  4 +++-
>  include/drm/drm_dp_mst_helper.h|  2 +-
>  5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d9edd84..b871d4e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2479,20 +2479,17 @@ int drm_dp_find_vcpi_slots(struct 
> drm_dp_mst_topology_mgr *mgr,
>  EXPORT_SYMBOL(drm_dp_find_vcpi_slots);
>
>  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> -   struct drm_dp_vcpi *vcpi, int pbn)
> +   struct drm_dp_vcpi *vcpi, int pbn, int slots)
>  {
> -   int num_slots;
> int ret;
>
> -   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> -
> /* max. time slots - one slot for MTP header */
> -   if (num_slots > 63)
> +   if (slots > 63)
> return -ENOSPC;
>
> vcpi->pbn = pbn;
> -   vcpi->aligned_pbn = num_slots * mgr->pbn_div;
> -   vcpi->num_slots = num_slots;
> +   vcpi->aligned_pbn = slots * mgr->pbn_div;
> +   vcpi->num_slots = slots;
>
> ret = drm_dp_mst_assign_payload_id(mgr, vcpi);
> if (ret < 0)
> @@ -2507,7 +2504,7 @@ static int drm_dp_init_vcpi(struct 
> drm_dp_mst_topology_mgr *mgr,
>   * @pbn: payload bandwidth number to request
>   * @slots: returned number of slots for this PBN.
>   */
> -bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> drm_dp_mst_port *port, int pbn, int *slots)
> +bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct 
> drm_dp_mst_port *port, int pbn, int slots)
>  {
> int ret;
>
> @@ -2515,16 +2512,18 @@ bool drm_dp_mst_allocate_vcpi(struct 
> drm_dp_mst_topology_mgr *mgr, struct drm_dp
> if (!port)
> return false;
>
> +   if (slots < 0)
> +   return false;
> +
> if (port->vcpi.vcpi > 0) {
> DRM_DEBUG_KMS("payload: vcpi %d already allocated for pbn %d 
> - requested pbn %d\n", port->vcpi.vcpi, port->vcpi.pbn, pbn);
> if (pbn == port->vcpi.pbn) {
> -   *slots = port->vcpi.num_slots;
> drm_dp_put_port(port);
> return true;
> }
> }
>
> -   ret = drm_dp_init_vcpi(mgr, >vcpi, pbn);
> +   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);
> @@ -2532,7 +2531,6 @@ bool drm_dp_mst_allocate_vcpi(struct 
> drm_dp_mst_topology_mgr *mgr, struct drm_dp
> }
> DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
> pbn, port->vcpi.num_slots);
> -   *slots = port->vcpi.num_slots;
>
> drm_dp_put_port(port);
> return true;
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 38e3ca2..f51574f 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -147,7 +147,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder 
> *encoder,
> to_intel_connector(conn_state->connector);
> int ret;
> uint32_t temp;
> -   int slots;
>
> /* MST encoders are bound to a crtc, not to a connector,
>  * force the mapping here for get_hw_state.
> @@ -177,7 +176,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder 
> *encoder,
>
> ret = drm_dp_mst_allocate_vcpi(_dp->mst_mgr,
>connector->port,
> -  pipe_config->pbn, );
> +  pipe_config->pbn,
> +  pipe_config->dp_m_n.tu);
> if (ret == false) {
>