Re: [Intel-gfx] [PATCH] drm/i915/dsb: DSB code refactoring

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

On Wed, Sep 27, 2023 at 05:50:10PM +0300, Jani Nikula wrote:
> On Wed, 27 Sep 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Sep 26, 2023 at 12:34:35PM +0300, Jani Nikula wrote:
> >> On Tue, 26 Sep 2023, "Manna, Animesh" <animesh.manna@intel.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Jani Nikula <jani.nikula@linux.intel.com>
> >> >> Sent: Monday, September 25, 2023 6:00 PM
> >> >> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> >> >> gfx@lists.freedesktop.org
> >> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dsb: DSB code refactoring
> >> >> 
> >> >> On Sat, 23 Sep 2023, Animesh Manna <animesh.manna@intel.com> 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. 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.

> 
> 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).

> 
> 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.

-- 
Ville Syrjälä
Intel