On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@xxxxxxxxx> 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?
Okay, I've moved this into the tegra.h header file.
>
> > +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?
Indeed. pushbuf was also unused to I removed it as well.
> 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?
They are currently freed by drm_tegra_job_free(), so yes, the current
intended usage is to create a new job for each submit. Do you mind if we
keep your proposal for a drm_tegra_job_reset() in mind for a follow-up
patch. It's an optimization that we can easily add later on and I'd like
to avoid too much premature optimization at this point.
> > 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?
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.
> > 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).
> 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?
I guess we could make drm_tegra_pushbuf_new() reinitialize the current
pushbuf object and always return the same. But perhaps there are
occasions where it's useful to keep a few of them around?
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.
> > +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?
Interesting. Have you ever tested whether that works properly that way?
I've never seen that in a command stream from the binary driver.
> 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.
> > +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.
Thanks for the review, and apologies for being sluggish.
Thierry
Attachment:
pgp_iF43HGdOn.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
