[PATCH 0/3] Track available link bandwidth for DP MST

[PATCH 0/3] Track available link bandwidth for DP MST

[Dhinakaran Pandiyan]

The number of available time slots in a MTP should be updated when a new
vcpi is added or an existing one is removed. Keeping this updated will be
useful to reject modes for which there is not enough link bandwidth early.

Dhinakaran Pandiyan (3):
  drm/i915/dp: Fail DP MST config when there are not enough vcpi slots
  drm/dp/mst: Calculate total link bandwidth instead of hardcoding it
  drm/dp/mst: Track available time slots in DP Multi-Stream Transport
    Packet

 drivers/gpu/drm/drm_dp_mst_topology.c | 58 ++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_dp_mst.c   |  9 +++++-
 include/drm/drm_dp_mst_helper.h       |  2 +-
 3 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Dhinakaran Pandiyan]

drm_dp_find_vcpi_slots() returns an error when there is not enough
available bandwidth on a link to support a mode. This error should make
compute_config() to fail. Not returning false could end up in a modeset
which will not work.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index e21cf08..4280a83 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->pbn = mst_pbn;
 	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
+	if (slots < 0) {
+		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
+		return false;
+	}
 
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/dp/mst: Calculate total link bandwidth instead of hardcoding it

[Dhinakaran Pandiyan]

The total or the nominal link bandwidth, which we save in terms of PBN, is
a factor of link rate and lane count. But, currently we hardcode it to
2560 PBN. This results in incorrect computation of total slots.

E.g, 2 lane HBR2 configuration and 4k@60Hz, 24bpp mode
  nominal link bw = 1080 MBps = 1280PBN = 64 slots
  required bw 533.25 MHz*3 = 1599.75 MBps or 1896 PBN
     with +0.6% margin = 1907.376 PBN = 96 slots
  This is greater than the max. possible value of 64 slots. But, we
  incorrectly compute available slots as 2560 PBN = 128 slots and don't
  return error.

So, let's fix this by calculating the total link bandwidth as
link bw (PBN) = BW per time slot(PBN) * max. time slots , where max. time
slots is 64

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 04e4571..26dfd99 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2038,9 +2038,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 			ret = -EINVAL;
 			goto out_unlock;
 		}
-
-		mgr->total_pbn = 2560;
-		mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
+		mgr->total_pbn = 64 * mgr->pbn_div;
+		mgr->total_slots = 64;
 		mgr->avail_slots = mgr->total_slots;
 
 		/* add initial branch device at LCT 1 */
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <dhinakaran.pandiyan@intel.com>
---
 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;
@@ -62,7 +64,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
 
 	pipe_config->pbn = mst_pbn;
-	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
+	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, connector->port,
+				       mst_pbn);
 	if (slots < 0) {
 		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
 		return false;
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 0032076..5c55528 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -590,7 +590,7 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 
 
 int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
-			   int pbn);
+			   struct drm_dp_mst_port *port, int pbn);
 
 
 int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 10856 bytes --]

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 found for parameter 'patchlevel'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'driver_features'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'dev_priv_size'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'ioctls'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'num_ioctls'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'fops'
   include/drm/drm_drv.h:407: warning: No description found for parameter 'legacy_dev_list'
>> drivers/gpu/drm/drm_dp_mst_topology.c:2474: warning: No description found for parameter 'port'
   drivers/gpu/drm/drm_dp_mst_topology.c:2475: warning: No description found for parameter 'port'
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_fence_reg.c
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_fence_reg.c
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -internal drivers/gpu/drm/i915/i915_gem_fence_reg.c' failed with return code 2
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_fence_reg.c
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -function fence register handling drivers/gpu/drm/i915/i915_gem_fence_reg.c' failed with return code 1
   Error: Cannot open file drivers/gpu/drm/i915/i915_gem_fence_reg.c
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -function tiling swizzling details drivers/gpu/drm/i915/i915_gem_fence_reg.c' failed with return code 1
   include/media/media-entity.h:1054: warning: No description found for parameter '...'
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:3212: ERROR: Unexpected indentation.
   include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1772: ERROR: Unexpected indentation.
   include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting

