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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6851 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel