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:
[...]
+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?
It has the benefit of keeping this code working even when somebody suddenly adds to the beginning of drm_tegra_pushbuf_private. I'd rather not resolve to force casting just to be on the safe side.
Is that a really a legitimate worry? There's a very clear relationship between the two structures. Both nouveau and freedreno does the same thing (for nouveau_bo_priv, nouveau_device_priv, msm_device, msm_pipe and msm_bo structures), and it seems to work fine for them. Perhaps a comment along the lines of "/* this needs to be the first member */" is enough to discourage people from messing with it?
Oh well, if everybody else is doing it and you keep insisting I'll just drop it. I do like explicit upcasting, but I guess doing it implicitly will work as well. Worst case this will crash on people if they don't know what they're doing.
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.
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.
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.
+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.
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. 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.
Thierry