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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 27 Sep 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

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