<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Am 10.04.25 um 19:20 schrieb Boris Brezillon: <blockquote type="cite" cite="mid:20250410192054.24a592a5@collabora.com">[SNIP] <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""></pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Faulting is only legal when you have something like HMM, SVM or whatever you call it. And then you can just use a plain shmem object to provide you with backing pages. I mean we could in theory allow faulting on GEM objects as well, but we would need to take very strict precautions on that we currently don't have as far as I know. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">We only use this mechanism for very specific allocations: tiler memory whose maximum size can't be guessed upfront because tile binning is by nature unpredictible. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">So could you explain how this works in the first place? </pre> </blockquote> <pre class="moz-quote-pre" wrap="">I can explain you how this works in Panthor, yes. You get an initial amount of memory that the tiler can use, when it runs out of memory, it will first ask the system for more memory, if the allocation fails, it will fallback to what they call "incremental rendering", where the already binned primitives are flushed to the FB in order to free memory, and the rendering starts over from there, with the memory that has been freed. In Panthor, this on-demand allocation scheme is something that allows us to speed-up rendering when there's memory available, but we can make progress when that's not the case, hence the failable allocation scheme I'm proposing here. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Puh, that sounds like it can potentially work but is also very very fragile. You must have a code comment when you select the GFP flags how and why that works. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> + /* We want non-blocking allocations, if we're OOM, we just fail the job + * to unblock things. + */ + ret = drm_gem_shmem_sparse_populate_range(&bo->base, page_offset, + NUM_FAULT_PAGES, page_gfp, + __GFP_NORETRY | __GFP_NOWARN); That's what I have right now in the failable allocation path. The tiler chunk allocation uses the same flags, but doesn't have a comment explaining that a fallback exists when the allocation fails.</pre> </blockquote> We agreed to use GFP_NOWAIT for such types of allocations to at least wake up kswapd on the low water mark. IIRC even using __GFP_NORETRY here was illegal, but I need to double check the discussions again. From the comment this at minimum needs an explanation what influence this has on the submission and that you can still guarantee fence forward progress. <blockquote type="cite" cite="mid:20250410192054.24a592a5@collabora.com"> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">In Panfrost and Lima, we don't have this concept of "incremental rendering", so when we fail the allocation, we just fail the GPU job with an unhandled GPU fault. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> To be honest I think that this is enough to mark those two drivers as broken. It's documented that this approach is a no-go for upstream drivers. How widely is that used? </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> It exists in lima and panfrost, and I wouldn't be surprised if a similar mechanism was used in other drivers for tiler-based GPUs (etnaviv, freedreno, powervr, ...), because ultimately that's how tilers work: the amount of memory needed to store per-tile primitives (and metadata) depends on what the geometry pipeline feeds the tiler with, and that can't be predicted. If you over-provision, that's memory the system won't be able to use while rendering takes place, even though only a small portion might actually be used by the GPU. If your allocation is too small, it will either trigger a GPU fault (for HW not supporting an "incremental rendering" mode) or under-perform (because flushing primitives has a huge cost on tilers). Calling these drivers broken without having a plan to fix them is also not option.</pre> </blockquote> Yeah, agree we need to have some kind of alternative. It's just that at the moment I can't think of any. <blockquote type="cite" cite="mid:20250410192054.24a592a5@collabora.com"> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">And that's how it is today, the alloc-on-fault mechanism is being used in at least 3 drivers, and all I'm trying to do here is standardize it and try to document the constraints that comes with this model, constraint that are currently being ignored. Like the fact allocations in the fault handler path shouldn't block so we're guaranteed to signal the job fence in finite time, and we don't risk a deadlock between the driver shrinker and the job triggering the fault. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Well on one hand it's good to that we document the pitfalls, but I clearly think that we should *not* spread that into common code. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Ack on not encouraging people to use that; but having a clear path describing how this should be done for HW that don't have other options, with helpers and extensive doc is IMHO better than letting new drivers reproduce the mistake that were done in the past. Because, if you tell people "don't do that", but don't have an alternative to propose, they'll end up doing it anyway.</pre> </blockquote> Just to be clear: We have already completely rejected code from going upstream because of that! And we are not talking about anything small, but rather a full blown framework and every developed by a major player. Additional to that both i915 and amdgpu had code which used this approach as well and we reverted back because it simply doesn't work reliable. <blockquote type="cite" cite="mid:20250410192054.24a592a5@collabora.com"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> That would give the impression that this actually works and to be honest I should start to charge money to rejecting stuff like that. It would probably get me rich. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">I'm well aware of the implications of what I'm proposing here, but ignoring the fact some drivers already violate the rules don't make them disappear. So I'd rather work with you and others to clearly state what the alloc-in-fault-path rules are, and enforce them in some helper functions, than pretend these ugly things don't exist. :D </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Yeah, but this kind of says to others it's ok to do this which clearly isn't as far as I can see. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Not really. At least not if we properly review any driver that use these APIs, and clearly state in the doc that this is provided as a last resort for HW that can't do without on-fault-allocation, because they are designed to work this way. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> What we should do instead is to add this as not ok approaches to the documentation and move on. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Well, that's what happened with panfrost, lima and panthor, and see where we are now: 3 drivers that you consider broken (probably rightfully), and more to come if we don't come up with a plan for HW that have the same requirements (as I said, I wouldn't be surprised if most tilers were relying on the same kind of on-demand-allocation).</pre> </blockquote> Well we have HW features in major drivers which we simply don't use because of that. <blockquote type="cite" cite="mid:20250410192054.24a592a5@collabora.com"> <pre class="moz-quote-pre" wrap="">As always, I appreciate your valuable inputs, and would be happy to try anything you think might be more adapted than what is proposed here, but saying "this is broken HW/driver, so let's ignore it" is not really an option, at least if you don't want the bad design pattern to spread.</pre> </blockquote> Yeah, I'm not sure what to do either. We *know* from experience that this approach will fail. We have already tried that. Regards, Christian. <blockquote type="cite" cite="mid:20250410192054.24a592a5@collabora.com"> <pre class="moz-quote-pre" wrap=""> Regards, Boris </pre> </blockquote> </body> </html>