diff mbox series

[5/5] drm/i915: add support for perf configuration queries

Message ID 20190521140855.3957-6-lionel.g.landwerlin@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Vulkan performance query support | expand

Commit Message

Lionel Landwerlin May 21, 2019, 2:08 p.m. UTC 
Listing configurations at the moment is supported only through sysfs.
This might cause issues for applications wanting to list
configurations from a container where sysfs isn't available.

This change adds a way to query the number of configurations and their
content through the i915 query uAPI.

v2: Fix sparse warnings (Lionel)
    Add support to query configuration using uuid (Lionel)

v3: Fix some inconsistency in uapi header (Lionel)
    Fix unlocking when not locked issue (Lionel)
    Add debug messages (Lionel)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |   6 +
 drivers/gpu/drm/i915/i915_perf.c  |   4 +
 drivers/gpu/drm/i915/i915_query.c | 277 ++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h       |  62 ++++++-
 4 files changed, 348 insertions(+), 1 deletion(-)

Comments

Dan Carpenter May 23, 2019, 10:32 a.m. UTC | #1 
Hi Lionel,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/drm-i915-Vulkan-performance-query-support/20190522-083115
base:   git://anongit.freedesktop.org/drm-intel for-linux-next

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/gpu/drm/i915/i915_query.c:290 query_perf_config_data() warn: inconsistent returns 'mutex:&i915->perf.metrics_lock'.
  Locked on:   line 220
  Unlocked on: line 170

# https://github.com/0day-ci/linux/commit/2df5c78741c44ada4e0b3b7b016923cbbb30ab76
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 2df5c78741c44ada4e0b3b7b016923cbbb30ab76
vim +290 drivers/gpu/drm/i915/i915_query.c

2df5c787 Lionel Landwerlin 2019-05-21  187  	if (__get_user(flags, &user_query_config_ptr->flags))
2df5c787 Lionel Landwerlin 2019-05-21  188  		return -EFAULT;
2df5c787 Lionel Landwerlin 2019-05-21  189  
2df5c787 Lionel Landwerlin 2019-05-21  190  	if (flags != 0)
2df5c787 Lionel Landwerlin 2019-05-21  191  		return -EINVAL;
2df5c787 Lionel Landwerlin 2019-05-21  192  
2df5c787 Lionel Landwerlin 2019-05-21  193  	ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Lock

