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

[Rob Herring]

On Fri, Mar 8, 2019 at 9:34 AM Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
>
> > 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.
>
> Bizarre, but if that's the standard, then OK.
>
> > Issues I trimmed down to only the kernel ones. Features were small
> > enough, I just left them all.
>
> +1
>
> > Job slots, yes. I think I have a define somewhere already I'll use.
>
> NUM_JOB_SLOTS
>
> > I'll just point out that freedreno is also a single address space
> > (though there are patches for that now).
>
> Huh. That seems.... huh.
>
> > Systems simply don't have enough RAM that you'd want to use 4G for
> > graphics memory.
>
> Fair enough.
>
> > That reminds me, I want to not start at 0 to catch NULL addresses.
>
> +1
>
> > 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.
>
> +1
>
> > Unlike the issues list, it was small enough I didn't really think about it here.
>
> +1 though this may change in the future.
>
> > 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.
>
> *angry acceptance noises*
>
> > 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..."
>
> Perhaps it would make sense to have a regs/ folder with a
> .h corresponding to each of the components. So it's separated out but
> still prevents unintended access (since you'd need to #include the wrong
> component explicitly, and that'd be immediately vetoed on a patch).
>
> > This means I just stuffed in fixed values to these registers. The
> > kbase driver uses hw_issues and such to determine the setup.
>
> Oh, I see.
>
> > Hard to say with the context deleted...
>
> Sorry! I'm new to this, bear with me :)
>
> > As powering on h/w takes time, a delay seemed appropriate. i think I
> > can check some status bits though.
>
> That conceptually seems better, I think.
>
> > IMO, if someone wants to improve the documentation here, that can come later.
>
> I'd be happy to submit a patch helping with this (either before we
> upstream or after).
>
> > Sadly, I confirmed it is sitting on my desk. Samsung chromebook
> > (snow).
>
> Any chance that was an earlier dev version of Snow that you got from
> your employer and this was maybe fixed in the real thing? Or would that
> be too convenient?

It was given to me and a bunch of other ARM kernel devs, but I think
it was in production by then. It's an A01 rev which matches this:

https://www.notebookcheck.net/Samsung-Chromebook-XE303C12-A01US.84022.0.html

The only other rev is a UK version.

>
> > Hard coded ATM.
>
> No, like, what does "affinity" mean in this context? I didn't study
> kbase too hard so I'm having troubles following any of this.

Core affinity. It's which shader cores to use which was based on flags
you pass in for the job. Some of the h/w has multiple L2 caches (IIRC
just bifrost) and the code is fairly hard to follow which is why it's
just hardcoded. Sorry, I don't have a better explanation, but I've
already forgotten some of the details and stopped looking at it once I
found hardcoding it would work for now...

This and other parts of this code is why I asked if there are other
features of the kbase job submit that we may need in the future
(besides just compute).

> > Tell me what you'd like it to be...
>
> Maybe some nice round number like 64 or 128..? Kidding ;)
>
> > Isn't it obvious?
> >
> > It's a TODO we need to figure out.
>
> ...Ah. Yes, that should have been obvious. My apologies.
>
> > Maybe? Reloading the module will reset things.
>
> The issue is that unprivileged userspace apps are able to submit
> arbitrarily crazy workloads to the kernel (without necessarily needing
> the driver's permission). It's trivial for unprivileged code to cause a
> fault (and thanks to driver bugs, WebGL is an attack surface here as
> well); doing so should not lock up the GPU requiring superuser access to
> correct. Faults are not catastrophic but they do happen.

I understand, but wouldn't just running conformance tests likely kill
things too? I'm guessing we can't even run a web browser yet, so WebGL
problem is solved. ;)

In any case, Tomeu said resetting is next up for him.

Rob