Re: [PATCH v2 13/17] drm/v3d: Create a CPU job extension for the timestamp query job
Hi Iago,
On 11/27/23 06:16, Iago Toral wrote:
El jue, 23-11-2023 a las 21:47 -0300, Maíra Canal escribió:
(...)
+static void
+v3d_timestamp_query(struct v3d_cpu_job *job)
+{
+ struct v3d_timestamp_query_info *timestamp_query = &job-
timestamp_query;
+ struct v3d_bo *bo = to_v3d_bo(job->base.bo[0]);
I presume there is an implicit expectation here that a timestamp query
job only has one BO where we are writing query results, right? Maybe we
should document this more explicitly in the uAPI and also check that we
have at least that one BO before trying to map it and write to it?
I'll do it.
Also, is there a reason why we take the job reference from job-
base.bo[0] instead of job->bo[0]?
job is a struct v3d_cpu_job, which has a struct v3d_job as base.
The BOs are stored on struct v3d_job, as this is a common functionality
of all job types.
Best Regards,
- Maíra
Iago
+ u8 *value_addr;
+
+ v3d_get_bo_vaddr(bo);
+
+ for (int i = 0; i < timestamp_query->count; i++) {
+ value_addr = ((u8 *) bo->vaddr) + timestamp_query-
queries[i].offset;
+ *((u64 *) value_addr) = i == 0 ? ktime_get_ns() :
0ull;
+
+ drm_syncobj_replace_fence(timestamp_query-
queries[i].syncobj,
+ job->base.done_fence);
+ }
+
+ v3d_put_bo_vaddr(bo);
+}
+
static struct dma_fence *
v3d_cpu_job_run(struct drm_sched_job *sched_job)
{
@@ -315,6 +352,7 @@ v3d_cpu_job_run(struct drm_sched_job *sched_job)
void (*v3d_cpu_job_fn[])(struct v3d_cpu_job *job) = {
[V3D_CPU_JOB_TYPE_INDIRECT_CSD] =
v3d_rewrite_csd_job_wg_counts_from_indirect,
+ [V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY] =
v3d_timestamp_query,
};
v3d->cpu_job = job;
@@ -504,7 +542,7 @@ static const struct drm_sched_backend_ops
v3d_cache_clean_sched_ops = {
static const struct drm_sched_backend_ops v3d_cpu_sched_ops = {
.run_job = v3d_cpu_job_run,
.timedout_job = v3d_generic_job_timedout,
- .free_job = v3d_sched_job_free
+ .free_job = v3d_cpu_job_free
};
int
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
b/drivers/gpu/drm/v3d/v3d_submit.c
index b6707ef42706..2f03c8bca593 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -438,6 +438,64 @@ v3d_get_cpu_indirect_csd_params(struct drm_file
*file_priv,
NULL, &info->acquire_ctx);
}
+/* Get data for the query timestamp job submission. */
+static int
+v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv,
+ struct drm_v3d_extension __user
*ext,
+ struct v3d_cpu_job *job)
+{
+ u32 __user *offsets, *syncs;
+ struct drm_v3d_timestamp_query timestamp;
+
+ if (!job) {
+ DRM_DEBUG("CPU job extension was attached to a GPU
job.\n");
+ return -EINVAL;
+ }
+
+ if (job->job_type) {
+ DRM_DEBUG("Two CPU job extensions were added to the
same CPU job.\n");
+ return -EINVAL;
+ }
+
+ if (copy_from_user(×tamp, ext, sizeof(timestamp)))
+ return -EFAULT;
+
+ if (timestamp.pad)
+ return -EINVAL;
+
+ job->job_type = V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY;
+
+ job->timestamp_query.queries =
kvmalloc_array(timestamp.count,
+ sizeof(struct
v3d_timestamp_query),
+ GFP_KERNEL);
+ if (!job->timestamp_query.queries)
+ return -ENOMEM;
+
+ offsets = u64_to_user_ptr(timestamp.offsets);
+ syncs = u64_to_user_ptr(timestamp.syncs);
+
+ for (int i = 0; i < timestamp.count; i++) {
+ u32 offset, sync;
+
+ if (copy_from_user(&offset, offsets++,
sizeof(offset))) {
+ kvfree(job->timestamp_query.queries);
+ return -EFAULT;
+ }
+
+ job->timestamp_query.queries[i].offset = offset;
+
+ if (copy_from_user(&sync, syncs++, sizeof(sync))) {
+ kvfree(job->timestamp_query.queries);
+ return -EFAULT;
+ }
+
+ job->timestamp_query.queries[i].syncobj =
drm_syncobj_find(file_priv, sync);
+ }
+ job->timestamp_query.count = timestamp.count;
+
+ return 0;
+}
+
/* Whenever userspace sets ioctl extensions, v3d_get_extensions
parses data
* according to the extension id (name).
*/
@@ -466,6 +524,9 @@ v3d_get_extensions(struct drm_file *file_priv,
case DRM_V3D_EXT_ID_CPU_INDIRECT_CSD:
ret =
v3d_get_cpu_indirect_csd_params(file_priv, user_ext, job);
break;
+ case DRM_V3D_EXT_ID_CPU_TIMESTAMP_QUERY:
+ ret =
v3d_get_cpu_timestamp_que
Re: [PATCH v2 13/17] drm/v3d: Create a CPU job extension for the timestamp query job
El jue, 23-11-2023 a las 21:47 -0300, Maíra Canal escribió:
(...)
> +static void
> +v3d_timestamp_query(struct v3d_cpu_job *job)
> +{
> + struct v3d_timestamp_query_info *timestamp_query = &job-
> >timestamp_query;
> + struct v3d_bo *bo = to_v3d_bo(job->base.bo[0]);
I presume there is an implicit expectation here that a timestamp query
job only has one BO where we are writing query results, right? Maybe we
should document this more explicitly in the uAPI and also check that we
have at least that one BO before trying to map it and write to it?
Also, is there a reason why we take the job reference from job-
>base.bo[0] instead of job->bo[0]?
Iago
> + u8 *value_addr;
> +
> + v3d_get_bo_vaddr(bo);
> +
> + for (int i = 0; i < timestamp_query->count; i++) {
> + value_addr = ((u8 *) bo->vaddr) + timestamp_query-
> >queries[i].offset;
> + *((u64 *) value_addr) = i == 0 ? ktime_get_ns() :
> 0ull;
> +
> + drm_syncobj_replace_fence(timestamp_query-
> >queries[i].syncobj,
> + job->base.done_fence);
> + }
> +
> + v3d_put_bo_vaddr(bo);
> +}
> +
> static struct dma_fence *
> v3d_cpu_job_run(struct drm_sched_job *sched_job)
> {
> @@ -315,6 +352,7 @@ v3d_cpu_job_run(struct drm_sched_job *sched_job)
>
> void (*v3d_cpu_job_fn[])(struct v3d_cpu_job *job) = {
> [V3D_CPU_JOB_TYPE_INDIRECT_CSD] =
> v3d_rewrite_csd_job_wg_counts_from_indirect,
> + [V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY] =
> v3d_timestamp_query,
> };
>
> v3d->cpu_job = job;
> @@ -504,7 +542,7 @@ static const struct drm_sched_backend_ops
> v3d_cache_clean_sched_ops = {
> static const struct drm_sched_backend_ops v3d_cpu_sched_ops = {
> .run_job = v3d_cpu_job_run,
> .timedout_job = v3d_generic_job_timedout,
> - .free_job = v3d_sched_job_free
> + .free_job = v3d_cpu_job_free
> };
>
> int
> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
> b/drivers/gpu/drm/v3d/v3d_submit.c
> index b6707ef42706..2f03c8bca593 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -438,6 +438,64 @@ v3d_get_cpu_indirect_csd_params(struct drm_file
> *file_priv,
> NULL, &info->acquire_ctx);
> }
>
> +/* Get data for the query timestamp job submission. */
> +static int
> +v3d_get_cpu_timestamp_query_params(struct drm_file *file_priv,
> + struct drm_v3d_extension __user
> *ext,
> + struct v3d_cpu_job *job)
> +{
> + u32 __user *offsets, *syncs;
> + struct drm_v3d_timestamp_query timestamp;
> +
> + if (!job) {
> + DRM_DEBUG("CPU job extension was attached to a GPU
> job.\n");
> + return -EINVAL;
> + }
> +
> + if (job->job_type) {
> + DRM_DEBUG("Two CPU job extensions were added to the
> same CPU job.\n");
> + return -EINVAL;
> + }
> +
> + if (copy_from_user(×tamp, ext, sizeof(timestamp)))
> + return -EFAULT;
> +
> + if (timestamp.pad)
> + return -EINVAL;
> +
> + job->job_type = V3D_CPU_JOB_TYPE_TIMESTAMP_QUERY;
> +
> + job->timestamp_query.queries =
> kvmalloc_array(timestamp.count,
> + sizeof(struct
> v3d_timestamp_query),
> + GFP_KERNEL);
> + if (!job->timestamp_query.queries)
> + return -ENOMEM;
> +
> + offsets = u64_to_user_ptr(timestamp.offsets);
> + syncs = u64_to_user_ptr(timestamp.syncs);
> +
> + for (int i = 0; i < timestamp.count; i++) {
> + u32 offset, sync;
> +
> + if (copy_from_user(&offset, offsets++,
> sizeof(offset))) {
> + kvfree(job->timestamp_query.queries);
> + return -EFAULT;
> + }
> +
> + job->timestamp_query.queries[i].offset = offset;
> +
> + if (copy_from_user(&sync, syncs++, sizeof(sync))) {
> + kvfree(job->timestamp_query.queries);
> + return -EFAULT;
> + }
> +
> + job->timestamp_query.queries[i].syncobj =
> drm_syncobj_find(file_priv, sync);
> + }
> + job->timestamp_query.count = timestamp.count;
> +
> + return 0;
> +}
> +
> /* Whenever userspace sets ioctl extensions, v3d_get_extensions
> parses data
> * according to the extension id (name).
> */
> @@ -466,6 +524,9 @@ v3d_get_extensions(struct drm_file *file_priv,
> case DRM_V3D_EXT_ID_CPU_INDIRECT_CSD:
> ret =
> v3d_get_cpu_indirect_csd_params(file_priv, user_ext, job);
> break;
> + case DRM_V3D_EXT_ID_CPU_TIMESTAMP_QUERY:
> + ret =
> v3d_get_cpu_timestamp_query_params(fil
