[RFC PATCH] drm/panfrost: Add initial panfrost driver

[Alyssa Rosenzweig]

Oh my onions, it's really here! It's really coming! It's really here!

----

> +	  DRM driver for ARM Mali Midgard (t6xx, t7xx, t8xx) and
> +	  Bifrost (G3x, G5x, G7x) GPUs.

Nitpick: the model names should maybe be capitalized? Or at least, the
T/G should be consistent? I'm not sure what the vendor marketing names
are exactly.

> +	unsigned long base_hw_features[64 / BITS_PER_LONG];
> +	unsigned long hw_issues[64 / BITS_PER_LONG];

This is confusing...? Is the idea to always have u64, regardless of
32/64-bitness? If so, why not just u64? I'm not totally sure why these
aren't just bitmasks, if we are capping to 64 for each. On the other
hand, there are a lot more than issues exposed by the kbase, though the
vast majority don't apply to kernel space (and should be sorted as a
purely userspace affair)..?

Also, nitpick: s/base_hw_features/hw_features/g, for consistency and not
inheriting naming cruft.

> +	struct panfrost_job *jobs[3];

3 is a bit of a magic number, it feels like. I'm guessing this
corresponds to job slots JS0/JS1/JS2? If so, I guess just add a quick
comment about that, since otherwise it feels a little random.

(Maybe I'm biased because `struct panfrost_job` means something totally
different in userspace for me...)

> +/* DRM_AUTH is required on SUBMIT for now, while all clients share a single
> + * address space.  Note that render nodes would be able to submit jobs that
> + * could access BOs from clients authenticated with the master node.
> + */

This concerns me. Per-process address spaces (configured natively in the
MMU) is essential from both security and stability standpoints. (It's
possible I'm misunderstanding what DRM_AUTH means in this context; this
is more responding to "share a single address space").

> +	drm_mm_init(&pfdev->mm, 0, SZ_4G); // 4G enough for now. can be 48-bit

What's the rationale for picking 4G (when the virtual address space is
64-bit, physical is 48-bit)? Easier portability to 32-bit for
simplicity, or something else?

> +static const struct of_device_id dt_match[] = {
> +	{ .compatible = "arm,mali-t760" },
> +	{ .compatible = "arm,mali-t860" },
> +	{}
> +};

Do we want to add compatibles for the rest of the Mali's on the initial
merge, or wait until they're actually confirmed working so we don't load
and cause problems on untested hardware?

> +enum base_hw_feature {
	...
> +	HW_FEATURE_PROTECTED_MODE
	...
> +};

1) I know these names are inherited from kbase, but might we prefer
panfrost-prefixed names for consistency?

2) I recall discussing this a bit over IRC, but most of these properties
are of more use to userspace than kernelspace. Does it make sense to
keep the feature list here rather than just in Mesa, bearing in mind
Mesa upgrades are easier than kernel upgrades? (I think you may have
been the one to bring up this fact, but hoping it doesn't get lost over
IRC).

3) On a matter of principle, I don't like wasting a bit on
Digital Rainbow Management (especially when it's not something we're
realistically going to implement for numerous reasons...)

> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -0,0 +1,464 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2018 Marty E. Plummer <hanetzer at startmail.com> */
> +/* Copyright 2019 Linaro, Ltd., Rob Herring <robh at kernel.org> */
> +/* Copyright 2019 Collabora ltd. */

Given the register definitions are here (including the comments from
kbase -- if we strip those, it might be a different story), it might be
safer to add a vendor copyright here too.

Alternatively, maybe the registers should be in their own file anyway. I
know they were explicitly moved inward earlier, but conceptually I don't
see why that's preferable to a centralized panfrost_regs.h file?
Copyright/origin is more transparent that way too.