vim +/port +2474 drivers/gpu/drm/drm_dp_mst_topology.c

8ae22cb4 Dave Airlie         2016-02-17  2458  		edid = drm_get_edid(connector, &port->aux.ddc);
8ae22cb4 Dave Airlie         2016-02-17  2459  		drm_mode_connector_set_tile_property(connector);
8ae22cb4 Dave Airlie         2016-02-17  2460  	}
ef8f9bea Libin Yang          2015-12-02  2461  	port->has_audio = drm_detect_monitor_audio(edid);
ad7f8a1f Dave Airlie         2014-06-05  2462  	drm_dp_put_port(port);
ad7f8a1f Dave Airlie         2014-06-05  2463  	return edid;
ad7f8a1f Dave Airlie         2014-06-05  2464  }
ad7f8a1f Dave Airlie         2014-06-05  2465  EXPORT_SYMBOL(drm_dp_mst_get_edid);
ad7f8a1f Dave Airlie         2014-06-05  2466  
ad7f8a1f Dave Airlie         2014-06-05  2467  /**
ad7f8a1f Dave Airlie         2014-06-05  2468   * drm_dp_find_vcpi_slots() - find slots for this PBN value
ad7f8a1f Dave Airlie         2014-06-05  2469   * @mgr: manager to use
ad7f8a1f Dave Airlie         2014-06-05  2470   * @pbn: payload bandwidth to convert into slots.
ad7f8a1f Dave Airlie         2014-06-05  2471   */
ad7f8a1f Dave Airlie         2014-06-05  2472  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2473  			   struct drm_dp_mst_port *port, int pbn)
ad7f8a1f Dave Airlie         2014-06-05 @2474  {
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2475  	int req_slots, curr_slots, new_slots, ret;
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2476  
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2477  	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2478  	curr_slots = drm_dp_mst_get_vcpi_slots(mgr, port);
ad7f8a1f Dave Airlie         2014-06-05  2479  
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2480  	if (req_slots <= curr_slots)
af2a61e3 Dhinakaran Pandiyan 2016-11-17  2481  		return req_slots;
ad7f8a1f Dave Airlie         2014-06-05  2482  

:::::: The code at line 2474 was first introduced by commit
:::::: ad7f8a1f9ced7f049f9b66d588723f243a7034cd drm/helper: add Displayport multi-stream helper (v0.6)

:::::: TO: Dave Airlie <airlied@redhat.com>
:::::: CC: Dave Airlie <airlied@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6425 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <dhinakaran.pandiyan@intel.com>
> ---
>  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 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;
> @@ -62,7 +64,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>  
>  	pipe_config->pbn = mst_pbn;
> -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> +	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, connector->port,
> +				       mst_pbn);
>  	if (slots < 0) {
>  		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
>  		return false;
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 0032076..5c55528 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -590,7 +590,7 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  
>  
>  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> -			   int pbn);
> +			   struct drm_dp_mst_port *port, int pbn);
>  
>  
>  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
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

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for Track available link bandwidth for DP MST

[Patchwork]

== Series Details ==

Series: Track available link bandwidth for DP MST
URL   : https://patchwork.freedesktop.org/series/15528/
State : success

== Summary ==

Series 15528v1 Track available link bandwidth for DP MST
https://patchwork.freedesktop.org/api/1.0/series/15528/revisions/1/mbox/


fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

65a7649362717adfae612dc8fbc7555e3f3ea64f drm-intel-nightly: 2016y-11m-18d-07h-27m-44s UTC integration manifest
fb3fb22 drm/dp/mst: Calculate total link bandwidth instead of hardcoding it
b90b9e3 drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3047/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Daniel Vetter]

On Thu, Nov 17, 2016 at 06:03:46PM -0800, Dhinakaran Pandiyan wrote:
> drm_dp_find_vcpi_slots() returns an error when there is not enough
> available bandwidth on a link to support a mode. This error should make
> compute_config() to fail. Not returning false could end up in a modeset
> which will not work.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index e21cf08..4280a83 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->pbn = mst_pbn;
>  	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> +	if (slots < 0) {
> +		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);

