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

[Alyssa Rosenzweig]

> +/**
> + * struct drm_panfrost_wait_bo - ioctl argument for waiting for
> + * completion of the last DRM_PANFROST_SUBMIT_CL on a BO.

Nit: Should be plain DRM_PANFROST_SUBMIT, there is no CL for us.

> +	__s64 timeout_ns;	/* absolute */

Erm, why is this signed? Semantically, what does a negative timestamp
mean? Seems suspect. The comment /* absolute */ seems to underscore that
we really do want an unsigned value, perhaps ascribing a special meaning
to 0/~0 for "nonblocking" and "block indefinitely" if needed. Of course,
"(2^64)-1 ns" is essentially indefinite, so the latter need not be a
special case.

* It's 585 years, according to a back of the envelope calculation.
  Panfrost will be obsolete many times over by the time that timeout
  elapses ;)

> +	struct drm_panfrost_mmap_bo mmap_bo = {0,};

Nit: "{0,} is confusing.

General nit: Spacing is all over the place in pan_drm.h. I'm not one to
particularly care (I think I use 8-space indents...), but it's
inconsistent from line to line which is a little distracting. If you use
vim, I have set:

	au BufNewFile,BufRead */mesa/* set expandtab tabstop=8 softtabstop=3 shiftwidth=3
	au BufNewFile,BufRead */panfrost/* set expandtab tabstop=8 shiftwidth=8 softtabstop=8

which generally does the right thing. Translating to $EDITOR left as an
exercise to the reader.

---

Overall, this looks really great. I haven't gotten the chance to test
this personally yet, but I think I can nevertheless push at least the
other 7 patches tonight :)

Keep up the awesome work!

Alyssa
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190308/6e7f04df/attachment.sig>