[PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding at gmail.com> wrote:
> diff --git a/tegra/fence.c b/tegra/fence.c
> new file mode 100644
> index 000000000000..f58725ca8472
> --- /dev/null
> +++ b/tegra/fence.c
> +drm_public
> +int drm_tegra_fence_wait(struct drm_tegra_fence *fence)
> +{
> +       return drm_tegra_fence_wait_timeout(fence, -1);
> +}

Perhaps a convenience-wrapper like this should be inline in the header
to reduce function-call overhead?

> +drm_public
> +int drm_tegra_job_submit(struct drm_tegra_job *job,
> +                        struct drm_tegra_fence **fencep)
> +{
> +       struct drm_tegra *drm = job->channel->drm;
> +       struct drm_tegra_pushbuf_private *pushbuf;
> +       struct drm_tegra_fence *fence = NULL;
> +       struct drm_tegra_cmdbuf *cmdbufs;
> +       struct drm_tegra_syncpt *syncpts;
> +       struct drm_tegra_submit args;
> +       unsigned int i;
> +       int err;
> +
> +       /*
> +        * Make sure the current command stream buffer is queued for
> +        * submission.
> +        */
> +       err = drm_tegra_pushbuf_queue(job->pushbuf);
> +       if (err < 0)
> +               return err;
> +
> +       job->pushbuf = NULL;
> +
> +       if (fencep) {
> +               fence = calloc(1, sizeof(*fence));
> +               if (!fence)
> +                       return -ENOMEM;
> +       }
> +
> +       syncpts = calloc(1, sizeof(*syncpts));
> +       if (!syncpts) {
> +               free(cmdbufs);

cmdbufs never gets assigned to, yet it gets free'd here (and a few
more places further down). I guess this is left-overs from the
previous round that should just die?

But this raises the question, how is job->cmdbufs (and job->relocs)
supposed to get free'd?

I'm a bit curious as to what the expected life-time of these objects
are. Do I need to create a new job-object for each submit, or can I
reuse a job? I think the latter would be preferable... So perhaps we
should have a drm_tegra_job_reset that allows us to throw away the
accumulated cmdbufs and relocs, and start building a new job?

> diff --git a/tegra/private.h b/tegra/private.h
> index 9b6bc9395d23..fc74fb56b58d 100644
> --- a/tegra/private.h
> +++ b/tegra/private.h
> @@ -26,13 +26,31 @@
>  #define __DRM_TEGRA_PRIVATE_H__ 1
>
>  #include <stdbool.h>
> +#include <stddef.h>
>  #include <stdint.h>
>
>  #include <libdrm.h>
> +#include <libdrm_lists.h>
>  #include <xf86atomic.h>
>
> +#include "tegra_drm.h"
>  #include "tegra.h"
>
> +#define container_of(ptr, type, member) ({                             \
> +               const typeof(((type *)0)->member) *__mptr = (ptr);      \
> +               (type *)((char *)__mptr - offsetof(type, member));      \
> +       })
> +
<snip>
> +
> +struct drm_tegra_pushbuf_private {
> +       struct drm_tegra_pushbuf base;
> +       struct drm_tegra_job *job;
> +       drmMMListHead list;
> +       drmMMListHead bos;
> +
> +       struct drm_tegra_bo *bo;
> +       uint32_t *start;
> +       uint32_t *end;
> +};
> +
> +static inline struct drm_tegra_pushbuf_private *
> +pushbuf_priv(struct drm_tegra_pushbuf *pb)
> +{
> +       return container_of(pb, struct drm_tegra_pushbuf_private, base);
> +}
> +

This seems to be the only use-case for container_of, and it just
happens to return the exact same pointer as was passed in... so do we
really need that helper?

> diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
> new file mode 100644
> index 000000000000..178d5cd57541
> --- /dev/null
> +++ b/tegra/pushbuf.c
> @@ -0,0 +1,205 @@
<snip>
> +drm_public
> +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
> +                         struct drm_tegra_job *job)
> +{
> +       struct drm_tegra_pushbuf_private *pushbuf;
> +       void *ptr;
> +       int err;
> +
> +       pushbuf = calloc(1, sizeof(*pushbuf));
> +       if (!pushbuf)
> +               return -ENOMEM;
> +
> +       DRMINITLISTHEAD(&pushbuf->list);
> +       DRMINITLISTHEAD(&pushbuf->bos);
> +       pushbuf->job = job;
> +
> +       *pushbufp = &pushbuf->base;
> +
> +       DRMLISTADD(&pushbuf->list, &job->pushbufs);

Hmm. So the job keeps a list of pushbufs. What purpose does this
serve? Shouldn't the job only need the cmdbuf-list (which it already
has) and the BOs (so they can be dereferenced after being submitted)?

AFAICT, we could make drm_tegra_pushbuf_queue append the BO to a list
in the job instead. That way we don't need to keep all the
pushbuf-objects around, and we might even be able to reuse the same
object rather than keep reallocating them. No?

> +drm_public
> +int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
> +                          enum drm_tegra_syncpt_cond cond)
> +{
> +       struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
> +
> +       if (cond >= DRM_TEGRA_SYNCPT_COND_MAX)
> +               return -EINVAL;
> +
> +       *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x0, 0x1);
> +       *pushbuf->ptr++ = cond << 8 | priv->job->syncpt;

Perhaps "HOST1X_OPCODE_IMM(0x0, cond << 8 | priv->job->syncpt)", which
saves a word?

Either way, I think this should either call drm_tegra_pushbuf_prepare
to make sure two words are available, or be renamed to reflect that it
causes a push (or two, as in the current form).

> +struct drm_tegra_channel;
> +struct drm_tegra_job;
> +
> +struct drm_tegra_pushbuf {
> +       uint32_t *ptr;
> +};

Hmm, single-member structs always makes me cringe a bit. But in this
case it's much better than a double-pointer IMO.

But perhaps some of the members in the private-struct might be useful
out here? For instance, if "uint32_t *end;" was also available, we
could do:

inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
*pushbuf, uint32_t value)
{
       assert(pushbuf->ptr < pushbuf->end);
       *pushbuf->ptr++ = value;
}

...and easily pick up bugs with too few words? The helper would of
course be in the header-file, so the write gets inlined nicely.

But all in all, a really nice read. Thanks!