2df5c787 Lionel Landwerlin 2019-05-21  194  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  195  		return ret;
2df5c787 Lionel Landwerlin 2019-05-21  196  
2df5c787 Lionel Landwerlin 2019-05-21  197  	if (use_uuid) {
2df5c787 Lionel Landwerlin 2019-05-21  198  		char uuid[UUID_STRING_LEN + 1] = { 0, };
2df5c787 Lionel Landwerlin 2019-05-21  199  		struct i915_oa_config *tmp;
2df5c787 Lionel Landwerlin 2019-05-21  200  		int id;
2df5c787 Lionel Landwerlin 2019-05-21  201  
2df5c787 Lionel Landwerlin 2019-05-21  202  		BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
2df5c787 Lionel Landwerlin 2019-05-21  203  
2df5c787 Lionel Landwerlin 2019-05-21  204  		if (__copy_from_user(uuid, user_query_config_ptr->uuid,
2df5c787 Lionel Landwerlin 2019-05-21  205  				     sizeof(user_query_config_ptr->uuid))) {
2df5c787 Lionel Landwerlin 2019-05-21  206  			ret = -EFAULT;
2df5c787 Lionel Landwerlin 2019-05-21  207  			goto out;
2df5c787 Lionel Landwerlin 2019-05-21  208  		}
2df5c787 Lionel Landwerlin 2019-05-21  209  
2df5c787 Lionel Landwerlin 2019-05-21  210  		idr_for_each_entry(&i915->perf.metrics_idr, tmp, id) {
2df5c787 Lionel Landwerlin 2019-05-21  211  			if (!strcmp(tmp->uuid, uuid)) {
2df5c787 Lionel Landwerlin 2019-05-21  212  				oa_config = tmp;
2df5c787 Lionel Landwerlin 2019-05-21  213  				break;
2df5c787 Lionel Landwerlin 2019-05-21  214  			}
2df5c787 Lionel Landwerlin 2019-05-21  215  		}
2df5c787 Lionel Landwerlin 2019-05-21  216  	} else {
2df5c787 Lionel Landwerlin 2019-05-21  217  		u64 config_id;
2df5c787 Lionel Landwerlin 2019-05-21  218  
2df5c787 Lionel Landwerlin 2019-05-21  219  		if (__get_user(config_id, &user_query_config_ptr->config))
2df5c787 Lionel Landwerlin 2019-05-21  220  			return -EFAULT;
                                                                ^^^^^^^^^^^^^^
This needs to be "goto out;"

2df5c787 Lionel Landwerlin 2019-05-21  221  
2df5c787 Lionel Landwerlin 2019-05-21  222  		if (config_id == 1)
2df5c787 Lionel Landwerlin 2019-05-21  223  			oa_config = &i915->perf.oa.test_config;
2df5c787 Lionel Landwerlin 2019-05-21  224  		else
2df5c787 Lionel Landwerlin 2019-05-21  225  			oa_config = idr_find(&i915->perf.metrics_idr, config_id);
2df5c787 Lionel Landwerlin 2019-05-21  226  	}
2df5c787 Lionel Landwerlin 2019-05-21  227  
2df5c787 Lionel Landwerlin 2019-05-21  228  	if (!oa_config) {
2df5c787 Lionel Landwerlin 2019-05-21  229  		ret = -ENOENT;
2df5c787 Lionel Landwerlin 2019-05-21  230  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  231  	}
2df5c787 Lionel Landwerlin 2019-05-21  232  
2df5c787 Lionel Landwerlin 2019-05-21  233  	if (__copy_from_user(&user_config, user_config_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  234  			     sizeof(user_config))) {
2df5c787 Lionel Landwerlin 2019-05-21  235  		ret = -EFAULT;
2df5c787 Lionel Landwerlin 2019-05-21  236  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  237  	}
2df5c787 Lionel Landwerlin 2019-05-21  238  
2df5c787 Lionel Landwerlin 2019-05-21  239  	ret = can_copy_perf_config_registers_or_number(user_config.n_boolean_regs,
2df5c787 Lionel Landwerlin 2019-05-21  240  						       user_config.boolean_regs_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  241  						       oa_config->b_counter_regs_len);
2df5c787 Lionel Landwerlin 2019-05-21  242  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  243  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  244  
2df5c787 Lionel Landwerlin 2019-05-21  245  	ret = can_copy_perf_config_registers_or_number(user_config.n_flex_regs,
2df5c787 Lionel Landwerlin 2019-05-21  246  						       user_config.flex_regs_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  247  						       oa_config->flex_regs_len);
2df5c787 Lionel Landwerlin 2019-05-21  248  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  249  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  250  
2df5c787 Lionel Landwerlin 2019-05-21  251  	ret = can_copy_perf_config_registers_or_number(user_config.n_mux_regs,
2df5c787 Lionel Landwerlin 2019-05-21  252  						       user_config.mux_regs_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  253  						       oa_config->mux_regs_len);
2df5c787 Lionel Landwerlin 2019-05-21  254  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  255  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  256  
2df5c787 Lionel Landwerlin 2019-05-21  257  	ret = copy_perf_config_registers_or_number(oa_config->b_counter_regs,
2df5c787 Lionel Landwerlin 2019-05-21  258  						   oa_config->b_counter_regs_len,
2df5c787 Lionel Landwerlin 2019-05-21  259  						   user_config.boolean_regs_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  260  						   &user_config.n_boolean_regs);
2df5c787 Lionel Landwerlin 2019-05-21  261  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  262  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  263  
2df5c787 Lionel Landwerlin 2019-05-21  264  	ret = copy_perf_config_registers_or_number(oa_config->flex_regs,
2df5c787 Lionel Landwerlin 2019-05-21  265  						   oa_config->flex_regs_len,
2df5c787 Lionel Landwerlin 2019-05-21  266  						   user_config.flex_regs_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  267  						   &user_config.n_flex_regs);
2df5c787 Lionel Landwerlin 2019-05-21  268  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  269  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  270  
2df5c787 Lionel Landwerlin 2019-05-21  271  	ret = copy_perf_config_registers_or_number(oa_config->mux_regs,
2df5c787 Lionel Landwerlin 2019-05-21  272  						   oa_config->mux_regs_len,
2df5c787 Lionel Landwerlin 2019-05-21  273  						   user_config.mux_regs_ptr,
2df5c787 Lionel Landwerlin 2019-05-21  274  						   &user_config.n_mux_regs);
2df5c787 Lionel Landwerlin 2019-05-21  275  	if (ret)
2df5c787 Lionel Landwerlin 2019-05-21  276  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  277  
2df5c787 Lionel Landwerlin 2019-05-21  278  	memcpy(user_config.uuid, oa_config->uuid, sizeof(user_config.uuid));
2df5c787 Lionel Landwerlin 2019-05-21  279  
2df5c787 Lionel Landwerlin 2019-05-21  280  	if (__copy_to_user(user_config_ptr, &user_config,
2df5c787 Lionel Landwerlin 2019-05-21  281  			   sizeof(user_config))) {
2df5c787 Lionel Landwerlin 2019-05-21  282  		ret = -EFAULT;
2df5c787 Lionel Landwerlin 2019-05-21  283  		goto out;
2df5c787 Lionel Landwerlin 2019-05-21  284  	}
2df5c787 Lionel Landwerlin 2019-05-21  285  
2df5c787 Lionel Landwerlin 2019-05-21  286  	ret = total_size;
2df5c787 Lionel Landwerlin 2019-05-21  287  
2df5c787 Lionel Landwerlin 2019-05-21  288  out:
2df5c787 Lionel Landwerlin 2019-05-21  289  	mutex_unlock(&i915->perf.metrics_lock);
2df5c787 Lionel Landwerlin 2019-05-21 @290  	return ret;
2df5c787 Lionel Landwerlin 2019-05-21  291  }
2df5c787 Lionel Landwerlin 2019-05-21  292  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Lionel Landwerlin May 23, 2019, 11:25 a.m. UTC | #2 
Hi Dan,