No DRM_ERROR for cases that are expected to fail, i.e. DRM_DEBUG_KMS is
the right level.

And I don't think this works correctly either. Assume you do an atomic
modeset where you enable 2 mst sinks at the same time, and the following
happens:
- mst connector 1 can be allocated, and passes
  intel_dp_mst_compute_config.
- mst connector 2 can be allocated, but not together with connector 1.
  But drm_dp_find_vcpi_slots only checks what's available, it doesn't do a
  temporary reservation.

And we can just reserve the slot in drm_dp_find_vcpi_slots, because then
in the above case (when connector 2 doesn't have enough slots anymore)
we'd leak the vcpi allocation for connector 1.

Even worse, when we do a TEST_ONLY atomic commit (i.e. only run
atomic_check/compute_config code, but not commit anything) this can even
happen for a successful commit.

Long story short: To fix this properly we need to rewrite the dp helpers
to be compliant with atomic design. I think for that we roughly need:

- Exract vcpi allocations into a free-standing state structure. I'd call
  it drm_dp_mst_state or similar. Provide duplicate(get_state)/release
  functions like we already have for plane, connector and crtc states in
  the core, and e.g. dpll configuration in i915/intel_dpll_mgr.c. Ander
  Conselvan can help you with this. I'm also planning to write better
  documentation for how to do this exactly (since you also need a ww_mutex
  to protect this state), and I'll prioritize that work.

- Wire up that dp mst state at the right places globally (we need one slot
  per drm_dp_mst_topology_mgr, i.e. per port), and duplicate that state in
  intel_dp_mst_compute_config. We can't wire this up anywhere in the core
  since the dp mst library is a helper library, so all the integration
  points need to be done explicitly in drivers.

- Do the same for nouveau - nouveau is now also atomic, and it also is
  atomic with mst support.

- While doing all that make sure that the existing (non-atomic-compliant)
  functions in the dp mst helpers keep working, we need those for amggpu.

- Create a new drm_dp_state_allocate_vcpi_slots, which only touches the
  new drm_dp_mst_state structure and allocats the vcpi slots there. Also
  add some function to find the allocation for each sink again (we need
  that in the modeset commit functions).

- Rework our compute_config and modeset code to use this new function, and
  not the old legacy find/allocate functions.

To make this happen we need buy-in from Ben Skeggs (nouveau maintainer)
and preferrably also from the AMD display team (since they support mst
already, and also plan to eventually support atomic).

Fixing this correctly is unfortunately a _lot_ more work than your simple
patch here :(
-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

[Daniel Vetter]

On Fri, Nov 18, 2016 at 06:57:01AM +0000, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Daniel Vetter]

On Fri, Nov 18, 2016 at 9:43 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Nov 17, 2016 at 06:03:46PM -0800, Dhinakaran Pandiyan wrote:
>> drm_dp_find_vcpi_slots() returns an error when there is not enough
>> available bandwidth on a link to support a mode. This error should make
>> compute_config() to fail. Not returning false could end up in a modeset
>> which will not work.
>>
>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index e21cf08..4280a83 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>>
>>       pipe_config->pbn = mst_pbn;
>>       slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
>> +     if (slots < 0) {
>> +             DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
>
> No DRM_ERROR for cases that are expected to fail, i.e. DRM_DEBUG_KMS is
> the right level.
>
> And I don't think this works correctly either. Assume you do an atomic
> modeset where you enable 2 mst sinks at the same time, and the following
> happens:
> - mst connector 1 can be allocated, and passes
>   intel_dp_mst_compute_config.
> - mst connector 2 can be allocated, but not together with connector 1.
>   But drm_dp_find_vcpi_slots only checks what's available, it doesn't do a
>   temporary reservation.
>
> And we can just reserve the slot in drm_dp_find_vcpi_slots, because then
> in the above case (when connector 2 doesn't have enough slots anymore)
> we'd leak the vcpi allocation for connector 1.
>
> Even worse, when we do a TEST_ONLY atomic commit (i.e. only run
> atomic_check/compute_config code, but not commit anything) this can even
> happen for a successful commit.
>
> Long story short: To fix this properly we need to rewrite the dp helpers
> to be compliant with atomic design. I think for that we roughly need:
>
> - Exract vcpi allocations into a free-standing state structure. I'd call
>   it drm_dp_mst_state or similar. Provide duplicate(get_state)/release
>   functions like we already have for plane, connector and crtc states in
>   the core, and e.g. dpll configuration in i915/intel_dpll_mgr.c. Ander
>   Conselvan can help you with this. I'm also planning to write better
>   documentation for how to do this exactly (since you also need a ww_mutex
>   to protect this state), and I'll prioritize that work.
>
> - Wire up that dp mst state at the right places globally (we need one slot
>   per drm_dp_mst_topology_mgr, i.e. per port), and duplicate that state in
>   intel_dp_mst_compute_config. We can't wire this up anywhere in the core
>   since the dp mst library is a helper library, so all the integration
>   points need to be done explicitly in drivers.
>
> - Do the same for nouveau - nouveau is now also atomic, and it also is
>   atomic with mst support.
>
> - While doing all that make sure that the existing (non-atomic-compliant)
>   functions in the dp mst helpers keep working, we need those for amggpu.
>
> - Create a new drm_dp_state_allocate_vcpi_slots, which only touches the
>   new drm_dp_mst_state structure and allocats the vcpi slots there. Also
>   add some function to find the allocation for each sink again (we need
>   that in the modeset commit functions).
>
> - Rework our compute_config and modeset code to use this new function, and
>   not the old legacy find/allocate functions.
>
> To make this happen we need buy-in from Ben Skeggs (nouveau maintainer)
> and preferrably also from the AMD display team (since they support mst
> already, and also plan to eventually support atomic).
>
> Fixing this correctly is unfortunately a _lot_ more work than your simple
> patch here :(

Adding Ben&Alex&Harry. Alex/Harry, pls pull in your mst expert into this too.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Pandiyan, Dhinakaran]

On Fri, 2016-11-18 at 09:43 +0100, Daniel Vetter wrote:
> On Thu, Nov 17, 2016 at 06:03:46PM -0800, Dhinakaran Pandiyan wrote:
> > drm_dp_find_vcpi_slots() returns an error when there is not enough
> > available bandwidth on a link to support a mode. This error should make
> > compute_config() to fail. Not returning false could end up in a modeset
> > which will not work.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index e21cf08..4280a83 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  
> >  	pipe_config->pbn = mst_pbn;
> >  	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> > +	if (slots < 0) {
> > +		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
> 
> No DRM_ERROR for cases that are expected to fail, i.e. DRM_DEBUG_KMS is
> the right level.
> 
It'd be nice to document the usage somewhere. There seems to be several
not very obvious reasons to choose one over the other. I can volunteer
to write it but I am not getting it right as it's evident here.


> And I don't think this works correctly either. Assume you do an atomic
> modeset where you enable 2 mst sinks at the same time, and the following
> happens:
> - mst connector 1 can be allocated, and passes
>   intel_dp_mst_compute_config.
> - mst connector 2 can be allocated, but not together with connector 1.
>   But drm_dp_find_vcpi_slots only checks what's available, it doesn't do a
>   temporary reservation.
> 
> And we can just reserve the slot in drm_dp_find_vcpi_slots, because then
> in the above case (when connector 2 doesn't have enough slots anymore)
> we'd leak the vcpi allocation for connector 1.
> 
> Even worse, when we do a TEST_ONLY atomic commit (i.e. only run
> atomic_check/compute_config code, but not commit anything) this can even
> happen for a successful commit.
> 
> Long story short: To fix this properly we need to rewrite the dp helpers
> to be compliant with atomic design. I think for that we roughly need:
> 
> - Exract vcpi allocations into a free-standing state structure. I'd call
>   it drm_dp_mst_state or similar. Provide duplicate(get_state)/release
>   functions like we already have for plane, connector and crtc states in
>   the core, and e.g. dpll configuration in i915/intel_dpll_mgr.c. Ander
>   Conselvan can help you with this. I'm also planning to write better
>   documentation for how to do this exactly (since you also need a ww_mutex
>   to protect this state), and I'll prioritize that work.
> 
> - Wire up that dp mst state at the right places globally (we need one slot
>   per drm_dp_mst_topology_mgr, i.e. per port), and duplicate that state in
>   intel_dp_mst_compute_config. We can't wire this up anywhere in the core
>   since the dp mst library is a helper library, so all the integration
>   points need to be done explicitly in drivers.
> 
> - Do the same for nouveau - nouveau is now also atomic, and it also is
>   atomic with mst support.
> 
> - While doing all that make sure that the existing (non-atomic-compliant)
>   functions in the dp mst helpers keep working, we need those for amggpu.
> 
> - Create a new drm_dp_state_allocate_vcpi_slots, which only touches the
>   new drm_dp_mst_state structure and allocats the vcpi slots there. Also
>   add some function to find the allocation for each sink again (we need
>   that in the modeset commit functions).
> 

Let me rephrase so that I am sure I understand this.
With the way that I am doing this (assuming the mutex bug in Patch 3/3
is fixed), we'll still end up passing compute_config() but fail modeset
in the scenario you pointed out. This is because the slots are not
reserved in drm_dp_find_vcpi_slots().

However, if we do reserve the slots for connector1 in
drm_dp_find_vcpi_slots() and fail compute_config() for connector2, then
the vcpi slots that were assigned to connector1 are not released.

But, by having drm_dp_mst_state, we can simulate vcpi slot allocation in
the atomic_check()/compute_config() phase and fail without committing,
while also releasing the slots. 


-DK


> - Rework our compute_config and modeset code to use this new function, and
>   not the old legacy find/allocate functions.
> 
> To make this happen we need buy-in from Ben Skeggs (nouveau maintainer)
> and preferrably also from the AMD display team (since they support mst
> already, and also plan to eventually support atomic).
> 
> Fixing this correctly is unfortunately a _lot_ more work than your simple
> patch here :(
> -Daniel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Fri, 2016-11-18 at 06:57 +0000, 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: [PATCH 2/3] drm/dp/mst: Calculate total link bandwidth instead of hardcoding it

[Pandiyan, Dhinakaran]

This patch along with https://patchwork.freedesktop.org/series/15305/
will fix https://bugs.freedesktop.org/show_bug.cgi?id=98141. I'd like
this to be reviewed independently since the other two patches in this
series require rework for atomic support.

-DK

On Thu, 2016-11-17 at 18:03 -0800, Dhinakaran Pandiyan wrote:
> The total or the nominal link bandwidth, which we save in terms of PBN, is
> a factor of link rate and lane count. But, currently we hardcode it to
> 2560 PBN. This results in incorrect computation of total slots.
> 
> E.g, 2 lane HBR2 configuration and 4k@60Hz, 24bpp mode
>   nominal link bw = 1080 MBps = 1280PBN = 64 slots
>   required bw 533.25 MHz*3 = 1599.75 MBps or 1896 PBN
>      with +0.6% margin = 1907.376 PBN = 96 slots
>   This is greater than the max. possible value of 64 slots. But, we
>   incorrectly compute available slots as 2560 PBN = 128 slots and don't
>   return error.
> 
> So, let's fix this by calculating the total link bandwidth as
> link bw (PBN) = BW per time slot(PBN) * max. time slots , where max. time
> slots is 64
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e4571..26dfd99 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2038,9 +2038,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  			ret = -EINVAL;
>  			goto out_unlock;
>  		}
> -
> -		mgr->total_pbn = 2560;
> -		mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> +		mgr->total_pbn = 64 * mgr->pbn_div;
> +		mgr->total_slots = 64;
>  		mgr->avail_slots = mgr->total_slots;
>  
>  		/* add initial branch device at LCT 1 */

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Daniel Vetter]

On Sat, Nov 19, 2016 at 12:04:32AM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2016-11-18 at 09:43 +0100, Daniel Vetter wrote:
> > On Thu, Nov 17, 2016 at 06:03:46PM -0800, Dhinakaran Pandiyan wrote:
> > > drm_dp_find_vcpi_slots() returns an error when there is not enough
> > > available bandwidth on a link to support a mode. This error should make
> > > compute_config() to fail. Not returning false could end up in a modeset
> > > which will not work.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index e21cf08..4280a83 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  
> > >  	pipe_config->pbn = mst_pbn;
> > >  	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> > > +	if (slots < 0) {
> > > +		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
> > 
> > No DRM_ERROR for cases that are expected to fail, i.e. DRM_DEBUG_KMS is
> > the right level.
> > 
> It'd be nice to document the usage somewhere. There seems to be several
> not very obvious reasons to choose one over the other. I can volunteer
> to write it but I am not getting it right as it's evident here.

Aw yes! I definitely want this better documented, and I can help out with
it for sure. Probably best we discuss this over irc, but would be really
awesome if you volunteer here (if just for the learning experience).

> > And I don't think this works correctly either. Assume you do an atomic
> > modeset where you enable 2 mst sinks at the same time, and the following
> > happens:
> > - mst connector 1 can be allocated, and passes
> >   intel_dp_mst_compute_config.
> > - mst connector 2 can be allocated, but not together with connector 1.
> >   But drm_dp_find_vcpi_slots only checks what's available, it doesn't do a
> >   temporary reservation.
> > 
> > And we can just reserve the slot in drm_dp_find_vcpi_slots, because then
> > in the above case (when connector 2 doesn't have enough slots anymore)
> > we'd leak the vcpi allocation for connector 1.
> > 
> > Even worse, when we do a TEST_ONLY atomic commit (i.e. only run
> > atomic_check/compute_config code, but not commit anything) this can even
> > happen for a successful commit.
> > 
> > Long story short: To fix this properly we need to rewrite the dp helpers
> > to be compliant with atomic design. I think for that we roughly need:
> > 
> > - Exract vcpi allocations into a free-standing state structure. I'd call
> >   it drm_dp_mst_state or similar. Provide duplicate(get_state)/release
> >   functions like we already have for plane, connector and crtc states in
> >   the core, and e.g. dpll configuration in i915/intel_dpll_mgr.c. Ander
> >   Conselvan can help you with this. I'm also planning to write better
> >   documentation for how to do this exactly (since you also need a ww_mutex
> >   to protect this state), and I'll prioritize that work.
> > 
> > - Wire up that dp mst state at the right places globally (we need one slot
> >   per drm_dp_mst_topology_mgr, i.e. per port), and duplicate that state in
> >   intel_dp_mst_compute_config. We can't wire this up anywhere in the core
> >   since the dp mst library is a helper library, so all the integration
> >   points need to be done explicitly in drivers.
> > 
> > - Do the same for nouveau - nouveau is now also atomic, and it also is
> >   atomic with mst support.
> > 
> > - While doing all that make sure that the existing (non-atomic-compliant)
> >   functions in the dp mst helpers keep working, we need those for amggpu.
> > 
> > - Create a new drm_dp_state_allocate_vcpi_slots, which only touches the
> >   new drm_dp_mst_state structure and allocats the vcpi slots there. Also
> >   add some function to find the allocation for each sink again (we need
> >   that in the modeset commit functions).
> > 
> 
> Let me rephrase so that I am sure I understand this.
> With the way that I am doing this (assuming the mutex bug in Patch 3/3
> is fixed), we'll still end up passing compute_config() but fail modeset
> in the scenario you pointed out. This is because the slots are not
> reserved in drm_dp_find_vcpi_slots().
> 
> However, if we do reserve the slots for connector1 in
> drm_dp_find_vcpi_slots() and fail compute_config() for connector2, then
> the vcpi slots that were assigned to connector1 are not released.
> 
> But, by having drm_dp_mst_state, we can simulate vcpi slot allocation in
> the atomic_check()/compute_config() phase and fail without committing,
> while also releasing the slots. 

Yup, that's the idea. And like I promised I'll try to document the general
principles of atomic a bit better, so that's it's easier to extend this
state handling concept to new things like mst. Rob Clark as me last week
about how to handle shared state between his hw plans, and we already have
special state to handle our shared dplls in i915, so there's demand for
this ;-)

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/dp/mst: Calculate total link bandwidth instead of hardcoding it

[Ville Syrjälä]

On Thu, Nov 17, 2016 at 06:03:47PM -0800, Dhinakaran Pandiyan wrote:
> The total or the nominal link bandwidth, which we save in terms of PBN, is
> a factor of link rate and lane count. But, currently we hardcode it to
> 2560 PBN. This results in incorrect computation of total slots.
> 
> E.g, 2 lane HBR2 configuration and 4k@60Hz, 24bpp mode
>   nominal link bw = 1080 MBps = 1280PBN = 64 slots
>   required bw 533.25 MHz*3 = 1599.75 MBps or 1896 PBN
>      with +0.6% margin = 1907.376 PBN = 96 slots
>   This is greater than the max. possible value of 64 slots. But, we
>   incorrectly compute available slots as 2560 PBN = 128 slots and don't
>   return error.
> 
> So, let's fix this by calculating the total link bandwidth as
> link bw (PBN) = BW per time slot(PBN) * max. time slots , where max. time
> slots is 64
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 04e4571..26dfd99 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2038,9 +2038,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  			ret = -EINVAL;
>  			goto out_unlock;
>  		}
> -
> -		mgr->total_pbn = 2560;
> -		mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> +		mgr->total_pbn = 64 * mgr->pbn_div;

Do we actually have a use in mind for total_pbn? It seems unused now.

> +		mgr->total_slots = 64;

Same for total_slots.

>  		mgr->avail_slots = mgr->total_slots;

So avail_slots is all that's used. And shouldn't it actually start
out at 63 instead of 64 on account of the MTP header always taking
up one slot?

>  
>  		/* add initial branch device at LCT 1 */
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/dp/mst: Calculate total link bandwidth instead of hardcoding it

[Pandiyan, Dhinakaran]

On Tue, 2016-11-29 at 22:58 +0200, Ville Syrjälä wrote:
> On Thu, Nov 17, 2016 at 06:03:47PM -0800, Dhinakaran Pandiyan wrote:
> > The total or the nominal link bandwidth, which we save in terms of PBN, is
> > a factor of link rate and lane count. But, currently we hardcode it to
> > 2560 PBN. This results in incorrect computation of total slots.
> > 
> > E.g, 2 lane HBR2 configuration and 4k@60Hz, 24bpp mode
> >   nominal link bw = 1080 MBps = 1280PBN = 64 slots
> >   required bw 533.25 MHz*3 = 1599.75 MBps or 1896 PBN
> >      with +0.6% margin = 1907.376 PBN = 96 slots
> >   This is greater than the max. possible value of 64 slots. But, we
> >   incorrectly compute available slots as 2560 PBN = 128 slots and don't
> >   return error.
> > 
> > So, let's fix this by calculating the total link bandwidth as
> > link bw (PBN) = BW per time slot(PBN) * max. time slots , where max. time
> > slots is 64
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 04e4571..26dfd99 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2038,9 +2038,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
> >  			ret = -EINVAL;
> >  			goto out_unlock;
> >  		}
> > -
> > -		mgr->total_pbn = 2560;
> > -		mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> > +		mgr->total_pbn = 64 * mgr->pbn_div;
> 
> Do we actually have a use in mind for total_pbn? It seems unused now.

Not really, I will remove it and submit this patch separately.

> 
> > +		mgr->total_slots = 64;
> 
> Same for total_slots.
> 
> >  		mgr->avail_slots = mgr->total_slots;
> 
> So avail_slots is all that's used. And shouldn't it actually start
> out at 63 instead of 64 on account of the MTP header always taking
> up one slot?
> 

Yeah, I had a check for < avail_slots in the patch that followed.


-DK
> >  
> >  		/* add initial branch device at LCT 1 */
> > -- 
> > 2.7.4
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx