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:4.915 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site