[PATCH 1/3] drm/i915: Fix VCS2's ring name.

[PATCH 1/3] drm/i915: Fix VCS2's ring name.

[Rodrigo Vivi]

It just fix a typo.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2faef26..c3f96a1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2224,7 +2224,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 		return -EINVAL;
 	}
 
-	ring->name = "bds2_ring";
+	ring->name = "bsd2_ring";
 	ring->id = VCS2;
 
 	ring->write_tail = ring_write_tail;
-- 
1.9.3

[PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Rodrigo Vivi]

On Broadwell GT3 we have 2 Video Command Streamers (VCS),
but userspace has no control when using VCS1 or VCS2. So we cannot test,
validate or debug specific changes or workaround that might affect only
one or another ring. So this patch introduces a mechanism to avoid the
ping-pong selection and use one specific ring given at boot time.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea596..7b6614f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2069,6 +2069,7 @@ struct i915_params {
 	int panel_ignore_lid;
 	unsigned int powersave;
 	int semaphores;
+	int dual_bsd_ring;
 	unsigned int lvds_downclock;
 	int lvds_channel_mode;
 	int panel_use_ssc;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d815ef5..09f350e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
+	int ring_id;
+	int dual = i915.dual_bsd_ring;
 
 	/* Check whether the file_priv is using one ring */
 	if (file_priv->bsd_ring)
 		return file_priv->bsd_ring->id;
-	else {
-		/* If no, use the ping-pong mechanism to select one ring */
-		int ring_id;
 
-		mutex_lock(&dev->struct_mutex);
-		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
-			ring_id = VCS;
-			dev_priv->mm.bsd_ring_dispatch_index = 1;
-		} else {
-			ring_id = VCS2;
-			dev_priv->mm.bsd_ring_dispatch_index = 0;
-		}
-		file_priv->bsd_ring = &dev_priv->ring[ring_id];
-		mutex_unlock(&dev->struct_mutex);
-		return ring_id;
+	/* If no, use the parameter defined or ping-pong mechanism
+	 * to select one ring */
+	mutex_lock(&dev->struct_mutex);
+
+	if (dual == 1 || (dual != 2 &&
+			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
+		ring_id = VCS;
+		dev_priv->mm.bsd_ring_dispatch_index = 1;
+	} else {
+		ring_id = VCS2;
+		dev_priv->mm.bsd_ring_dispatch_index = 0;
 	}
+
+	file_priv->bsd_ring = &dev_priv->ring[ring_id];
+	mutex_unlock(&dev->struct_mutex);
+
+	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
+	     file_priv->bsd_ring->name);
+	return ring_id;
 }
 
 static struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8145729..d4871c8 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
 	.panel_ignore_lid = 1,
 	.powersave = 1,
 	.semaphores = -1,
+	.dual_bsd_ring = 0,
 	.lvds_downclock = 0,
 	.lvds_channel_mode = 0,
 	.panel_use_ssc = -1,
@@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
 	"Use semaphores for inter-ring sync "
 	"(default: -1 (use per-chip defaults))");
 
+module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600);
+MODULE_PARM_DESC(dual_bsd_ring,
+	 "Specify bds rings for VCS when there are multiple VCSs available."
+	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
+
 module_param_named(enable_rc6, i915.enable_rc6, int, 0400);
 MODULE_PARM_DESC(enable_rc6,
 	"Enable power-saving render C-state 6. "
-- 
1.9.3

Re: [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Ben Widawsky]

On Mon, Jun 30, 2014 at 09:51:10AM -0700, Rodrigo Vivi wrote:
> On Broadwell GT3 we have 2 Video Command Streamers (VCS),
> but userspace has no control when using VCS1 or VCS2. So we cannot test,
> validate or debug specific changes or workaround that might affect only
> one or another ring. So this patch introduces a mechanism to avoid the
> ping-pong selection and use one specific ring given at boot time.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
I think going the debugfs route is the right thing to do if this should
move forward. If the goal is only for test, then I'm not really in favor
of moving it forward.

> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
>  3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8cea596..7b6614f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2069,6 +2069,7 @@ struct i915_params {
>  	int panel_ignore_lid;
>  	unsigned int powersave;
>  	int semaphores;
> +	int dual_bsd_ring;
>  	unsigned int lvds_downclock;
>  	int lvds_channel_mode;
>  	int panel_use_ssc;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d815ef5..09f350e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	int ring_id;
> +	int dual = i915.dual_bsd_ring;
>  
>  	/* Check whether the file_priv is using one ring */
>  	if (file_priv->bsd_ring)
>  		return file_priv->bsd_ring->id;
> -	else {
> -		/* If no, use the ping-pong mechanism to select one ring */
> -		int ring_id;
>  
> -		mutex_lock(&dev->struct_mutex);
> -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> -			ring_id = VCS;
> -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> -		} else {
> -			ring_id = VCS2;
> -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> -		}
> -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> -		mutex_unlock(&dev->struct_mutex);
> -		return ring_id;
> +	/* If no, use the parameter defined or ping-pong mechanism
> +	 * to select one ring */
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (dual == 1 || (dual != 2 &&
> +			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
> +		ring_id = VCS;
> +		dev_priv->mm.bsd_ring_dispatch_index = 1;
> +	} else {
> +		ring_id = VCS2;
> +		dev_priv->mm.bsd_ring_dispatch_index = 0;
>  	}
> +
> +	file_priv->bsd_ring = &dev_priv->ring[ring_id];
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
> +	     file_priv->bsd_ring->name);

Don't warn if this is valid module parameter people can use.

> +	return ring_id;
>  }
>  
>  static struct drm_i915_gem_object *
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8145729..d4871c8 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
>  	.panel_ignore_lid = 1,
>  	.powersave = 1,
>  	.semaphores = -1,
> +	.dual_bsd_ring = 0,
>  	.lvds_downclock = 0,
>  	.lvds_channel_mode = 0,
>  	.panel_use_ssc = -1,
> @@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
>  	"Use semaphores for inter-ring sync "
>  	"(default: -1 (use per-chip defaults))");
>  
> +module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600);
> +MODULE_PARM_DESC(dual_bsd_ring,
> +	 "Specify bds rings for VCS when there are multiple VCSs available."
> +	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
> +

It looks like life would be a lot easier if you did
0 = VCS
1 = VCS2
2 = BOTH

>  module_param_named(enable_rc6, i915.enable_rc6, int, 0400);
>  MODULE_PARM_DESC(enable_rc6,
>  	"Enable power-saving render C-state 6. "

Can't spot anything wrong, but again, I don't think the patch is worth
merging unless we can find a good use case.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Xiang, Haihao]

On Mon, 2014-06-30 at 09:51 -0700, Rodrigo Vivi wrote:
> On Broadwell GT3 we have 2 Video Command Streamers (VCS),
> but userspace has no control when using VCS1 or VCS2. So we cannot test,
> validate or debug specific changes or workaround that might affect only
> one or another ring. So this patch introduces a mechanism to avoid the
> ping-pong selection and use one specific ring given at boot time.

Hi, rodrigo

Could you use a mechanism to specify the ring at runtime ? If so, it is
flexible for us to use VCS ?

Thanks
Haihao

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
>  3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8cea596..7b6614f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2069,6 +2069,7 @@ struct i915_params {
>  	int panel_ignore_lid;
>  	unsigned int powersave;
>  	int semaphores;
> +	int dual_bsd_ring;
>  	unsigned int lvds_downclock;
>  	int lvds_channel_mode;
>  	int panel_use_ssc;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d815ef5..09f350e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	int ring_id;
> +	int dual = i915.dual_bsd_ring;
>  
>  	/* Check whether the file_priv is using one ring */
>  	if (file_priv->bsd_ring)
>  		return file_priv->bsd_ring->id;
> -	else {
> -		/* If no, use the ping-pong mechanism to select one ring */
> -		int ring_id;
>  
> -		mutex_lock(&dev->struct_mutex);
> -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> -			ring_id = VCS;
> -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> -		} else {
> -			ring_id = VCS2;
> -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> -		}
> -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> -		mutex_unlock(&dev->struct_mutex);
> -		return ring_id;
> +	/* If no, use the parameter defined or ping-pong mechanism
> +	 * to select one ring */
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (dual == 1 || (dual != 2 &&
> +			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
> +		ring_id = VCS;
> +		dev_priv->mm.bsd_ring_dispatch_index = 1;
> +	} else {
> +		ring_id = VCS2;
> +		dev_priv->mm.bsd_ring_dispatch_index = 0;
>  	}
> +
> +	file_priv->bsd_ring = &dev_priv->ring[ring_id];
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
> +	     file_priv->bsd_ring->name);
> +	return ring_id;
>  }
>  
>  static struct drm_i915_gem_object *
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8145729..d4871c8 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
>  	.panel_ignore_lid = 1,
>  	.powersave = 1,
>  	.semaphores = -1,
> +	.dual_bsd_ring = 0,
>  	.lvds_downclock = 0,
>  	.lvds_channel_mode = 0,
>  	.panel_use_ssc = -1,
> @@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
>  	"Use semaphores for inter-ring sync "
>  	"(default: -1 (use per-chip defaults))");
>  
> +module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600);
> +MODULE_PARM_DESC(dual_bsd_ring,
> +	 "Specify bds rings for VCS when there are multiple VCSs available."
> +	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
> +
>  module_param_named(enable_rc6, i915.enable_rc6, int, 0400);
>  MODULE_PARM_DESC(enable_rc6,
>  	"Enable power-saving render C-state 6. "

Re: [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Zhao, Yakui]

On Mon, 2014-06-30 at 10:51 -0600, Rodrigo Vivi wrote:
> On Broadwell GT3 we have 2 Video Command Streamers (VCS),
> but userspace has no control when using VCS1 or VCS2. So we cannot test,
> validate or debug specific changes or workaround that might affect only
> one or another ring. So this patch introduces a mechanism to avoid the
> ping-pong selection and use one specific ring given at boot time.

   If it is mainly used for the test/validation, can we add one override
flag so that the user-space app can explicitly declare which BSD ring is
used to dispatch the corresponding BSD commands? In such case it will
force to dispatch the corresponding commands on the ring passed by
user-application.

   At the same time this patch is not helpful under the following
scenario. For example: One application hopes to use the BSD Ring 0 while
another application hopes to use the BSD ring 1. 

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
>  3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8cea596..7b6614f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2069,6 +2069,7 @@ struct i915_params {
>  	int panel_ignore_lid;
>  	unsigned int powersave;
>  	int semaphores;
> +	int dual_bsd_ring;
>  	unsigned int lvds_downclock;
>  	int lvds_channel_mode;
>  	int panel_use_ssc;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d815ef5..09f350e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	int ring_id;
> +	int dual = i915.dual_bsd_ring;
>  
>  	/* Check whether the file_priv is using one ring */
>  	if (file_priv->bsd_ring)
>  		return file_priv->bsd_ring->id;
> -	else {
> -		/* If no, use the ping-pong mechanism to select one ring */
> -		int ring_id;
>  
> -		mutex_lock(&dev->struct_mutex);
> -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> -			ring_id = VCS;
> -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> -		} else {
> -			ring_id = VCS2;
> -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> -		}
> -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> -		mutex_unlock(&dev->struct_mutex);
> -		return ring_id;
> +	/* If no, use the parameter defined or ping-pong mechanism
> +	 * to select one ring */
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (dual == 1 || (dual != 2 &&
> +			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
> +		ring_id = VCS;
> +		dev_priv->mm.bsd_ring_dispatch_index = 1;
> +	} else {
> +		ring_id = VCS2;
> +		dev_priv->mm.bsd_ring_dispatch_index = 0;
>  	}
> +
> +	file_priv->bsd_ring = &dev_priv->ring[ring_id];
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
> +	     file_priv->bsd_ring->name);
> +	return ring_id;
>  }
>  
>  static struct drm_i915_gem_object *
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8145729..d4871c8 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
>  	.panel_ignore_lid = 1,
>  	.powersave = 1,
>  	.semaphores = -1,
> +	.dual_bsd_ring = 0,
>  	.lvds_downclock = 0,
>  	.lvds_channel_mode = 0,
>  	.panel_use_ssc = -1,
> @@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
>  	"Use semaphores for inter-ring sync "
>  	"(default: -1 (use per-chip defaults))");
>  
> +module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600);
> +MODULE_PARM_DESC(dual_bsd_ring,
> +	 "Specify bds rings for VCS when there are multiple VCSs available."
> +	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
> +
>  module_param_named(enable_rc6, i915.enable_rc6, int, 0400);
>  MODULE_PARM_DESC(enable_rc6,
>  	"Enable power-saving render C-state 6. "

Re: [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Vivi, Rodrigo]

It seems the flexibility on rings is more wanted and needed than I imagined.
Please ignore this patch here...

 I liked both execution flag or debugfs, but exec flag would cover this case of different applications using different command streamers.

With flags Would it be something like:
Execution without flag = ping-pong
Flag BSD1 use only VCS1
Flag BSD2 use only VCS2

Haihao, what do you think?

With debugfs would be something like i195_dual_bsd_ring file with 3 options: all bsd1 bsd2

Thanks,
Rodrigo.

-----Original Message-----
From: Zhao, Yakui 
Sent: Monday, June 30, 2014 6:37 PM
To: Vivi, Rodrigo
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

On Mon, 2014-06-30 at 10:51 -0600, Rodrigo Vivi wrote:
> On Broadwell GT3 we have 2 Video Command Streamers (VCS), but 
> userspace has no control when using VCS1 or VCS2. So we cannot test, 
> validate or debug specific changes or workaround that might affect 
> only one or another ring. So this patch introduces a mechanism to 
> avoid the ping-pong selection and use one specific ring given at boot time.

   If it is mainly used for the test/validation, can we add one override flag so that the user-space app can explicitly declare which BSD ring is used to dispatch the corresponding BSD commands? In such case it will force to dispatch the corresponding commands on the ring passed by user-application.

   At the same time this patch is not helpful under the following scenario. For example: One application hopes to use the BSD Ring 0 while another application hopes to use the BSD ring 1. 

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
>  3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> b/drivers/gpu/drm/i915/i915_drv.h index 8cea596..7b6614f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2069,6 +2069,7 @@ struct i915_params {
>  	int panel_ignore_lid;
>  	unsigned int powersave;
>  	int semaphores;
> +	int dual_bsd_ring;
>  	unsigned int lvds_downclock;
>  	int lvds_channel_mode;
>  	int panel_use_ssc;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d815ef5..09f350e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct 
> drm_device *dev,  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
> +	int ring_id;
> +	int dual = i915.dual_bsd_ring;
>  
>  	/* Check whether the file_priv is using one ring */
>  	if (file_priv->bsd_ring)
>  		return file_priv->bsd_ring->id;
> -	else {
> -		/* If no, use the ping-pong mechanism to select one ring */
> -		int ring_id;
>  
> -		mutex_lock(&dev->struct_mutex);
> -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> -			ring_id = VCS;
> -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> -		} else {
> -			ring_id = VCS2;
> -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> -		}
> -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> -		mutex_unlock(&dev->struct_mutex);
> -		return ring_id;
> +	/* If no, use the parameter defined or ping-pong mechanism
> +	 * to select one ring */
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (dual == 1 || (dual != 2 &&
> +			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
> +		ring_id = VCS;
> +		dev_priv->mm.bsd_ring_dispatch_index = 1;
> +	} else {
> +		ring_id = VCS2;
> +		dev_priv->mm.bsd_ring_dispatch_index = 0;
>  	}
> +
> +	file_priv->bsd_ring = &dev_priv->ring[ring_id];
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
> +	     file_priv->bsd_ring->name);
> +	return ring_id;
>  }
>  
>  static struct drm_i915_gem_object *
> diff --git a/drivers/gpu/drm/i915/i915_params.c 
> b/drivers/gpu/drm/i915/i915_params.c
> index 8145729..d4871c8 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
>  	.panel_ignore_lid = 1,
>  	.powersave = 1,
>  	.semaphores = -1,
> +	.dual_bsd_ring = 0,
>  	.lvds_downclock = 0,
>  	.lvds_channel_mode = 0,
>  	.panel_use_ssc = -1,
> @@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
>  	"Use semaphores for inter-ring sync "
>  	"(default: -1 (use per-chip defaults))");
>  
> +module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600); 
> +MODULE_PARM_DESC(dual_bsd_ring,
> +	 "Specify bds rings for VCS when there are multiple VCSs available."
> +	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
> +
>  module_param_named(enable_rc6, i915.enable_rc6, int, 0400);  
> MODULE_PARM_DESC(enable_rc6,
>  	"Enable power-saving render C-state 6. "

Re: [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Zhao, Yakui]

On Tue, 2014-07-01 at 09:26 -0600, Vivi, Rodrigo wrote:
> It seems the flexibility on rings is more wanted and needed than I imagined.
> Please ignore this patch here...
> 
>  I liked both execution flag or debugfs, but exec flag would cover this case of different applications using different command streamers.
> 
> With flags Would it be something like:
> Execution without flag = ping-pong
> Flag BSD1 use only VCS1
> Flag BSD2 use only VCS2
> 

IMO the execution flag looks reasonable. It can cover the flexibility of
different applications. In such case it can determine which ring is used
to dispatch command at runtime.

> Haihao, what do you think?
> 
> With debugfs would be something like i195_dual_bsd_ring file with 3 options: all bsd1 bsd2
> 
> Thanks,
> Rodrigo.
> 
> -----Original Message-----
> From: Zhao, Yakui 
> Sent: Monday, June 30, 2014 6:37 PM
> To: Vivi, Rodrigo
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.
> 
> On Mon, 2014-06-30 at 10:51 -0600, Rodrigo Vivi wrote:
> > On Broadwell GT3 we have 2 Video Command Streamers (VCS), but 
> > userspace has no control when using VCS1 or VCS2. So we cannot test, 
> > validate or debug specific changes or workaround that might affect 
> > only one or another ring. So this patch introduces a mechanism to 
> > avoid the ping-pong selection and use one specific ring given at boot time.
> 
>    If it is mainly used for the test/validation, can we add one override flag so that the user-space app can explicitly declare which BSD ring is used to dispatch the corresponding BSD commands? In such case it will force to dispatch the corresponding commands on the ring passed by user-application.
> 
>    At the same time this patch is not helpful under the following scenario. For example: One application hopes to use the BSD Ring 0 while another application hopes to use the BSD ring 1. 
> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h            |  1 +
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
> >  drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
> >  3 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h index 8cea596..7b6614f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2069,6 +2069,7 @@ struct i915_params {
> >  	int panel_ignore_lid;
> >  	unsigned int powersave;
> >  	int semaphores;
> > +	int dual_bsd_ring;
> >  	unsigned int lvds_downclock;
> >  	int lvds_channel_mode;
> >  	int panel_use_ssc;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
> > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index d815ef5..09f350e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct 
> > drm_device *dev,  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_file_private *file_priv = file->driver_priv;
> > +	int ring_id;
> > +	int dual = i915.dual_bsd_ring;
> >  
> >  	/* Check whether the file_priv is using one ring */
> >  	if (file_priv->bsd_ring)
> >  		return file_priv->bsd_ring->id;
> > -	else {
> > -		/* If no, use the ping-pong mechanism to select one ring */
> > -		int ring_id;
> >  
> > -		mutex_lock(&dev->struct_mutex);
> > -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> > -			ring_id = VCS;
> > -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> > -		} else {
> > -			ring_id = VCS2;
> > -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> > -		}
> > -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> > -		mutex_unlock(&dev->struct_mutex);
> > -		return ring_id;
> > +	/* If no, use the parameter defined or ping-pong mechanism
> > +	 * to select one ring */
> > +	mutex_lock(&dev->struct_mutex);
> > +
> > +	if (dual == 1 || (dual != 2 &&
> > +			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
> > +		ring_id = VCS;
> > +		dev_priv->mm.bsd_ring_dispatch_index = 1;
> > +	} else {
> > +		ring_id = VCS2;
> > +		dev_priv->mm.bsd_ring_dispatch_index = 0;
> >  	}
> > +
> > +	file_priv->bsd_ring = &dev_priv->ring[ring_id];
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
> > +	     file_priv->bsd_ring->name);
> > +	return ring_id;
> >  }
> >  
> >  static struct drm_i915_gem_object *
> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 8145729..d4871c8 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
> >  	.panel_ignore_lid = 1,
> >  	.powersave = 1,
> >  	.semaphores = -1,
> > +	.dual_bsd_ring = 0,
> >  	.lvds_downclock = 0,
> >  	.lvds_channel_mode = 0,
> >  	.panel_use_ssc = -1,
> > @@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
> >  	"Use semaphores for inter-ring sync "
> >  	"(default: -1 (use per-chip defaults))");
> >  
> > +module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600); 
> > +MODULE_PARM_DESC(dual_bsd_ring,
> > +	 "Specify bds rings for VCS when there are multiple VCSs available."
> > +	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
> > +
> >  module_param_named(enable_rc6, i915.enable_rc6, int, 0400);  
> > MODULE_PARM_DESC(enable_rc6,
> >  	"Enable power-saving render C-state 6. "
> 
> 

Re: [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.

[Xiang, Haihao]

On Wed, 2014-07-02 at 07:52 +0800, Zhao, Yakui wrote:
> On Tue, 2014-07-01 at 09:26 -0600, Vivi, Rodrigo wrote:
> > It seems the flexibility on rings is more wanted and needed than I imagined.
> > Please ignore this patch here...
> > 
> >  I liked both execution flag or debugfs, but exec flag would cover this case of different applications using different command streamers.
> > 
> > With flags Would it be something like:
> > Execution without flag = ping-pong
> > Flag BSD1 use only VCS1
> > Flag BSD2 use only VCS2
> > 
> 
> IMO the execution flag looks reasonable. It can cover the flexibility of
> different applications. In such case it can determine which ring is used
> to dispatch command at runtime.
> 

I prefer the execution flag too.

Thanks
Haihao


> > Haihao, what do you think?
> > 
> > With debugfs would be something like i195_dual_bsd_ring file with 3 options: all bsd1 bsd2
> > 
> > Thanks,
> > Rodrigo.
> > 
> > -----Original Message-----
> > From: Zhao, Yakui 
> > Sent: Monday, June 30, 2014 6:37 PM
> > To: Vivi, Rodrigo
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.
> > 
> > On Mon, 2014-06-30 at 10:51 -0600, Rodrigo Vivi wrote:
> > > On Broadwell GT3 we have 2 Video Command Streamers (VCS), but 
> > > userspace has no control when using VCS1 or VCS2. So we cannot test, 
> > > validate or debug specific changes or workaround that might affect 
> > > only one or another ring. So this patch introduces a mechanism to 
> > > avoid the ping-pong selection and use one specific ring given at boot time.
> > 
> >    If it is mainly used for the test/validation, can we add one override flag so that the user-space app can explicitly declare which BSD ring is used to dispatch the corresponding BSD commands? In such case it will force to dispatch the corresponding commands on the ring passed by user-application.
> > 
> >    At the same time this patch is not helpful under the following scenario. For example: One application hopes to use the BSD Ring 0 while another application hopes to use the BSD ring 1. 
> > 
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h            |  1 +
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++++++++++++++++------------
> > >  drivers/gpu/drm/i915/i915_params.c         |  6 ++++++
> > >  3 files changed, 27 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h index 8cea596..7b6614f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2069,6 +2069,7 @@ struct i915_params {
> > >  	int panel_ignore_lid;
> > >  	unsigned int powersave;
> > >  	int semaphores;
> > > +	int dual_bsd_ring;
> > >  	unsigned int lvds_downclock;
> > >  	int lvds_channel_mode;
> > >  	int panel_use_ssc;
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
> > > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index d815ef5..09f350e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct 
> > > drm_device *dev,  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct drm_i915_file_private *file_priv = file->driver_priv;
> > > +	int ring_id;
> > > +	int dual = i915.dual_bsd_ring;
> > >  
> > >  	/* Check whether the file_priv is using one ring */
> > >  	if (file_priv->bsd_ring)
> > >  		return file_priv->bsd_ring->id;
> > > -	else {
> > > -		/* If no, use the ping-pong mechanism to select one ring */
> > > -		int ring_id;
> > >  
> > > -		mutex_lock(&dev->struct_mutex);
> > > -		if (dev_priv->mm.bsd_ring_dispatch_index == 0) {
> > > -			ring_id = VCS;
> > > -			dev_priv->mm.bsd_ring_dispatch_index = 1;
> > > -		} else {
> > > -			ring_id = VCS2;
> > > -			dev_priv->mm.bsd_ring_dispatch_index = 0;
> > > -		}
> > > -		file_priv->bsd_ring = &dev_priv->ring[ring_id];
> > > -		mutex_unlock(&dev->struct_mutex);
> > > -		return ring_id;
> > > +	/* If no, use the parameter defined or ping-pong mechanism
> > > +	 * to select one ring */
> > > +	mutex_lock(&dev->struct_mutex);
> > > +
> > > +	if (dual == 1 || (dual != 2 &&
> > > +			  dev_priv->mm.bsd_ring_dispatch_index == 0)) {
> > > +		ring_id = VCS;
> > > +		dev_priv->mm.bsd_ring_dispatch_index = 1;
> > > +	} else {
> > > +		ring_id = VCS2;
> > > +		dev_priv->mm.bsd_ring_dispatch_index = 0;
> > >  	}
> > > +
> > > +	file_priv->bsd_ring = &dev_priv->ring[ring_id];
> > > +	mutex_unlock(&dev->struct_mutex);
> > > +
> > > +	WARN(dual, "Forcibly trying to use only one bsd ring. Using: %s\n",
> > > +	     file_priv->bsd_ring->name);
> > > +	return ring_id;
> > >  }
> > >  
> > >  static struct drm_i915_gem_object *
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index 8145729..d4871c8 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
> > >  	.panel_ignore_lid = 1,
> > >  	.powersave = 1,
> > >  	.semaphores = -1,
> > > +	.dual_bsd_ring = 0,
> > >  	.lvds_downclock = 0,
> > >  	.lvds_channel_mode = 0,
> > >  	.panel_use_ssc = -1,
> > > @@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
> > >  	"Use semaphores for inter-ring sync "
> > >  	"(default: -1 (use per-chip defaults))");
> > >  
> > > +module_param_named(dual_bsd_ring, i915.dual_bsd_ring, int, 0600); 
> > > +MODULE_PARM_DESC(dual_bsd_ring,
> > > +	 "Specify bds rings for VCS when there are multiple VCSs available."
> > > +	 "(0=All available bsd rings [default], 1=only VCS1, 2=only VCS2)");
> > > +
> > >  module_param_named(enable_rc6, i915.enable_rc6, int, 0400);  
> > > MODULE_PARM_DESC(enable_rc6,
> > >  	"Enable power-saving render C-state 6. "
> > 
> > 
> 
> 

[PATCH 3/3] drm/i915: Updating comments.

[Rodrigo Vivi]

ring index calculation table was out of date after other rings were added,
although the formula is flexible and scale when adding new rings.

So this patch just update the comments and add a brief explanation
why to use sync_seqno[ring index].

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         | 2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f6d1238..e85c85c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2842,6 +2842,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	idx = intel_ring_sync_index(from, to);
 
 	seqno = obj->last_read_seqno;
+	/* Optimization: Avoid semaphore sync when we are sure we already
+	 * waited for an object with higher seqno */
 	if (seqno <= from->semaphore.sync_seqno[idx])
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index e72017b..2e8b516 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -238,9 +238,11 @@ intel_ring_sync_index(struct intel_engine_cs *ring,
 	int idx;
 
 	/*
-	 * cs -> 0 = vcs, 1 = bcs
-	 * vcs -> 0 = bcs, 1 = cs,
-	 * bcs -> 0 = cs, 1 = vcs.
+	 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
+	 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
+	 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
+	 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
+	 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
 	 */
 
 	idx = (other - ring) - 1;
-- 
1.9.3

Re: [PATCH 3/3] drm/i915: Updating comments.

[Ben Widawsky]

On Mon, Jun 30, 2014 at 09:51:11AM -0700, Rodrigo Vivi wrote:
> ring index calculation table was out of date after other rings were added,
> although the formula is flexible and scale when adding new rings.
> 
> So this patch just update the comments and add a brief explanation
> why to use sync_seqno[ring index].
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f6d1238..e85c85c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2842,6 +2842,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  	idx = intel_ring_sync_index(from, to);
>  
>  	seqno = obj->last_read_seqno;
> +	/* Optimization: Avoid semaphore sync when we are sure we already
> +	 * waited for an object with higher seqno */
>  	if (seqno <= from->semaphore.sync_seqno[idx])
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index e72017b..2e8b516 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -238,9 +238,11 @@ intel_ring_sync_index(struct intel_engine_cs *ring,
>  	int idx;
>  
>  	/*
> -	 * cs -> 0 = vcs, 1 = bcs
> -	 * vcs -> 0 = bcs, 1 = cs,
> -	 * bcs -> 0 = cs, 1 = vcs.
> +	 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
> +	 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
> +	 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
> +	 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
> +	 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
>  	 */

I'd be a favor of dropping this table, and instead explaining the goal
of the math (to save the dword)
>  
>  	idx = (other - ring) - 1;

I'm guessing this hunk is from your private branch?

In any event, the topmost comment is a nice addition:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: Updating comments.

[Daniel Vetter]

On Mon, Jun 30, 2014 at 06:16:50PM -0700, Ben Widawsky wrote:
> On Mon, Jun 30, 2014 at 09:51:11AM -0700, Rodrigo Vivi wrote:
> > ring index calculation table was out of date after other rings were added,
> > although the formula is flexible and scale when adding new rings.
> > 
> > So this patch just update the comments and add a brief explanation
> > why to use sync_seqno[ring index].
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         | 2 ++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++---
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index f6d1238..e85c85c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2842,6 +2842,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
> >  	idx = intel_ring_sync_index(from, to);
> >  
> >  	seqno = obj->last_read_seqno;
> > +	/* Optimization: Avoid semaphore sync when we are sure we already
> > +	 * waited for an object with higher seqno */
> >  	if (seqno <= from->semaphore.sync_seqno[idx])
> >  		return 0;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index e72017b..2e8b516 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -238,9 +238,11 @@ intel_ring_sync_index(struct intel_engine_cs *ring,
> >  	int idx;
> >  
> >  	/*
> > -	 * cs -> 0 = vcs, 1 = bcs
> > -	 * vcs -> 0 = bcs, 1 = cs,
> > -	 * bcs -> 0 = cs, 1 = vcs.
> > +	 * rcs -> 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
> > +	 * vcs -> 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
> > +	 * bcs -> 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
> > +	 * vecs -> 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
> > +	 * vcs2 -> 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
> >  	 */
> 
> I'd be a favor of dropping this table, and instead explaining the goal
> of the math (to save the dword)

tbh I don't mind either way ...

> >  
> >  	idx = (other - ring) - 1;
> 
> I'm guessing this hunk is from your private branch?

Applied here without fuzz ...

> In any event, the topmost comment is a nice addition:

Indeed.

> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: Fix VCS2's ring name.

[Ben Widawsky]

On Mon, Jun 30, 2014 at 09:51:09AM -0700, Rodrigo Vivi wrote:
> It just fix a typo.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: Fix VCS2's ring name.

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Rodrigo Vivi
> Sent: Monday, June 30, 2014 5:51 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo
> Subject: [Intel-gfx] [PATCH 1/3] drm/i915: Fix VCS2's ring name.
> 
> It just fix a typo.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2faef26..c3f96a1 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2224,7 +2224,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device
> *dev)
>  		return -EINVAL;
>  	}
> 
> -	ring->name = "bds2_ring";
> +	ring->name = "bsd2_ring";
>  	ring->id = VCS2;
> 
>  	ring->write_tail = ring_write_tail;

Jus nitpicking but, wouldn´t it be better without the underscore, like the other rings?: "bsd2 ring"

[PATCH] drm/i915: Fix VCS2's ring name.

[Rodrigo Vivi]

It just fix a typo.

v2: removing underscore to let this like all other ring names (Oscar)

Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by (v1): Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2faef26..22c2b9a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2224,7 +2224,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 		return -EINVAL;
 	}
 
-	ring->name = "bds2_ring";
+	ring->name = "bsd2 ring";
 	ring->id = VCS2;
 
 	ring->write_tail = ring_write_tail;
-- 
1.9.3

Re: [PATCH] drm/i915: Fix VCS2's ring name.

[Daniel Vetter]

On Tue, Jul 01, 2014 at 02:41:36AM -0700, Rodrigo Vivi wrote:
> It just fix a typo.
> 
> v2: removing underscore to let this like all other ring names (Oscar)
> 
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Reviewed-by (v1): Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2faef26..22c2b9a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2224,7 +2224,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  		return -EINVAL;
>  	}
>  
> -	ring->name = "bds2_ring";
> +	ring->name = "bsd2 ring";
>  	ring->id = VCS2;
>  
>  	ring->write_tail = ring_write_tail;
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch