On Fri, May 2, 2014 at 5:16 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Fri, May 02, 2014 at 04:53:55PM +0200, Erik Faye-Lund wrote:
On Fri, May 2, 2014 at 4:06 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
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)?
This is currently used to keep track of existing pushbuffers and make sure that they are freed (when the job is freed).
OK, it seems to me that the need for keeping the pushbuffers around is simply not there. Only the BOs needs to be kept around, no?
Right. But unless we come up with an alternative API I can't just drop this. Otherwise we'll be leaking pushbuffers all over the place if callers don't clean them up themselves.
OK. But just to get some clarity, if the same level of abstraction calls drm_tegra_pushbuf_new() and drm_tegra_pushbuf_free() when it's done, we'll currently get use-after-free and double-frees, right?
As-is, the client would have to call drm_tegra_pushbuf_new(), transfer the ownership of that object to the job object by calling drm_tegra_pushbuf_prepare(), and clean it up by calling drm_tegra_job_submit()? Or am I missing something?
If reusing the pushbuf objects makes sense, then perhaps a different API would be more useful. Rather than allocate in the context of a job, they could be allocated in a channel-wide context and added to/associated with a job only as needed.
Yes, I think this might make sense. I'll have to ponder a bit more about this, though.
What I've been thinking is something rougly like this:
int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbuf, struct drm_tegra_channel *channel); int drm_tegra_pushbuf_free(struct drm_tegra_pushbuf *pushbuf); int drm_tegra_pushbuf_reset(struct drm_tegra_pushbuf *pushbuf);
int drm_tegra_job_queue(struct drm_tegra_job *job, struct drm_tegra_pushbuf *pushbuf);
Then we could either reset the pushbuf in drm_tegra_job_queue() (in which case drm_tegra_pushbuf_reset() could be private) or leave it up to the caller to manually reset when they need to reuse the pushbuffer.
OK, this looks better to me. I'll still have to wrap my head around these things a bit better to tell for sure, though ;)
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).
I've added a call to drm_tegra_pushbuf_prepare() here and in drm_tegra_pushbuf_relocate() which also pushes one word to the pushbuf.
Actually, I think drm_tegra_pushbuf_relocate should just be renamed to drm_tegra_pushbuf_push_reloc(), as that conveys quite clearly that it is just a push-helper and need to be counted when preparing. That way it doesn't need to prepare itself.
I don't think we necessarily need to rename the function to make it obvious. This is completely new API anyway, so we can add comments to explain that drm_tegra_pushbuf_relocate() will push a placeholder word with a dummy address that will be used to relocate a BO and needs to be included when preparing a pushbuffer.
True.
I have a "secret plan" here, which I guess I should just be open about:
I wonder if it would make sense in the long run to add multiple push-variants, to save client code from repeating the same logic, similar to the drm_tegra_pushbuf_push_word I mentioned above:
inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf *pushbuf, uint32_t value) { assert(pushbuf->ptr < pushbuf->end); *pushbuf->ptr++ = value; }
inline void drm_tegra_pushbuf_push_float(struct drm_tegra_pushbuf *pushbuf, float value) { union { float f; uint32_t i; } u; u.f = value; drm_tegra_pushbuf_push_word(pushbuf, u.i); }
And probably a similar version for FP20. My thinking was that a reloc belongs in that same category, and probably should have a similar interface.
...But thinking about this a bit more, I think libdrm is the wrong place to add these things. Most registers are packed bits of sorts, so I guess the client code *needs* to wrap up loads of similar things anyway. And besides, DDX doesn't care about many of these, so perhaps Mesa is a better suited place for these "higher-level pushes".
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.
Sounds good. If you have no strong objections, I'd like to keep that for a follow on patch when there's more code that actively uses this API. It should be fairly safe to make more members public via drm_tegra_pushbuf rather than the other way around, so I'd like to err on the side of carefulness for a bit longer.
Hmm, isn't it the *current* code that is "careless" in this case? The client doesn't have enough context to validate this itself (without having access to the end-pointer)?
What I meant was being careful about how much we expose. Whatever we expose now we can potentially never hide again because code may depend on it being public.
Fair enough. We're still hidden behind an experimental api-switch ATM. But yeah, we should be careful about what we expose.
I don't think it's particularly careless to require the caller to prepare a buffer. If they then write past its end, that's their issue. That's like malloc()ing a buffer and then filling it with more bytes than you've allocated. That's not the level of safety that this API needs in my opinion.
True, but we should also try to be helpful in finding out what went wrong. And to follow up on your malloc-example, most sane CRTs have some heap-corruption detection mechanisms to aid debugging. I don't think this suggestion in particular is bending over backwards, though.