Not quite sure if you read responses to what seems like an automated 
message.
I have a question below.

Thanks,

-Lionel

On 23/05/2019 11:32, Dan Carpenter wrote:
> Hi Lionel,
>
> Thank you for the patch! Perhaps something to improve:
>
> url:    https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/drm-i915-Vulkan-performance-query-support/20190522-083115
> base:   git://anongit.freedesktop.org/drm-intel for-linux-next
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/gpu/drm/i915/i915_query.c:290 query_perf_config_data() warn: inconsistent returns 'mutex:&i915->perf.metrics_lock'.
>    Locked on:   line 220
>    Unlocked on: line 170
>
> # https://github.com/0day-ci/linux/commit/2df5c78741c44ada4e0b3b7b016923cbbb30ab76
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 2df5c78741c44ada4e0b3b7b016923cbbb30ab76
> vim +290 drivers/gpu/drm/i915/i915_query.c
>
> 2df5c787 Lionel Landwerlin 2019-05-21  187  	if (__get_user(flags, &user_query_config_ptr->flags))
> 2df5c787 Lionel Landwerlin 2019-05-21  188  		return -EFAULT;
> 2df5c787 Lionel Landwerlin 2019-05-21  189
> 2df5c787 Lionel Landwerlin 2019-05-21  190  	if (flags != 0)
> 2df5c787 Lionel Landwerlin 2019-05-21  191  		return -EINVAL;
> 2df5c787 Lionel Landwerlin 2019-05-21  192
> 2df5c787 Lionel Landwerlin 2019-05-21  193  	ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
>                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Lock


What do you mean by that?


