On Fri, Nov 10, 2023 at 02:23:16PM +0000, Simon Ser wrote: > On Friday, November 10th, 2023 at 15:01, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > > On Fri, Nov 10, 2023 at 11:21:15AM +0000, Simon Ser wrote: > > > > > On Thursday, November 9th, 2023 at 20:17, Maxime Ripard mripard@xxxxxxxxxx wrote: > > > > > > > > Can we add another function pointer to the struct drm_driver (or > > > > > similar) to do the allocation, and move the actual dmabuf handling > > > > > code into the core? > > > > > > > > Yeah, I agree here, it just seems easier to provide a global hook and a > > > > somewhat sane default to cover all drivers in one go (potentially with > > > > additional restrictions, like only for modeset-only drivers or > > > > something). > > > > > > First off not all drivers are using the GEM DMA helpers (e.g. vc4 with > > > !vc5 does not). > > > > Right. vc4 pre-RPi4 is the exception though, so it's kind of what I > > meant by providing sane defaults: the vast majority of drivers are using > > GEM DMA helpers, and we should totally let drivers override that if > > relevant. > > > > Basically, we already have 2.5 citizen classes, I'd really like to avoid > > having 3 officially, even more so if it's not super difficult to do. > > > > > The heap code in this patch only works with drivers leveraging GEM DMA > > > helpers. > > > > We could add a new hook to drm_driver to allocate heaps, link to a > > default implementation in DRM_GEM_DMA_DRIVER_OPS_WITH_DUMB_CREATE(), and > > then use that new hook in your new heap. It would already cover 40 > > drivers at the moment, instead of just one, with all of them (but > > atmel-hlcdc maybe?) being in the same situation than RPi4-vc4 is. > > As said in another e-mail, I really think the consequences of DMA heaps > need to be thought out on a per-driver basis. The issue you pointed out doesn't show up on a per-driver basis though. > Moreover this approach makes core DRM go in the wrong direction, > deeper in midlayer territory. I have no idea why that makes it more of a midlayer here, but even if it does, just because it does doesn't mean it's bad or something to avoid. We've been striving for more than a decade now to make drivers easy to write and easy to extend / deviate from the norm. AFAIK, what I suggested provides both. Yours create unnecessary boilerplate and will leave a lot of drivers out of a needed solution. > > > Then maybe it's somewhat simple to cover typical display devices found > > > on split render/display SoCs, however for the rest it's going to be much > > > more complicated. For x86 typically there are multiple buffer placements > > > supported by the GPU and we need to have one heap per possible placement. > > > And then figuring out all of the rules, priority and compatibility stuff > > > is a whole other task in and of itself. > > > > But x86 typically doesn't have a split render/display setup, right? > > So you're saying we should fix everything at once, but why is x86 not > part of "everything" then? "everything" is your word, not mine. I'm saying that the issue you've mentioned for this series applies to a lot of other drivers, and we should fix it for those too. x86 doesn't suffer from the issue you've mentioned, just like the Pi0-3, and thus we don't have to come up with a solution for them. > x86 also has issues regarding buffer placement. You're saying you > don't want to fragment the ecosystem, but it seems like there would > still be "fragmentation" between split render/display SoCs and the > rest? If you want to come up with a generic solution to buffer placement, then you need to consider both sides. A buffer that can be addressed by the scanout engine might not be addressable by the codec that will fill that buffer for example. The problem is far broader than what you described here, and the solution far more involved too. I don't see why you're bringing that up here, I don't think we need a solution for that at this point, and yet I think your current patch is a step in the right direction if we enroll every relevant driver. Maxime > I'm having a hard time understanding your goals here.
Attachment:
signature.asc
Description: PGP signature
