On Thu, Nov 06, 2025 at 04:27:42PM +0200, Ville Syrjälä wrote: > On Thu, Nov 06, 2025 at 03:48:25PM +0200, Jani Nikula wrote: > > On Wed, 29 Oct 2025, Jani Nikula <[email protected]> wrote: > > > On Wed, 29 Oct 2025, Jani Nikula <[email protected]> wrote: > > >> On Thu, 16 Oct 2025, Ville Syrjala <[email protected]> wrote: > > >>> From: Ville Syrjälä <[email protected]> > > >>> > > >>> The current attempted split between xe/i915 vs. display > > >>> for intel_frontbuffer is a mess: > > >>> - the i915 rcu leaks through the interface to the display side > > >>> - the obj->frontbuffer write-side is now protected by a display > > >>> specific spinlock even though the actual obj->framebuffer > > >>> pointer lives in a i915 specific structure > > >>> - the kref is getting poked directly from both sides > > >>> - i915_active is still on the display side > > >>> > > >>> Clean up the mess by moving everything about the frontbuffer > > >>> lifetime management to the i915/xe side: > > >>> - the rcu usage is now completely contained in i915 > > >>> - frontbuffer_lock is moved into i915 > > >>> - kref is on the i915/xe side (xe needs the refcount as well > > >>> due to intel_frontbuffer_queue_flush()->intel_frontbuffer_ref()) > > >>> - the bo (and its refcounting) is no longer on the display side > > >>> - i915_active is contained in i915 > > >>> > > >>> I was pondering whether we could do this in some kind of smaller > > >>> steps, and perhaps we could, but it would probably have to start > > >>> with a bunch of reverts (which for sure won't go cleanly anymore). > > >>> So not convinced it's worth the hassle. > > >> > > >> It's a PITA to review, that's for sure. :p > > >> > > >> I'm not particularly fond of embedding struct intel_frontbuffer inside > > >> struct i915_frontbuffer and struct xe_frontbuffer, because it means i915 > > >> and xe will need to know the struct intel_frontbuffer definition. If we > > >> can't live with the embedding long term, we'll probably need opaque > > >> pointers back and forth. > > >> > > >> That said, I think the overall change here is net positive, and makes > > >> life much easier. We don't have to fix everything at once, so let's go > > >> with this. > > >> > > >> I didn't spot any obvious issues, but my confidence level with the > > >> review is super low. :( > > >> > > >> I guess the alternatives are to just go with that, trusting CI, or give > > >> me more time to review. I'm fine either way, as I can trust you to step > > >> up if it goes crashing down. ;) > > > > > > One approach is to send 1-8 first, get CI, get them merged, and then do > > > 9-10 separately, to get separate CI. Maybe? *shrug* > > > > Any conclusions on this? Just merge the whole thing as-is rather than > > let it go stale...? > > I think we could just merge as is. Pretty sure I didn't have any > real functional changes in there.
Merged the whole thing. Thanks for the review. -- Ville Syrjälä Intel
