A few comments below.
In particular I think USECOEF handling is a bit broken?
Otherwise looks good to me.
> I think one interesting question here is if TFU hangs (has it ever hung,
> in our experience?) do we want to reset the whole V3D, or is the reset
> flag in the TFU block enough?
We've never seen the TFU hang AFAIK.
Seems prudent to handle anyway; what you've done looks fine to me.
I wouldn't try to reset the TFU on its own. I don't know if that TFU
reset bit has ever been tested!
> > @@ -251,6 +256,7 @@ static const struct drm_ioctl_desc v3d_drm_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(V3D_MMAP_BO, v3d_mmap_bo_ioctl, DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(V3D_GET_PARAM, v3d_get_param_ioctl, DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(V3D_GET_BO_OFFSET, v3d_get_bo_offset_ioctl, DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF_DRV(V3D_SUBMIT_TFU, v3d_submit_tfu_ioctl, DRM_RENDER_ALLOW | DRM_AUTH),
> > };
I would extend the comment above this block to note that DRM_AUTH is
currently required on SUBMIT_TFU because TFU commands are currently
not validated. (The TFU does not access memory via the GMP so I assume
we will want to explicitly validate commands instead?)
> > static void
> > v3d_unlock_bo_reservations(struct drm_device *dev,
dev not used? Wouldn't be needed by v3d_lock_bo_reservations either,
if it didn't need to be passed to unlock.
> > +static void
> > +v3d_tfu_job_cleanup(struct kref *ref)
> > +{
> > + struct v3d_tfu_job *job = container_of(ref, struct v3d_tfu_job,
> > + refcount);
> > + struct v3d_dev *v3d = job->v3d;
> > + unsigned int i;
> > +
> > + dma_fence_put(job->in_fence);
> > + dma_fence_put(job->done_fence);
> > +
> > + for (i = 0; i < ARRAY_SIZE(job->bo); i++)
> > + drm_gem_object_put_unlocked(&job->bo[i]->base);
This is a bit questionable. job->bo[i] may be NULL. &job->bo[i]->base
would work out as NULL too, but this strictly speaking invokes
undefined behaviour.
> > +#define V3D_TFU_INT_STS 0x00438
> > +#define V3D_TFU_INT_SET 0x0043c
> > +#define V3D_TFU_INT_CLR 0x00440
> > +#define V3D_TFU_INT_MSK_STS 0x00444
> > +#define V3D_TFU_INT_MSK_SET 0x00448
> > +#define V3D_TFU_INT_MSK_CLR 0x0044c
> > +#define V3D_TFU_INT_TFUC BIT(1)
> > +#define V3D_TFU_INT_TFUF BIT(0)
These just alias the HUB_CTL_INT registers.
They shouldn't be used.
I would probably avoid listing them here to avoid confusion.
> > + if (job->args.coef[0] & V3D_TFU_COEF0_USECOEF) {
> > + V3D_WRITE(V3D_TFU_COEF0, job->args.coef[0]);
> > + V3D_WRITE(V3D_TFU_COEF1, job->args.coef[1]);
> > + V3D_WRITE(V3D_TFU_COEF2, job->args.coef[2]);
> > + V3D_WRITE(V3D_TFU_COEF3, job->args.coef[3]);
> > + }
If USECOEF isn't set, still want to write COEF0 to clear the bit?
> > +#define DRM_IOCTL_V3D_SUBMIT_TFU DRM_IOWR(DRM_COMMAND_BASE + DRM_V3D_SUBMIT_TFU, struct drm_v3d_submit_tfu)
Should this not be DRM_IOW? No data is returned to userspace in the
drm_v3d_submit_tfu struct AFAICT?
> > + /* sync object to block on before submitting the TFU job. Each TFU
> > + * job will execute in the order submitted to its FD. Synchronization
> > + * against rendering jobs requires using sync objects.
> > + */
> > + __u32 in_sync;
"Submit" is used to mean two different things here. Maybe "before
submitting the TFU job" --> "before running the TFU job" to avoid
confusion?
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
