On Wed, 27 Sep 2023, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Wed, Sep 27, 2023 at 05:50:10PM +0300, Jani Nikula wrote: >> On Wed, 27 Sep 2023, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: >> > On Tue, Sep 26, 2023 at 12:34:35PM +0300, Jani Nikula wrote: >> >> On Tue, 26 Sep 2023, "Manna, Animesh" <animesh.manna@xxxxxxxxx> wrote: >> >> >> -----Original Message----- >> >> >> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> >> >> >> Sent: Monday, September 25, 2023 6:00 PM >> >> >> To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel- >> >> >> gfx@xxxxxxxxxxxxxxxxxxxxx >> >> >> Subject: Re: [PATCH] drm/i915/dsb: DSB code refactoring >> >> >> >> >> >> On Sat, 23 Sep 2023, Animesh Manna <animesh.manna@xxxxxxxxx> wrote: >> >> >> > Refactor DSB implementation to be compatible with Xe driver. >> >> >> >> >> >> Sad trombone. >> >> >> >> >> >> struct intel_dsb should remain an opaque type. I put effort into hiding its >> >> >> definition, so its guts wouldn't be accessed nilly-willy all over the place. If it's >> >> >> not hidden, it just will get accessed. >> >> > >> >> > Hi Jani, >> >> > >> >> > Xe driver need access to intel_dsb structure, so I can create a new header file intel_dsb_ops.h and keep intel_dsb structure in it. Is it ok? >> >> >> >> I just think you need to find a different abstraction level that doesn't >> >> involve exposing struct intel_dsb. >> > >> > I hate the fact that we seem to be adding these ad-hoc wrappers all >> > over the place. Someone should just fix xe to give us the same API as >> > i915, or a single wrapper should do whatever conversion is needed. >> >> I think one of the problems is that i915 doesn't really give us a proper >> API either, but requires us to fiddle with the objects' guts, and thus >> have access to the struct definitions. In i915, with the include >> hierarchies, that effectively means including absolutely >> everything. Can't have that in xe. > > obj+vma is a pretty reasonable API IMO. And we're not doing anything > weird with their guts IIRC. Okay, I'll take your word for it. > But apparently xe decided to not give us > that, and instead of adding a single wrapper to bridge the gap we > now have several different ad-hoc wrappers for whatever reason. For this specific thing? Do we really have several? Or do you mean all the different things that bridge the gap between xe and i915-display? >> >> Having the same API for both i915 and xe requires turning it into an >> actual API that doesn't depend on either i915 or xe specific types. But >> that's kind of tough before xe is upstream. Catch-22. > > Nothing preventing anyone from coming up with the single wrapper and > upstreaming the i915 side (assuming we even want some kind of extra > wrapper for i915 given it already uses a reasonable approach). Well, so far nobody has stepped up to do that. Needs knowledge of i915-gem, i915-display, and xe. It seems like someone else's problem for everyone working on each of those components. And yeah, I'm not volunteering either. >> Part of the reason we have these ad-hoc wrappers is that they also serve >> as the todo list of stuff to fix properly. > > Feels more like we are trying to polish these to the point where > they are supposed to be permanent solutions. I'm trying to flesh out ideas how to separate i915-display from the rest of i915.ko better [1]. Eventually that'll require very clearly defining the interfaces to/from i915-display as well. Maybe via aux-bus function pointers. Funny thing is, currently the only way to even check what interfaces i915-display needs is to build it as part of xe, where those i915 interfaces aren't available. BR, Jani. [1] https://patchwork.freedesktop.org/series/124286/ -- Jani Nikula, Intel
