linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Emett <david.emett@broadcom.com>
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
Subject: Re: [PATCH 4/4] drm/v3d: Add support for submitting jobs to the TFU.
Date: Wed, 28 Nov 2018 19:45:47 +0000	[thread overview]
Message-ID: <CAApk1dWrj3gvvOXAaU1e17Ec1+GpCccCb5pKwyrGUomwSQo5qg@mail.gmail.com> (raw)
In-Reply-To: <87ftw4y6rc.fsf@anholt.net>

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?

      reply	other threads:[~2018-11-28 19:46 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-08 16:16 [PATCH 0/4] V3D TFU engine support Eric Anholt
2018-11-08 16:16 ` [PATCH 1/4] drm/v3d: Fix whitespace inconsistency in the header Eric Anholt
2018-11-13 10:22   ` Boris Brezillon
2018-11-08 16:16 ` [PATCH 2/4] drm/v3d: Update a comment about what uses v3d_job_dependency() Eric Anholt
2018-11-13 10:22   ` Boris Brezillon
2018-11-08 16:16 ` [PATCH 3/4] drm/v3d: Clean up the reservation object setup Eric Anholt
2018-11-13 10:22   ` Boris Brezillon
2018-11-08 16:16 ` [PATCH 4/4] drm/v3d: Add support for submitting jobs to the TFU Eric Anholt
2018-11-13 22:03   ` Eric Anholt
2018-11-28 19:45     ` Dave Emett [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAApk1dWrj3gvvOXAaU1e17Ec1+GpCccCb5pKwyrGUomwSQo5qg@mail.gmail.com \
    --to=david.emett@broadcom.com \
    --cc=boris.brezillon@bootlin.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=eric@anholt.net \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 4/4] drm/v3d: Add support for submitting jobs to the TFU.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).