> +	for (timeout = 500; timeout > 0; timeout--) {

500 seems a little arbitrary...?

> +	// Need more version detection
> +	if (pfdev->features.id == 0x0860) {

Seconded, you have the GPU_MODELs just below...?

> +		for (i = 0; i < MAX_HW_REVS; i++) {
> +			if ((model->revs[i].revision != rev) &&
> +			    (model->revs[i].revision != (rev & ~0xf)))
> +				continue;
> +			hw_issues |= model->revs[i].issues;
> +			break;
> +		}

Nitpick: The control flow logic seems a little overcomplicated here.

> +	msleep(10);

What kind of race condition are we working around here? ;)

> +void panfrost_gpu_fini(struct panfrost_device *pfdev)
> +{
> +
> +}

Anything that has to happen here? If no, add a comment in the body
saying that. If yes, well, that (or at least /* stub */)...

> +/*
> + * This is not a complete list of issues, but only the ones the driver needs
> + * to care about.
> + */
> +enum base_hw_issue {
> +	HW_ISSUE_6367,

Similar nitpicks as with the feature list. I will say, this is
_incredibly opaque_. I realize the vendor driver is _intentionally_
opaque here, but that doesn't make this any easier to follow ;)

The good news is that older vendor driver releases (for T760 and
earlier?) had comments documenting what all the errata were, so the vast
majority of these we do have documentation on. Plus, if these are just
the issues the _kernel_ driver cares about, we have documentation for
that given the has_issue calls across kbase. Regardless, something more
transparent than an uncommented number might be nice.

> +	GPUCORE_1619,

What's a GPUCORE and what was kbase thinking...? ;)

> +#endif /* _HWCONFIG_ISSUES_H_ */

s/_HWCONFIG_ISSUES_H/__PANFROST_ISSUES_H_/

> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2019 Linaro, Ltd, Rob Herring <robh at kernel.org> */
> +/* Copyright 2019 Collabora ltd. */

See register concerns again.

> +static int panfrost_job_get_slot(struct panfrost_job *job)

It might help to have a quick comment explaining what JS0/JS1/JS2 are to
refresh the reader's (and eventually your) memory. Just a simple, you
know:

/* JS0: fragment jobs.
 * JS1: vertex/tiler jobs
 * JS2: compute jobs
 */

> +#if 0
> +// Ignore compute for now

Maybe don't ignore compute if it's just this little routine? :)

> +		if (kbase_hw_has_issue(kbdev, BASE_HW_ISSUE_8987))
> +			return 2;

It looks like 8987 only applies to the very earliest dev models (based
on the issues list) so we shouldn't need to worry about this.
t600_r0p0_15dev0 can probably be safely ignored entirely (and deleted
from the issues/features/models list, frankly). I doubt that model is
even publicly available...

> +static void panfrost_job_write_affinity(struct panfrost_device *pfdev,

What's affinity? :)

> +		udelay(100);

(Arbitrary?)

> +#if 0
> +	if (kbase_hw_has_issue(kbdev, BASE_HW_ISSUE_10649))
> +		cfg |= JS_CONFIG_START_MMU;

This issue seems to apply to a lot of GPUs we *do* care about; better
handle this.

> +
> +	if (panfrost_has_hw_feature(kbdev,
> +				BASE_HW_FEATURE_JOBCHAIN_DISAMBIGUATION)) {
> +		if (!kbdev->hwaccess.backend.slot_rb[js].job_chain_flag) {
> +			cfg |= JS_CONFIG_JOB_CHAIN_FLAG;
> +			katom->atom_flags |= KBASE_KATOM_FLAGS_JOBCHAIN;
> +			kbdev->hwaccess.backend.slot_rb[js].job_chain_flag =
> +								true;
> +		} else {
> +			katom->atom_flags &= ~KBASE_KATOM_FLAGS_JOBCHAIN;
> +			kbdev->hwaccess.backend.slot_rb[js].job_chain_flag =
> +								false;
> +		}
> +	}

What does this code do / why is it if 0'd?

> +	for (i = 0; i < bo_count; i++)
> +		/* XXX: Use shared fences for read-only objects. */
> +		reservation_object_add_excl_fence(bos[i]->resv, fence);

I might be paranoid, but 2 lines means braces :)

> +	for (i = 0; i < job->bo_count; i++)
> +		drm_gem_object_put_unlocked(job->bos[i]);
> +	kvfree(job->bos);
> +
> +	kfree(job);

Nitpick: move the blank space up a line.

> +	if (job_read(pfdev, JS_STATUS(js)) == 8) {

What does 8 mean?

> +//		dev_err(pfdev->dev, "reseting gpu");
> +//		panfrost_gpu_reset(pfdev);
> +	}
> +
> +	/* For now, just say we're done. No reset and retry. */
> +//	job_write(pfdev, JS_COMMAND(js), JS_COMMAND_HARD_STOP);
> +	dma_fence_signal(job->done_fence);

That's probably reasonable, at least for now. If our job faults we have
bigger issues / retrying is probably futile. That said, if we're not
resetting is there a risk of lock-ups?

> +		/* Non-Fault Status code */
> +		/* Job exceptions */

I think the "FAULT" suffix implies that loudly enough :)

> +	job_write(pfdev, JOB_INT_CLEAR, 0x70007);
> +	job_write(pfdev, JOB_INT_MASK, 0x70007);

Meaning of the magic numbers...?

> +#define NUM_JOB_SLOTS	2	/* Don't need 3rd one until we have compute support */

Sure, but there _are_ 3 slots in the hardware; there's no need to lie
about that even if we don't presently schedule anything there?

> +// SPDX-License-Identifier:	GPL-2.0
> +/* Copyright 2019 Linaro, Ltd, Rob Herring <robh at kernel.org> */

(Likewise, register copyright).

> +//	if (kbdev->system_coherency == COHERENCY_ACE)
> +//		current_setup->transtab |= AS_TRANSTAB_LPAE_SHARE_OUTER;

Bwap?

> +	//struct panfrost_device *pfdev = cookie;
> +	// Wait 1000 GPU cycles!?

?!
> +		if (panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU))
> +			return "ATOMIC";
> +		else
> +			return "UNKNOWN";

Does it really make sense to check for the feature to determine the
name...? I mean, that code path should be unreachable, but still (at
least without the check the code is slightly neater..)

> +		fault_status = mmu_read(pfdev, AS_FAULTSTATUS(i));
> +		addr = mmu_read(pfdev, AS_FAULTADDRESS_LO(i));
> +		addr |= (u64)mmu_read(pfdev, AS_FAULTADDRESS_HI(i)) << 32;

I don't know if it's necessary for the initial merge, but maybe at least
put a TODO comment in here that growable memory (lazy allocations) will
be implemented here in the future...?

> +void panfrost_mmu_fini(struct panfrost_device *pfdev)
> +{
> +
> +}

Empty?

----------------------------

Overall, I'm super happy to see this! Nice work, guys! ^_^

-Alyssa