Re: [PATCH v2 13/17] drm/v3d: Create a CPU job extension for the timestamp query job

[Maira Canal <mcanal@xxxxxxxxxx>]

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(&timestamp, 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;