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

[Erik Faye-Lund]

On Fri, May 2, 2014 at 5:16 PM, Thierry Reding <thierry.reding at 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 at 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.