Re: [Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

2016-11-18 Thread Pandiyan, Dhinakaran 
On Fri, 2016-11-18 at 06:57 +, Chris Wilson wrote:
> On Thu, Nov 17, 2016 at 06:03:48PM -0800, Dhinakaran Pandiyan wrote:
> >  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > struct drm_dp_vcpi *vcpi, int pbn)
> >  {
> > -   int num_slots;
> > +   int req_slots;
> > int ret;
> >  
> > -   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > +   req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >  
> > -   if (num_slots > mgr->avail_slots)
> > -   return -ENOSPC;
> > +   mutex_lock(&mgr->lock);
> > +   if (req_slots > mgr->avail_slots) {
> > +   ret = -ENOSPC;
> > +   goto out;
> > +   }
> 
> You are not atomically reducing the mgr->avail_slots, leading to
> possible overallocation of multiple streams?
> -Chris
> 

Yeah, I got it wrong. I should have reduced mgr->avail_slots here before
releasing the mutex. Thanks for pointing it out.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

2016-11-18 Thread Daniel Vetter 
On Fri, Nov 18, 2016 at 06:57:01AM +, Chris Wilson wrote:
> On Thu, Nov 17, 2016 at 06:03:48PM -0800, Dhinakaran Pandiyan wrote:
> >  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > struct drm_dp_vcpi *vcpi, int pbn)
> >  {
> > -   int num_slots;
> > +   int req_slots;
> > int ret;
> >  
> > -   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > +   req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >  
> > -   if (num_slots > mgr->avail_slots)
> > -   return -ENOSPC;
> > +   mutex_lock(&mgr->lock);
> > +   if (req_slots > mgr->avail_slots) {
> > +   ret = -ENOSPC;
> > +   goto out;
> > +   }
> 
> You are not atomically reducing the mgr->avail_slots, leading to
> possible overallocation of multiple streams?

Yup, see my lenghty reply to patch 1 for what needs to be done here to fix
this correctly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

2016-11-17 Thread Chris Wilson 
On Thu, Nov 17, 2016 at 06:03:48PM -0800, Dhinakaran Pandiyan wrote:
>  static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>   struct drm_dp_vcpi *vcpi, int pbn)
>  {
> - int num_slots;
> + int req_slots;
>   int ret;
>  
> - num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> + req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>  
> - if (num_slots > mgr->avail_slots)
> - return -ENOSPC;
> + mutex_lock(&mgr->lock);
> + if (req_slots > mgr->avail_slots) {
> + ret = -ENOSPC;
> + goto out;
> + }

You are not atomically reducing the mgr->avail_slots, leading to
possible overallocation of multiple streams?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

2016-11-17 Thread Manasi Navare 
On Thu, Nov 17, 2016 at 06:03:48PM -0800, Dhinakaran Pandiyan wrote:
> The avail_slots member in struct drm_dp_mst_topology_mgr does not really
> track the available time slots in a MTP(Multi-Stream Transport Packet). It
> is assigned an initial value when the topology manager is setup but not
> updated after that.
> 
> So, let's use avail_slots to store the number of unallocated slots out of
> the total 64. The value will be updated when vcpi allocation or reset
> happens. Since vcpi allocation and deallocation can happen simultaneously,
> the struct drm_dp_mst_topology_mgr.lock mutex is used to protect
> it from concurrent accesses.
> 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 55 
> ++-
>  drivers/gpu/drm/i915/intel_dp_mst.c   |  5 +++-
>  include/drm/drm_dp_mst_helper.h   |  2 +-
>  3 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 26dfd99..19e2250 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2040,7 +2040,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct 
> drm_dp_mst_topology_mgr *mgr, bool ms
>   }
>   mgr->total_pbn = 64 * mgr->pbn_div;
>   mgr->total_slots = 64;
> - mgr->avail_slots = mgr->total_slots;
> +
> + /* 1 slot out of the 64 time slots is used for MTP header */
> + mgr->avail_slots = mgr->total_slots - 1;
>  
>   /* add initial branch device at LCT 1 */
>   mstb = drm_dp_add_mst_branch_device(1, NULL);
> @@ -2465,34 +2467,52 @@ EXPORT_SYMBOL(drm_dp_mst_get_edid);
>   * @pbn: payload bandwidth to convert into slots.
>   */
>  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> -int pbn)
> +struct drm_dp_mst_port *port, int pbn)
>  {
> - int num_slots;
> + int req_slots, curr_slots, new_slots, ret;
> +
> + req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> + curr_slots = drm_dp_mst_get_vcpi_slots(mgr, port);
>  
> - num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> + if (req_slots <= curr_slots)
> + return req_slots;

Are you sure you want to return from the function at this
or should this just be ret = req_slots as you are returning ret at the end of 
the function.

Manasi
>  
> - if (num_slots > mgr->avail_slots)
> - return -ENOSPC;
> - return num_slots;
> + new_slots = req_slots - curr_slots;
> + mutex_lock(&mgr->lock);
> + if (new_slots <= mgr->avail_slots) {
> + ret = req_slots;
> + } else {
> + DRM_DEBUG_KMS("not enough vcpi slots, req=%d avail=%d\n", 
> req_slots, mgr->avail_slots);
> + ret =  -ENOSPC;
> + }
> + mutex_unlock(&mgr->lock);
> +
> + return ret;
>  }
>  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)
>  {
> - int num_slots;
> + int req_slots;
>   int ret;
>  
> - num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> + req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
>  
> - if (num_slots > mgr->avail_slots)
> - return -ENOSPC;
> + mutex_lock(&mgr->lock);
> + if (req_slots > mgr->avail_slots) {
> + ret = -ENOSPC;
> + goto out;
> + }
>  
>   vcpi->pbn = pbn;
> - vcpi->aligned_pbn = num_slots * mgr->pbn_div;
> - vcpi->num_slots = num_slots;
> + vcpi->aligned_pbn = req_slots * mgr->pbn_div;
> + vcpi->num_slots = req_slots;
>  
>   ret = drm_dp_mst_assign_payload_id(mgr, vcpi);
> +
> +out:
> + mutex_unlock(&mgr->lock);
>   if (ret < 0)
>   return ret;
>   return 0;
> @@ -2530,6 +2550,10 @@ bool drm_dp_mst_allocate_vcpi(struct 
> drm_dp_mst_topology_mgr *mgr, struct drm_dp
>   DRM_DEBUG_KMS("initing vcpi for %d %d\n", pbn, port->vcpi.num_slots);
>   *slots = port->vcpi.num_slots;
>  
> + mutex_lock(&mgr->lock);
> + mgr->avail_slots -= port->vcpi.num_slots;
> + mutex_unlock(&mgr->lock);
> +
>   drm_dp_put_port(port);
>   return true;
>  out:
> @@ -2562,6 +2586,11 @@ void drm_dp_mst_reset_vcpi_slots(struct 
> drm_dp_mst_topology_mgr *mgr, struct drm
>   port = drm_dp_get_validated_port_ref(mgr, port);
>   if (!port)
>   return;
> +
> + mutex_lock(&mgr->lock);
> + mgr->avail_slots += port->vcpi.num_slots;
> + mutex_unlock(&mgr->lock);
> +
>   port->vcpi.num_slots = 0;
>   drm_dp_put_port(port);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 4280a83..bad9300 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -42,6 +42,8 @@ static bool intel_dp_mst_compute_config(struct 
> i

Re: [Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

2016-11-17 Thread kbuild test robot 
Hi Dhinakaran,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.9-rc5 next-20161117]
[cannot apply to drm-intel/for-linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Dhinakaran-Pandiyan/Track-available-link-bandwidth-for-DP-MST/20161118-101200
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
reproduce: make htmldocs; make DOCBOOKS='' pdfdocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make 
rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter 
'...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' 
description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter 
'...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' 
description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   include/drm/drm_drv.h:295: warning: Incorrect use of kernel-doc format:  
 * Hook for allocating the GEM object struct, for use by core
   include/drm/drm_drv.h:407: warning: No description found for parameter 'load'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'firstopen'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'open'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'preclose'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'postclose'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'lastclose'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'unload'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'dma_ioctl'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'dma_quiescent'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'context_dtor'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'set_busid'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'irq_handler'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'irq_preinstall'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'irq_postinstall'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'irq_uninstall'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'debugfs_init'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'debugfs_cleanup'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_open_object'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_close_object'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_create_object'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'prime_handle_to_fd'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'prime_fd_to_handle'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_export'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_import'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_pin'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_unpin'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_res_obj'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_get_sg_table'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_import_sg_table'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_vmap'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_vunmap'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_prime_mmap'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'vgaarb_irq'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'gem_vm_ops'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'major'
   include/drm/drm_drv.h:407: warning: No description found for parameter 
'minor'
   include/drm/drm_drv.h:407: warning: No description foun

[Intel-gfx] [PATCH 3/3] drm/dp/mst: Track available time slots in DP Multi-Stream Transport Packet

2016-11-17 Thread Dhinakaran Pandiyan 
The avail_slots member in struct drm_dp_mst_topology_mgr does not really
track the available time slots in a MTP(Multi-Stream Transport Packet). It
is assigned an initial value when the topology manager is setup but not
updated after that.

So, let's use avail_slots to store the number of unallocated slots out of
the total 64. The value will be updated when vcpi allocation or reset
happens. Since vcpi allocation and deallocation can happen simultaneously,
the struct drm_dp_mst_topology_mgr.lock mutex is used to protect
it from concurrent accesses.

Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 55 ++-
 drivers/gpu/drm/i915/intel_dp_mst.c   |  5 +++-
 include/drm/drm_dp_mst_helper.h   |  2 +-
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 26dfd99..19e2250 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2040,7 +2040,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct 
drm_dp_mst_topology_mgr *mgr, bool ms
}
mgr->total_pbn = 64 * mgr->pbn_div;
mgr->total_slots = 64;
-   mgr->avail_slots = mgr->total_slots;
+
+   /* 1 slot out of the 64 time slots is used for MTP header */
+   mgr->avail_slots = mgr->total_slots - 1;
 
/* add initial branch device at LCT 1 */
mstb = drm_dp_add_mst_branch_device(1, NULL);
@@ -2465,34 +2467,52 @@ EXPORT_SYMBOL(drm_dp_mst_get_edid);
  * @pbn: payload bandwidth to convert into slots.
  */
 int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
-  int pbn)
+  struct drm_dp_mst_port *port, int pbn)
 {
-   int num_slots;
+   int req_slots, curr_slots, new_slots, ret;
+
+   req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+   curr_slots = drm_dp_mst_get_vcpi_slots(mgr, port);
 
-   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+   if (req_slots <= curr_slots)
+   return req_slots;
 
-   if (num_slots > mgr->avail_slots)
-   return -ENOSPC;
-   return num_slots;
+   new_slots = req_slots - curr_slots;
+   mutex_lock(&mgr->lock);
+   if (new_slots <= mgr->avail_slots) {
+   ret = req_slots;
+   } else {
+   DRM_DEBUG_KMS("not enough vcpi slots, req=%d avail=%d\n", 
req_slots, mgr->avail_slots);
+   ret =  -ENOSPC;
+   }
+   mutex_unlock(&mgr->lock);
+
+   return ret;
 }
 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)
 {
-   int num_slots;
+   int req_slots;
int ret;
 
-   num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+   req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
 
-   if (num_slots > mgr->avail_slots)
-   return -ENOSPC;
+   mutex_lock(&mgr->lock);
+   if (req_slots > mgr->avail_slots) {
+   ret = -ENOSPC;
+   goto out;
+   }
 
vcpi->pbn = pbn;
-   vcpi->aligned_pbn = num_slots * mgr->pbn_div;
-   vcpi->num_slots = num_slots;
+   vcpi->aligned_pbn = req_slots * mgr->pbn_div;
+   vcpi->num_slots = req_slots;
 
ret = drm_dp_mst_assign_payload_id(mgr, vcpi);
+
+out:
+   mutex_unlock(&mgr->lock);
if (ret < 0)
return ret;
return 0;
@@ -2530,6 +2550,10 @@ bool drm_dp_mst_allocate_vcpi(struct 
drm_dp_mst_topology_mgr *mgr, struct drm_dp
DRM_DEBUG_KMS("initing vcpi for %d %d\n", pbn, port->vcpi.num_slots);
*slots = port->vcpi.num_slots;
 
+   mutex_lock(&mgr->lock);
+   mgr->avail_slots -= port->vcpi.num_slots;
+   mutex_unlock(&mgr->lock);
+
drm_dp_put_port(port);
return true;
 out:
@@ -2562,6 +2586,11 @@ void drm_dp_mst_reset_vcpi_slots(struct 
drm_dp_mst_topology_mgr *mgr, struct drm
port = drm_dp_get_validated_port_ref(mgr, port);
if (!port)
return;
+
+   mutex_lock(&mgr->lock);
+   mgr->avail_slots += port->vcpi.num_slots;
+   mutex_unlock(&mgr->lock);
+
port->vcpi.num_slots = 0;
drm_dp_put_port(port);
 }
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 4280a83..bad9300 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -42,6 +42,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder 
*encoder,
int lane_count, slots;
const struct drm_display_mode *adjusted_mode = 
&pipe_config->base.adjusted_mode;
int mst_pbn;
+   struct intel_connector *connector =
+   to_intel_connector(conn_state->connector);
 
pipe_config->dp_encoder_is_mst = true;
pipe_config->has_pch_encoder = false;