lkml.org 
[lkml]   [2018]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 4/4] drm/v3d: Add support for submitting jobs to the TFU.
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?

\
 
 \ /
  Last update: 2018-11-28 20:46    [W:0.061 / U:1.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site