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

[Rob Herring]

On Thu, Mar 7, 2019 at 11:00 PM Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
>
> 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.

bitmasks in the kernel use unsigned long arrays. A strange choice
which I guess was either because it predated 64-bit or enables atomic
ops which tend to be on the native size. So this just fixes the size
to 64-bits for 32 and 64 bit systems.

> 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)..?

Issues I trimmed down to only the kernel ones. Features were small
enough, I just left them all.

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

+1

> > +     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.

Job slots, yes. I think I have a define somewhere already I'll use.

> (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").

We'll get there. I'll just point out that freedreno is also a single
address space (though there are patches for that now).

> > +     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?

Systems simply don't have enough RAM that you'd want to use 4G for
graphics memory.

That reminds me, I want to not start at 0 to catch NULL addresses.

> > +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?

I'd say wait. I also suggested on the bifrost bindings that we do a
more generic fallback as the h/w is pretty much discoverable for
model, version and features.

> > +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?

They aren't exposed outside of the driver, so namespacing them is a
bit pointless and just makes the names too long. I will at least
rename base_hw_feature.

> 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).

Unlike the issues list, it was small enough I didn't really think about it here.

> 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...)

If this is ever going to be used in commercial products, it will need
to be supported whether in tree or out. We can leave that for another
day.

>
> > +++ 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?

Primarily to enforce good hygiene to only access a sub-block's
registers within its .c file.

> Copyright/origin is more transparent that way too.

Good point. Otherwise, I can say "Register definitions from ...,
Copyright ARM..."

>
> > +     for (timeout = 500; timeout > 0; timeout--) {
>
> 500 seems a little arbitrary...?

We should should have a udelay in here at least to get a known time
rather than how much the compiler optimizes. Though the timeout would
still be arbitrary.

>
> > +     // Need more version detection
> > +     if (pfdev->features.id == 0x0860) {
>
> Seconded, you have the GPU_MODELs just below...?

This means I just stuffed in fixed values to these registers. The
kbase driver uses hw_issues and such to determine the setup.

>
> > +             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? ;)

Hard to say with the context deleted...

As powering on h/w takes time, a delay seemed appropriate. i think I
can check some status bits though.

>
> > +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.

It's not always clear looking at the kbase driver. Often it's just if
some issue, set or don't set some other bit.

IMO, if someone wants to improve the documentation here, that can come later.

>
> > +     GPUCORE_1619,
>
> What's a GPUCORE and what was kbase thinking...? ;)

No idea other than it is only for T604 and s/w models.

>
> > +#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...

Sadly, I confirmed it is sitting on my desk. Samsung chromebook
(snow). As to whether anyone still cares about snow and wants to run a
current mainline kernel on it, I don't know. It doesn't look to me
like there's any effort there. I certainly don't plan to try.

> > +static void panfrost_job_write_affinity(struct panfrost_device *pfdev,
>
> What's affinity? :)

Hard coded ATM.

> > +             udelay(100);
>
> (Arbitrary?)

Tell me what you'd like it to be...

>
> > +#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?

Isn't it obvious?

It's a TODO we need to figure out.

>
> > +     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?

Maybe? Reloading the module will reset things.

>
> > +             /* 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?

Only matters for bifrost. There for a TODO I guess.

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

That's what the kbase driver does...

> > +             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...?

That affects more than just the code here. We probably need a higher
level TODO list.

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

Yeah, that needs some cleanup.

Rob