On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding thierry.reding@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!