[RFC PATCH v2 17/17] WIP: drm/tegra: Implement new UAPI

[Mikko Perttunen]

On 9/9/20 3:47 AM, Dmitry Osipenko wrote:
> 05.09.2020 13:34, Mikko Perttunen пишет:
>> +static int submit_process_bufs(struct drm_device *drm, struct gather_bo *bo,
>> +			       struct tegra_drm_job_data *job_data,
>> +			       struct tegra_drm_channel_ctx *ctx,
>> +			       struct drm_tegra_channel_submit *args,
>> +			       struct ww_acquire_ctx *acquire_ctx)
>> +{
>> +	struct drm_tegra_submit_buf __user *user_bufs_ptr =
>> +		u64_to_user_ptr(args->bufs_ptr);
> 
> If assignment makes line too long, then factor it out.
> 
>    struct drm_tegra_submit_buf __user *user_bufs_ptr;
> 
>    user_bufs_ptr = u64_to_user_ptr(args->bufs_ptr);
> 
>> +	struct tegra_drm_mapping *mapping;
>> +	struct drm_tegra_submit_buf buf;
>> +	unsigned long copy_err;
>> +	int err;
>> +	u32 i;
>> +
>> +	job_data->used_mappings =
>> +		kcalloc(args->num_bufs, sizeof(*job_data->used_mappings), GFP_KERNEL);
> 
> The checkpatch should disallow this coding style. I'd write it as:
> 
> size_t size;
> 
> size = sizeof(*job_data->used_mappings);
> job_data->used_mappings = kcalloc(args->num_bufs, size..);

I'll make these cleaner for next version.

> 
>> +	if (!job_data->used_mappings)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < args->num_bufs; i++) {
>> +		copy_err = copy_from_user(&buf, user_bufs_ptr+i, sizeof(buf));
> 
> Whole array always should be copied at once. Please keep in mind that
> each copy_from_user() has a cpu-time cost, there should maximum up to 2
> copyings per job.
> 

OK. BTW, do you have some reference/numbers for this or is it based on 
grate-driver experience?

Mikko