On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@xxxxxxxxx> 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!
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
