[PATCH v2 13/17] drm/v3d: Create a CPU job extension for the timestamp query job
Maira Canal
mcanal at igalia.com
Mon Nov 27 13:26:16 UTC 2023
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_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;
>
More information about the dri-devel
mailing list