[Mesa-dev] [PATCH 8/8] panfrost: Add backend targeting the DRM driver

[Rob Herring]

On Mon, Mar 4, 2019 at 6:43 PM Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
>
> Wooo!!!!!!!
>
> Seeing this patch in my inbox has been some of the best news all day!
>
> Without further ado, my (solicited?) comments:
>
> > +/* Copyright 2018, Linaro, Ltd., Rob Herring <robh at kernel.org> */
>
> Please triple check upstream that this license is okay; the other files
> in include/drm-uapi are BSDish.

There's a mixture of MIT and 'GPL-2.0 WITH Linux-syscall-note'. These
are the ones with GPL:

include/uapi/drm/armada_drm.h:/* SPDX-License-Identifier: GPL-2.0 WITH
Linux-syscall-note */
include/uapi/drm/etnaviv_drm.h:/* SPDX-License-Identifier: GPL-2.0
WITH Linux-syscall-note */
include/uapi/drm/exynos_drm.h:/* SPDX-License-Identifier: GPL-2.0+
WITH Linux-syscall-note */
include/uapi/drm/i810_drm.h:/* SPDX-License-Identifier: GPL-2.0 WITH
Linux-syscall-note */
include/uapi/drm/omap_drm.h:/* SPDX-License-Identifier: GPL-2.0 WITH
Linux-syscall-note */

I don't think it matters, but imagine all corporate entities would prefer MIT.

>
> > +/* timeouts are specified in clock-monotonic absolute times (to simplify
> > + * restarting interrupted ioctls).  The following struct is logically the
> > + * same as 'struct timespec' but 32/64b ABI safe.
> > + */
> > +struct drm_panfrost_timespec {
> > +     __s64 tv_sec;          /* seconds */
> > +     __s64 tv_nsec;         /* nanoseconds */
> > +};
>
> What's this for -- is there not a shared struct for this if it's
> necessary? (I'm assuming this was copied from etna..?)

I couldn't find any common struct.

>
> > +     __u32 flags;          /* in, mask of ETNA_BO_x */
>
> As Rob said, s/ETNA/PAN/g
>
> > +     struct drm_panfrost_timespec timeout;   /* in */
>
> (Presumably we just want a timeout in one of nanoseconds / milliseconds
> / second, not the full timespec... 64-bit time gives a pretty wide range
> even for high-precision timeouts, which I don't know that we need. Also,
> it's signed for some reason...?)

Yes, but I was keeping it aligned with etnaviv. Changing to just u64
ns should be fine as I think rollover in 500 years is acceptable.

Arnd confirmed with me that a plain u64 nanoseconds is preferred.

> > +     struct drm_panfrost_gem_submit_atom_dep deps[2];
>
> I'm concerned about hardcoding deps to 2 here. I know the Arm driver
> does it, but I'm super uncomfortable by them doing it too. Keep in mind,
> when they have complex dependencies they insert dummy "dependency-only"
> jobs into the graph, though I concede I haven't seen this yet.
>
> I'm not at all sure what the right number is, especially since the new
> kernel interface seems to support waiting on BOs? Or am I
> misinterpreting this?
>
> Regardless, this will start to matter when we implement support for more
> complex FBOs (where various FBOs samples from various other FBOs), which
> I think can have arbitrarily complicated dependency graphs. So
> hardcoding to [2] is inappropriate if we want to use deps for waiting on
> FBOs. On the other hand, if we use a kernel-side BO wait or fences or
> something to resolve dependencies, do we even need two deps, or just
> one?
>
> > +     // TODO cache allocations
> > +     // TODO properly handle errors
> > +     // TODO take into account extra_flags
>
> Not a blocker at all for merge, but these should be taken care of before
> we drop support for the non-DRM stuff (since at least the
> lack of GROWABLE support represents a regression compared to the Arm
> driver).

Need to research how other drivers deal with this. Presumably other
drivers should need some heap as well? I found one case that has a
specific ioctl call to do allocate it (don't remember which one
offhand).

>
> > +     // TODO map and unmap on demand?
>
> I don't know if this matters too much...? Usually if we're allocating a
> slab, that means we want it mapped immediately, unless we set the
> INVISIBLE flag which means we odn't want it mapped at all.

Map to what? The GPU or CPU?

> Maybe we'll have fancier scenarios in the future, but at the moment I
> think we can safely cross off at least the first half of the TODO.
>
> As you know I'm not a believer in unmapping/freeing resources ever, so I
> don't get an opinion there ;)
>
> > +     gem_info.handle = gem_new.handle;
> > +     ret = drmIoctl(drm->fd, DRM_IOCTL_PANFROST_GEM_INFO, &gem_info);
> > +     if (ret) {
> > +                fprintf(stderr, "DRM_IOCTL_PANFROST_GEM_INFO failed: %d\n", ret);
> > +             assert(0);
> > +     }
>
> Maybe a question for Rob, but why isn't this info passed along with the
> allocate in the first place? I guess the extra roundtrip to the kernel
> isn't a huge deal, but it seems a little odd...?

It is. GEM_INFO should only be needed when you import a dmabuf.

>
> > +static uint8_t
> > +allocate_atom()
> > +{
> > +     /* Use to allocate atom numbers for jobs. We probably want to overhaul this in kernel space at some point. */
> > +     static uint8_t atom_counter = 0;
> > +
> > +        atom_counter++;
> > +
> > +        /* Workaround quirk where atoms must be strictly positive */
> > +
> > +        if (atom_counter == 0)
> > +                atom_counter++;
> > +
> > +        return atom_counter;
> > +}
>
> So, this routine (which is copied straight from the non-DRM code) is
> specifically to workaround a quirk in the Arm driver where atom numbers
> must be non-zero u8's. I'm skeptical the DRM interface needs this.

And why u8? A sequence number is pretty common and we should just copy
what other drivers do rather than think about it.

Rob