>
> 2df5c787 Lionel Landwerlin 2019-05-21  194  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  195  		return ret;
> 2df5c787 Lionel Landwerlin 2019-05-21  196
> 2df5c787 Lionel Landwerlin 2019-05-21  197  	if (use_uuid) {
> 2df5c787 Lionel Landwerlin 2019-05-21  198  		char uuid[UUID_STRING_LEN + 1] = { 0, };
> 2df5c787 Lionel Landwerlin 2019-05-21  199  		struct i915_oa_config *tmp;
> 2df5c787 Lionel Landwerlin 2019-05-21  200  		int id;
> 2df5c787 Lionel Landwerlin 2019-05-21  201
> 2df5c787 Lionel Landwerlin 2019-05-21  202  		BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
> 2df5c787 Lionel Landwerlin 2019-05-21  203
> 2df5c787 Lionel Landwerlin 2019-05-21  204  		if (__copy_from_user(uuid, user_query_config_ptr->uuid,
> 2df5c787 Lionel Landwerlin 2019-05-21  205  				     sizeof(user_query_config_ptr->uuid))) {
> 2df5c787 Lionel Landwerlin 2019-05-21  206  			ret = -EFAULT;
> 2df5c787 Lionel Landwerlin 2019-05-21  207  			goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  208  		}
> 2df5c787 Lionel Landwerlin 2019-05-21  209
> 2df5c787 Lionel Landwerlin 2019-05-21  210  		idr_for_each_entry(&i915->perf.metrics_idr, tmp, id) {
> 2df5c787 Lionel Landwerlin 2019-05-21  211  			if (!strcmp(tmp->uuid, uuid)) {
> 2df5c787 Lionel Landwerlin 2019-05-21  212  				oa_config = tmp;
> 2df5c787 Lionel Landwerlin 2019-05-21  213  				break;
> 2df5c787 Lionel Landwerlin 2019-05-21  214  			}
> 2df5c787 Lionel Landwerlin 2019-05-21  215  		}
> 2df5c787 Lionel Landwerlin 2019-05-21  216  	} else {
> 2df5c787 Lionel Landwerlin 2019-05-21  217  		u64 config_id;
> 2df5c787 Lionel Landwerlin 2019-05-21  218
> 2df5c787 Lionel Landwerlin 2019-05-21  219  		if (__get_user(config_id, &user_query_config_ptr->config))
> 2df5c787 Lionel Landwerlin 2019-05-21  220  			return -EFAULT;
>                                                                  ^^^^^^^^^^^^^^
> This needs to be "goto out;"


Nice catch thanks!


>
> 2df5c787 Lionel Landwerlin 2019-05-21  221
> 2df5c787 Lionel Landwerlin 2019-05-21  222  		if (config_id == 1)
> 2df5c787 Lionel Landwerlin 2019-05-21  223  			oa_config = &i915->perf.oa.test_config;
> 2df5c787 Lionel Landwerlin 2019-05-21  224  		else
> 2df5c787 Lionel Landwerlin 2019-05-21  225  			oa_config = idr_find(&i915->perf.metrics_idr, config_id);
> 2df5c787 Lionel Landwerlin 2019-05-21  226  	}
> 2df5c787 Lionel Landwerlin 2019-05-21  227
> 2df5c787 Lionel Landwerlin 2019-05-21  228  	if (!oa_config) {
> 2df5c787 Lionel Landwerlin 2019-05-21  229  		ret = -ENOENT;
> 2df5c787 Lionel Landwerlin 2019-05-21  230  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  231  	}
> 2df5c787 Lionel Landwerlin 2019-05-21  232
> 2df5c787 Lionel Landwerlin 2019-05-21  233  	if (__copy_from_user(&user_config, user_config_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  234  			     sizeof(user_config))) {
> 2df5c787 Lionel Landwerlin 2019-05-21  235  		ret = -EFAULT;
> 2df5c787 Lionel Landwerlin 2019-05-21  236  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  237  	}
> 2df5c787 Lionel Landwerlin 2019-05-21  238
> 2df5c787 Lionel Landwerlin 2019-05-21  239  	ret = can_copy_perf_config_registers_or_number(user_config.n_boolean_regs,
> 2df5c787 Lionel Landwerlin 2019-05-21  240  						       user_config.boolean_regs_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  241  						       oa_config->b_counter_regs_len);
> 2df5c787 Lionel Landwerlin 2019-05-21  242  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  243  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  244
> 2df5c787 Lionel Landwerlin 2019-05-21  245  	ret = can_copy_perf_config_registers_or_number(user_config.n_flex_regs,
> 2df5c787 Lionel Landwerlin 2019-05-21  246  						       user_config.flex_regs_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  247  						       oa_config->flex_regs_len);
> 2df5c787 Lionel Landwerlin 2019-05-21  248  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  249  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  250
> 2df5c787 Lionel Landwerlin 2019-05-21  251  	ret = can_copy_perf_config_registers_or_number(user_config.n_mux_regs,
> 2df5c787 Lionel Landwerlin 2019-05-21  252  						       user_config.mux_regs_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  253  						       oa_config->mux_regs_len);
> 2df5c787 Lionel Landwerlin 2019-05-21  254  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  255  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  256
> 2df5c787 Lionel Landwerlin 2019-05-21  257  	ret = copy_perf_config_registers_or_number(oa_config->b_counter_regs,
> 2df5c787 Lionel Landwerlin 2019-05-21  258  						   oa_config->b_counter_regs_len,
> 2df5c787 Lionel Landwerlin 2019-05-21  259  						   user_config.boolean_regs_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  260  						   &user_config.n_boolean_regs);
> 2df5c787 Lionel Landwerlin 2019-05-21  261  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  262  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  263
> 2df5c787 Lionel Landwerlin 2019-05-21  264  	ret = copy_perf_config_registers_or_number(oa_config->flex_regs,
> 2df5c787 Lionel Landwerlin 2019-05-21  265  						   oa_config->flex_regs_len,
> 2df5c787 Lionel Landwerlin 2019-05-21  266  						   user_config.flex_regs_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  267  						   &user_config.n_flex_regs);
> 2df5c787 Lionel Landwerlin 2019-05-21  268  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  269  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  270
> 2df5c787 Lionel Landwerlin 2019-05-21  271  	ret = copy_perf_config_registers_or_number(oa_config->mux_regs,
> 2df5c787 Lionel Landwerlin 2019-05-21  272  						   oa_config->mux_regs_len,
> 2df5c787 Lionel Landwerlin 2019-05-21  273  						   user_config.mux_regs_ptr,
> 2df5c787 Lionel Landwerlin 2019-05-21  274  						   &user_config.n_mux_regs);
> 2df5c787 Lionel Landwerlin 2019-05-21  275  	if (ret)
> 2df5c787 Lionel Landwerlin 2019-05-21  276  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  277
> 2df5c787 Lionel Landwerlin 2019-05-21  278  	memcpy(user_config.uuid, oa_config->uuid, sizeof(user_config.uuid));
> 2df5c787 Lionel Landwerlin 2019-05-21  279
> 2df5c787 Lionel Landwerlin 2019-05-21  280  	if (__copy_to_user(user_config_ptr, &user_config,
> 2df5c787 Lionel Landwerlin 2019-05-21  281  			   sizeof(user_config))) {
> 2df5c787 Lionel Landwerlin 2019-05-21  282  		ret = -EFAULT;
> 2df5c787 Lionel Landwerlin 2019-05-21  283  		goto out;
> 2df5c787 Lionel Landwerlin 2019-05-21  284  	}
> 2df5c787 Lionel Landwerlin 2019-05-21  285
> 2df5c787 Lionel Landwerlin 2019-05-21  286  	ret = total_size;
> 2df5c787 Lionel Landwerlin 2019-05-21  287
> 2df5c787 Lionel Landwerlin 2019-05-21  288  out:
> 2df5c787 Lionel Landwerlin 2019-05-21  289  	mutex_unlock(&i915->perf.metrics_lock);
> 2df5c787 Lionel Landwerlin 2019-05-21 @290  	return ret;
> 2df5c787 Lionel Landwerlin 2019-05-21  291  }
> 2df5c787 Lionel Landwerlin 2019-05-21  292
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>
Dan Carpenter May 23, 2019, 11:38 a.m. UTC | #3 
On Thu, May 23, 2019 at 12:25:44PM +0100, Lionel Landwerlin wrote:
> Hi Dan,
> 
> Not quite sure if you read responses to what seems like an automated
> message.
> I have a question below.
> 
> Thanks,
> 
> -Lionel
> 
> On 23/05/2019 11:32, Dan Carpenter wrote:
> > Hi Lionel,
> > 
> > Thank you for the patch! Perhaps something to improve:
> > 
> > url:    https://github.com/0day-ci/linux/commits/Lionel-Landwerlin/drm-i915-Vulkan-performance-query-support/20190522-083115
> > base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> > 
> > If you fix the issue, kindly add following tag
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > smatch warnings:
> > drivers/gpu/drm/i915/i915_query.c:290 query_perf_config_data() warn: inconsistent returns 'mutex:&i915->perf.metrics_lock'.
> >    Locked on:   line 220
> >    Unlocked on: line 170
> > 
> > # https://github.com/0day-ci/linux/commit/2df5c78741c44ada4e0b3b7b016923cbbb30ab76
> > git remote add linux-review https://github.com/0day-ci/linux
> > git remote update linux-review
> > git checkout 2df5c78741c44ada4e0b3b7b016923cbbb30ab76
> > vim +290 drivers/gpu/drm/i915/i915_query.c
> > 
> > 2df5c787 Lionel Landwerlin 2019-05-21  187  	if (__get_user(flags, &user_query_config_ptr->flags))
> > 2df5c787 Lionel Landwerlin 2019-05-21  188  		return -EFAULT;
> > 2df5c787 Lionel Landwerlin 2019-05-21  189
> > 2df5c787 Lionel Landwerlin 2019-05-21  190  	if (flags != 0)
> > 2df5c787 Lionel Landwerlin 2019-05-21  191  		return -EINVAL;
> > 2df5c787 Lionel Landwerlin 2019-05-21  192
> > 2df5c787 Lionel Landwerlin 2019-05-21  193  	ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
> >                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Lock
> 
> 
> What do you mean by that?
> 

