Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181108161654.19888-1-eric@anholt.net> <20181108161654.19888-5-eric@anholt.net>
 <87ftw4y6rc.fsf@anholt.net>
In-Reply-To: <87ftw4y6rc.fsf@anholt.net>
From:   Dave Emett <david.emett@broadcom.com>
Date:   Wed, 28 Nov 2018 19:45:47 +0000
Message-ID: <CAApk1dWrj3gvvOXAaU1e17Ec1+GpCccCb5pKwyrGUomwSQo5qg@mail.gmail.com>
Subject: Re: [PATCH 4/4] drm/v3d: Add support for submitting jobs to the TFU.
To:     Eric Anholt <eric@anholt.net>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        boris.brezillon@bootlin.com, daniel.vetter@ffwll.ch
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?