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(file_priv, user_ext, job);
> + break;
> default:
> DRM_DEBUG_DRIVER("Unknown extension id:
> %d\n", ext.id);
> return -EINVAL;
> @@ -954,6 +1015,7 @@ v3d_submit_cpu_ioctl(struct drm_device *dev,
> void *data,
> v3d_job_cleanup((void *)csd_job);
> v3d_job_cleanup(clean_job);
> v3d_put_multisync_post_deps(&se);
> + kvfree(cpu_job->timestamp_query.queries);
>
> return ret;
> }
> diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h
> index 059b84984fb0..65d5de076366 100644
> --- a/include/uapi/drm/v3d_drm.h
> +++ b/include/uapi/drm/v3d_drm.h
> @@ -73,6 +73,7 @@ struct drm_v3d_extension {
> __u32 id;
> #define DRM_V3D_EXT_ID_MULTI_SYNC 0x01
> #define DRM_V3D_EXT_ID_CPU_INDIRECT_CSD 0x02
> +#define DRM_V3D_EXT_ID_CPU_TIMESTAMP_QUERY 0x03
> __u32 flags; /* mbz */
> };
>
> @@ -400,6 +401,32 @@ struct drm_v3d_indirect_csd {
> __u32 wg_uniform_offsets[3];
> };
>
> +/**
> + * struct drm_v3d_timestamp_query - ioctl extension for the CPU job
> to calculate
> + * a timestamp query
> + *
> + * When an extension DRM_V3D_EXT_ID_TIMESTAMP_QUERY is defined, it
> points to
> + * this extension to define a timestamp query submission. This CPU
> job will
> + * calculate the timestamp query and update the query value within
> the
> + * timestamp BO. Moreover, it will signal the timestamp syncobj to
> indicate
> + * query availability.
> + */
> +struct drm_v3d_timestamp_query {
> + struct drm_v3d_extension base;
> +
> + /* Array of queries' offsets within the timestamp BO for
> their value */
> + __u64 offsets;
> +
> + /* Array of timestamp's syncobjs to indicate its availability
> */
> + __u64 syncs;
> +
> + /* Number of queries */
> + __u32 count;
> +
> + /* mbz */
> + __u32 pad;
> +};
> +
> struct drm_v3d_submit_cpu {
> /* Pointer to a u32 array of the BOs that are referenced by
> the job. */
> __u64 bo_handles;