Just observing that we take the lock on this line.

:P

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 25860d99ffc6..6127c6890e0b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1875,6 +1875,12 @@  struct drm_i915_private {
 		 */
 		struct list_head metrics_buffers;
 
+		/*
+		 * Number of dynamic configurations, you need to hold
+		 * dev_priv->perf.metrics_lock to access it.
+		 */
+		u32 n_metrics;
+
 		/*
 		 * Lock associated with anything below within this structure
 		 * except exclusive_stream.
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 7b861f12f161..6ab414e0ba1c 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -3432,6 +3432,8 @@  int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
 		goto sysfs_err;
 	}
 
+	dev_priv->perf.n_metrics++;
+
 	mutex_unlock(&dev_priv->perf.metrics_lock);
 
 	DRM_DEBUG("Added config %s id=%i\n", oa_config->uuid, oa_config->id);
@@ -3493,6 +3495,8 @@  int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
 
 	idr_remove(&dev_priv->perf.metrics_idr, *arg);
 
+	dev_priv->perf.n_metrics--;
+
 	DRM_DEBUG("Removed config %s id=%i\n", oa_config->uuid, oa_config->id);
 
 	i915_oa_config_put(oa_config);
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 782183b78f49..82bd6d973527 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -96,9 +96,286 @@  static int query_topology_info(struct drm_i915_private *dev_priv,
 	return total_length;
 }
 
+static int can_copy_perf_config_registers_or_number(u32 user_n_regs,
+						    u64 user_regs_ptr,
+						    u32 kernel_n_regs)
+{
+	/*
+	 * We'll just put the number of registers, and won't copy the
+	 * register.
+	 */
+	if (user_n_regs == 0)
+		return 0;
+
+	if (user_n_regs < kernel_n_regs)
+		return -EINVAL;
+
+	if (!access_ok(u64_to_user_ptr(user_regs_ptr),
+		       2 * sizeof(u32) * kernel_n_regs))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int copy_perf_config_registers_or_number(const struct i915_oa_reg *kernel_regs,
+						u32 kernel_n_regs,
+						u64 user_regs_ptr,
+						u32 *user_n_regs)
+{
+	u32 r;
+
+	if (*user_n_regs == 0) {
+		*user_n_regs = kernel_n_regs;
+		return 0;
+	}
+
+	*user_n_regs = kernel_n_regs;
+
+	for (r = 0; r < kernel_n_regs; r++) {
+		u32 __user *user_reg_ptr =
+			u64_to_user_ptr(user_regs_ptr + sizeof(u32) * r * 2);
+		u32 __user *user_val_ptr =
+			u64_to_user_ptr(user_regs_ptr + sizeof(u32) * r * 2 +
+					sizeof(u32));
+		int ret;
+
+		ret = __put_user(i915_mmio_reg_offset(kernel_regs[r].addr),
+				 user_reg_ptr);
+		if (ret)
+			return -EFAULT;
+
+		ret = __put_user(kernel_regs[r].value, user_val_ptr);
+		if (ret)
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
+static int query_perf_config_data(struct drm_i915_private *i915,
+				  struct drm_i915_query_item *query_item,
+				  bool use_uuid)
+{
+	struct drm_i915_query_perf_config __user *user_query_config_ptr =
+		u64_to_user_ptr(query_item->data_ptr);
+	struct drm_i915_perf_oa_config __user *user_config_ptr =
+		u64_to_user_ptr(query_item->data_ptr +
+				sizeof(struct drm_i915_query_perf_config));
+	struct drm_i915_perf_oa_config user_config;
+	struct i915_oa_config *oa_config = NULL;
+	u32 flags, total_size;
+	int ret;
+
+	if (!i915->perf.initialized)
+		return -ENODEV;
+
+	total_size = sizeof(struct drm_i915_query_perf_config) +
+		sizeof(struct drm_i915_perf_oa_config);
+
+	if (query_item->length == 0)
+		return total_size;
+
+	if (query_item->length < total_size) {
+		DRM_DEBUG("Invalid query config data item size=%u expected=%u\n",
+			  query_item->length, total_size);
+		return -EINVAL;
+	}
+
+	if (!access_ok(user_query_config_ptr, total_size))
+		return -EFAULT;
+
+	if (__get_user(flags, &user_query_config_ptr->flags))
+		return -EFAULT;
+
+	if (flags != 0)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
+	if (ret)
+		return ret;
+
+	if (use_uuid) {
+		char uuid[UUID_STRING_LEN + 1] = { 0, };
+		struct i915_oa_config *tmp;
+		int id;
+
+		BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
+
+		if (__copy_from_user(uuid, user_query_config_ptr->uuid,
+				     sizeof(user_query_config_ptr->uuid))) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		idr_for_each_entry(&i915->perf.metrics_idr, tmp, id) {
+			if (!strcmp(tmp->uuid, uuid)) {
+				oa_config = tmp;
+				break;
+			}
+		}
+	} else {
+		u64 config_id;
+
+		if (__get_user(config_id, &user_query_config_ptr->config))
+			return -EFAULT;
+
+		if (config_id == 1)
+			oa_config = &i915->perf.oa.test_config;
+		else
+			oa_config = idr_find(&i915->perf.metrics_idr, config_id);
+	}
+
+	if (!oa_config) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (__copy_from_user(&user_config, user_config_ptr,
+			     sizeof(user_config))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = can_copy_perf_config_registers_or_number(user_config.n_boolean_regs,
+						       user_config.boolean_regs_ptr,
+						       oa_config->b_counter_regs_len);
+	if (ret)
+		goto out;
+
+	ret = can_copy_perf_config_registers_or_number(user_config.n_flex_regs,
+						       user_config.flex_regs_ptr,
+						       oa_config->flex_regs_len);
+	if (ret)
+		goto out;
+
+	ret = can_copy_perf_config_registers_or_number(user_config.n_mux_regs,
+						       user_config.mux_regs_ptr,
+						       oa_config->mux_regs_len);
+	if (ret)
+		goto out;
+
+	ret = copy_perf_config_registers_or_number(oa_config->b_counter_regs,
+						   oa_config->b_counter_regs_len,
+						   user_config.boolean_regs_ptr,
+						   &user_config.n_boolean_regs);
+	if (ret)
+		goto out;
+
+	ret = copy_perf_config_registers_or_number(oa_config->flex_regs,
+						   oa_config->flex_regs_len,
+						   user_config.flex_regs_ptr,
+						   &user_config.n_flex_regs);
+	if (ret)
+		goto out;
+
+	ret = copy_perf_config_registers_or_number(oa_config->mux_regs,
+						   oa_config->mux_regs_len,
+						   user_config.mux_regs_ptr,
+						   &user_config.n_mux_regs);
+	if (ret)
+		goto out;
+
+	memcpy(user_config.uuid, oa_config->uuid, sizeof(user_config.uuid));
+
+	if (__copy_to_user(user_config_ptr, &user_config,
+			   sizeof(user_config))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = total_size;
+
+out:
+	mutex_unlock(&i915->perf.metrics_lock);
+	return ret;
+}
+
+static int query_perf_config_list(struct drm_i915_private *i915,
+				  struct drm_i915_query_item *query_item)
+{
+	struct drm_i915_query_perf_config __user *user_query_config_ptr =
+		u64_to_user_ptr(query_item->data_ptr);
+	struct i915_oa_config *oa_config;
+	u32 flags, total_size;
+	u64 n_configs;
+	int ret, id;
+
+	if (!i915->perf.initialized)
+		return -ENODEV;
+
+	/* Count the default test configuration */
+	n_configs = i915->perf.n_metrics + 1;
+	total_size = sizeof(struct drm_i915_query_perf_config) +
+		sizeof(u64) * n_configs;
+
+	if (query_item->length == 0)
+		return total_size;
+
+	if (query_item->length < total_size) {
+		DRM_DEBUG("Invalid query config list item size=%u expected=%u\n",
+			  query_item->length, total_size);
+		return -EINVAL;
+	}
+
+	if (!access_ok(user_query_config_ptr, total_size))
+		return -EFAULT;
+
+	if (__get_user(flags, &user_query_config_ptr->flags))
+		return -EFAULT;
+
+	if (flags != 0)
+		return -EINVAL;
+
+	if (__put_user(n_configs, &user_query_config_ptr->config))
+		return -EFAULT;
+
+	if (__put_user((u64)1ULL, &user_query_config_ptr->data[0]))
+		return -EFAULT;
+
+	ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
+	if (ret)
+		return ret;
+
+	n_configs = 1;
+	idr_for_each_entry(&i915->perf.metrics_idr, oa_config, id) {
+		u64 __user *item =
+			u64_to_user_ptr(query_item->data_ptr +
+					sizeof(struct drm_i915_query_perf_config) +
+					n_configs * sizeof(u64));
+
+		if (__put_user((u64)id, item)) {
+			ret = -EFAULT;
+			goto out;
+		}
+		n_configs++;
+	}
+
+	ret = total_size;
+
+out:
+	mutex_unlock(&i915->perf.metrics_lock);
+	return ret;
+}
+
+static int query_perf_config(struct drm_i915_private *i915,
+			     struct drm_i915_query_item *query_item)
+{
+	switch (query_item->flags) {
+	case DRM_I915_QUERY_PERF_CONFIG_LIST:
+		return query_perf_config_list(i915, query_item);
+	case DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID:
+		return query_perf_config_data(i915, query_item, true);
+	case DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_ID:
+		return query_perf_config_data(i915, query_item, false);
+	default:
+		return -EINVAL;
+	}
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
 					struct drm_i915_query_item *query_item) = {
 	query_topology_info,
+	query_perf_config,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index e57fb5f249da..aafe7a3569ef 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1869,6 +1869,7 @@  struct drm_i915_perf_oa_config {
 struct drm_i915_query_item {
 	__u64 query_id;
 #define DRM_I915_QUERY_TOPOLOGY_INFO    1
+#define DRM_I915_QUERY_PERF_CONFIG      2
 /* Must be kept compact -- no holes and well documented */
 
 	/*
@@ -1880,9 +1881,18 @@  struct drm_i915_query_item {
 	__s32 length;
 
 	/*
-	 * Unused for now. Must be cleared to zero.
+	 * When query_id == DRM_I915_QUERY_TOPOLOGY_INFO, must be 0.
+	 *
+	 * When query_id == DRM_I915_QUERY_PERF_CONFIG, must be one of the
+	 * following :
+	 *         - DRM_I915_QUERY_PERF_CONFIG_LIST
+	 *         - DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID
+	 *         - DRM_I915_QUERY_PERF_CONFIG_FOR_UUID
 	 */
 	__u32 flags;
+#define DRM_I915_QUERY_PERF_CONFIG_LIST          1
+#define DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID 2
+#define DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_ID   3
 
 	/*
 	 * Data will be written at the location pointed by data_ptr when the
@@ -1967,6 +1977,56 @@  struct drm_i915_query_topology_info {
 	__u8 data[];
 };
 
+/*
+ * Data written by the kernel with query DRM_I915_QUERY_PERF_CONFIG.
+ */
+struct drm_i915_query_perf_config {
+	union {
+		/*
+		 * When query_item.flags == DRM_I915_QUERY_PERF_CONFIG_LIST, i915 sets
+		 * this fields to the number of configurations available.
+		 */
+		__u64 n_configs;
+
+		/*
+		 * When query_id == DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_ID,
+		 * i915 will use the value in this field as configuration
+		 * identifier to decide what data to write into config_ptr.
+		 */
+		__u64 config;
+
+		/*
+		 * When query_id == DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID,
+		 * i915 will use the value in this field as configuration
+		 * identifier to decide what data to write into config_ptr.
+		 *
+		 * String formatted like "%08x-%04x-%04x-%04x-%012x"
+		 */
+		char uuid[36];
+	};
+
+	/*
+	 * Unused for now. Must be cleared to zero.
+	 */
+	__u32 flags;
+
+	/*
+	 * When query_item.flags == DRM_I915_QUERY_PERF_CONFIG_LIST, i915 will
+	 * write an array of __u64 of configuration identifiers.
+	 *
+	 * When query_item.flags == DRM_I915_QUERY_PERF_CONFIG_DATA, i915 will
+	 * write a struct drm_i915_perf_oa_config. If the following fields of
+	 * drm_i915_perf_oa_config are set not set to 0, i915 will write into
+	 * the associated pointers the values of submitted when the
+	 * configuration was created :
+	 *
+	 *         - n_mux_regs
+	 *         - n_boolean_regs
+	 *         - n_flex_regs
+	 */
+	__u8 data[];
+};
+
 #if defined(__cplusplus)
 }
 #endif