Messages in this thread | | | Date | Tue, 10 Aug 2021 00:02:10 +0530 | From | Sai Prakash Ranjan <> | Subject | Re: [Freedreno] [PATCH 0/3] iommu/drm/msm: Allow non-coherent masters to use system cache |
| |
On 2021-08-10 00:00, Rob Clark wrote: > On Mon, Aug 9, 2021 at 11:11 AM Sai Prakash Ranjan > <saiprakash.ranjan@codeaurora.org> wrote: >> >> On 2021-08-09 23:37, Rob Clark wrote: >> > On Mon, Aug 9, 2021 at 10:47 AM Sai Prakash Ranjan >> > <saiprakash.ranjan@codeaurora.org> wrote: >> >> >> >> On 2021-08-09 23:10, Will Deacon wrote: >> >> > On Mon, Aug 09, 2021 at 10:18:21AM -0700, Rob Clark wrote: >> >> >> On Mon, Aug 9, 2021 at 10:05 AM Will Deacon <will@kernel.org> wrote: >> >> >> > >> >> >> > On Mon, Aug 09, 2021 at 09:57:08AM -0700, Rob Clark wrote: >> >> >> > > On Mon, Aug 9, 2021 at 7:56 AM Will Deacon <will@kernel.org> wrote: >> >> >> > > > On Mon, Aug 02, 2021 at 06:36:04PM -0700, Rob Clark wrote: >> >> >> > > > > On Mon, Aug 2, 2021 at 8:14 AM Will Deacon <will@kernel.org> wrote: >> >> >> > > > > > On Mon, Aug 02, 2021 at 08:08:07AM -0700, Rob Clark wrote: >> >> >> > > > > > > On Mon, Aug 2, 2021 at 3:55 AM Will Deacon <will@kernel.org> wrote: >> >> >> > > > > > > > On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan wrote: >> >> >> > > > > > > > > On 2021-07-28 19:30, Georgi Djakov wrote: >> >> >> > > > > > > > > > On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote: >> >> >> > > > > > > > > > > commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") >> >> >> > > > > > > > > > > removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went >> >> >> > > > > > > > > > > the memory type setting required for the non-coherent masters to use >> >> >> > > > > > > > > > > system cache. Now that system cache support for GPU is added, we will >> >> >> > > > > > > > > > > need to set the right PTE attribute for GPU buffers to be sys cached. >> >> >> > > > > > > > > > > Without this, the system cache lines are not allocated for GPU. >> >> >> > > > > > > > > > > >> >> >> > > > > > > > > > > So the patches in this series introduces a new prot flag IOMMU_LLC, >> >> >> > > > > > > > > > > renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC >> >> >> > > > > > > > > > > and makes GPU the user of this protection flag. >> >> >> > > > > > > > > > >> >> >> > > > > > > > > > Thank you for the patchset! Are you planning to refresh it, as it does >> >> >> > > > > > > > > > not apply anymore? >> >> >> > > > > > > > > > >> >> >> > > > > > > > > >> >> >> > > > > > > > > I was waiting on Will's reply [1]. If there are no changes needed, then >> >> >> > > > > > > > > I can repost the patch. >> >> >> > > > > > > > >> >> >> > > > > > > > I still think you need to handle the mismatched alias, no? You're adding >> >> >> > > > > > > > a new memory type to the SMMU which doesn't exist on the CPU side. That >> >> >> > > > > > > > can't be right. >> >> >> > > > > > > > >> >> >> > > > > > > >> >> >> > > > > > > Just curious, and maybe this is a dumb question, but what is your >> >> >> > > > > > > concern about mismatched aliases? I mean the cache hierarchy on the >> >> >> > > > > > > GPU device side (anything beyond the LLC) is pretty different and >> >> >> > > > > > > doesn't really care about the smmu pgtable attributes.. >> >> >> > > > > > >> >> >> > > > > > If the CPU accesses a shared buffer with different attributes to those which >> >> >> > > > > > the device is using then you fall into the "mismatched memory attributes" >> >> >> > > > > > part of the Arm architecture. It's reasonably unforgiving (you should go and >> >> >> > > > > > read it) and in some cases can apply to speculative accesses as well, but >> >> >> > > > > > the end result is typically loss of coherency. >> >> >> > > > > >> >> >> > > > > Ok, I might have a few other sections to read first to decipher the >> >> >> > > > > terminology.. >> >> >> > > > > >> >> >> > > > > But my understanding of LLC is that it looks just like system memory >> >> >> > > > > to the CPU and GPU (I think that would make it "the point of >> >> >> > > > > coherence" between the GPU and CPU?) If that is true, shouldn't it be >> >> >> > > > > invisible from the point of view of different CPU mapping options? >> >> >> > > > >> >> >> > > > You could certainly build a system where mismatched attributes don't cause >> >> >> > > > loss of coherence, but as it's not guaranteed by the architecture and the >> >> >> > > > changes proposed here affect APIs which are exposed across SoCs, then I >> >> >> > > > don't think it helps much. >> >> >> > > > >> >> >> > > >> >> >> > > Hmm, the description of the new mapping flag is that it applies only >> >> >> > > to transparent outer level cache: >> >> >> > > >> >> >> > > +/* >> >> >> > > + * Non-coherent masters can use this page protection flag to set cacheable >> >> >> > > + * memory attributes for only a transparent outer level of cache, also known as >> >> >> > > + * the last-level or system cache. >> >> >> > > + */ >> >> >> > > +#define IOMMU_LLC (1 << 6) >> >> >> > > >> >> >> > > But I suppose we could call it instead IOMMU_QCOM_LLC or something >> >> >> > > like that to make it more clear that it is not necessarily something >> >> >> > > that would work with a different outer level cache implementation? >> >> >> > >> >> >> > ... or we could just deal with the problem so that other people can reuse >> >> >> > the code. I haven't really understood the reluctance to solve this properly. >> >> >> > >> >> >> > Am I missing some reason this isn't solvable? >> >> >> >> >> >> Oh, was there another way to solve it (other than foregoing setting >> >> >> INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a >> >> >> corresponding setting on the MMU pgtables side of things? >> >> > >> >> > Right -- we just need to program the CPU's MMU with the matching memory >> >> > attributes! It's a bit more fiddly if you're just using ioremap_wc() >> >> > though, as it's usually the DMA API which handles the attributes under >> >> > the >> >> > hood. >> >> > >> >> > Anyway, sorry, I should've said that explicitly earlier on. We've done >> >> > this >> >> > sort of thing in the Android tree so I assumed Sai knew what needed to >> >> > be >> >> > done and then I didn't think to explain to you :( >> >> > >> >> >> >> Right I was aware of that but even in the android tree there is no >> >> user >> >> :) >> >> I think we can't have a new memory type without any user right in >> >> upstream >> >> like android tree? >> >> >> >> @Rob, I think you already tried adding a new MT and used >> >> pgprot_syscached() >> >> in GPU driver but it was crashing? >> > >> > Correct, but IIRC there were some differences in the code for memory >> > types compared to the android tree.. I couldn't figure out the >> > necessary patches to cherry-pick to get the android patch to apply >> > cleanly, so I tried re-implementing it without having much of a clue >> > about how that code works (which was probably the issue) ;-) >> > >> >> Hehe no, even I get the same crash after porting/modifying the >> required >> patches from android ;) and I think crashes would be seen in android >> as >> well, its just that they don't have any user exercising that code. >> >> Thing is I can't make head and tail of the GPU crash logs, maybe you >> know >> how to decode those errors, if not I can start a thread with QC GPU >> team >> and ask them to decode? >> > > If you have a gpu devcore dump, I can take a look at it with > crashdec.. otherwise I can try to find the branch where I had that > patch backported. > > I'm more familiar with using crashdec to figure out mesa bugs, but > maybe I could spot something where what the GPU is seeing disagrees > with what the CPU expects it to be seeing. >
Sure, I will get a devcoredump tomorrow and attach in the bug, currently I don't have it handy.
Thanks, Sai
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |