Re: [PATCH v2 09/10] drm/i915/frontbuffer: Fix intel_frontbuffer lifetime handling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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 <jani.nikula@intel.com> wrote:
> > > On Wed, 29 Oct 2025, Jani Nikula <jani.nikula@intel.com> wrote:
> > >> On Thu, 16 Oct 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>
> > >>> 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