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 <hanet...@startmail.com> */ > +/* Copyright 2019 Linaro, Ltd., Rob Herring <r...@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 <r...@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 <r...@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel