On Wed, 8 Sept 2021 at 04:14, Ville Syrjälä <ville.syrjala@xxxxxxxxx> wrote:
>
> On Tue, Sep 07, 2021 at 07:52:17PM +1000, Dave Airlie wrote:
> > On Tue, 7 Sept 2021 at 18:15, Daniel Vetter <daniel@xxxxxxxx> wrote:
> > >
> > > On Mon, Sep 6, 2021 at 9:45 PM Dave Airlie <airlied@xxxxxxxxx> wrote:
> > > > On Mon, 6 Sept 2021 at 18:18, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > > > > On Mon, 06 Sep 2021, Dave Airlie <airlied@xxxxxxxxx> wrote:
> > > > > > From: Dave Airlie <airlied@xxxxxxxxxx>
> > > > > >
> > > > > > This is the first step in an idea to refactor the display code
> > > > > > into a bit more of a corner.
> > > > >
> > > > > So, do we want to make i915->display a pointer?
> > > > >
> > > > > If we do, and we're about to touch every place accessing the display
> > > > > struct, we might just as well have:
> > > > >
> > > > > struct drm_i915_private {
> > > > > struct drm_i915_display _display;
> > > > > struct drm_i915_display *display;
> > > > > };
> > > > >
> > > > > and initialize i915->display = &i915->_display, and make all access
> > > > > happen via the pointer. If we want to allocate it dynamically at some
> > > > > point, it'll be a *much* easier task.
> > > > >
> > > > > But the first question to figure out is whether we want to do that or
> > > > > not.
> > > >
> > > > I think the advantage is that we can hide a lot more structs from the
> > > > main struct,
> > > > the disadvantage is additional pointer chasing, esp for the drm_device and other
> > > > i915_priv members.
> > >
> > > For display pointer chasing doesn't matter at all. Imo the case is
> > > more what make sense as object hierarchy, and embedding vs pointer has
> > > quite different meaning. We've discussed in the past that the split
> > > into display/gem with branches seems to work ok-ish, but could
> > > probably be improved a lot in code org. If we make display a lot more
> > > a free-standing thing (i.e. pointer, not embedding) with a much more
> > > clearer/cleaner api contract to other pieces, then maybe there's some
> > > case to be made for all this churn.
> >
> > I'd like to make it at least have some form of API between display and core/gt.
> >
> > I think the main things I've noticed where it's kinda free for all at
> > the moment are:
> > - display funcs has pm internal funcs, display<->pm funcs, display
> > only audio funcs,
> > display only color funcs, other display internal funcs all mixed into
> > one super struct.
>
> I think the solution for these is mostly just more localized function
> pointer structs/etc. Never been a fan of the old catch-all display
> funcs thing. Pretty sure I even have eg. a cdclk_funcs struct sitting
> in some branch somewhere.
I've just posted a series cleaning this out, I've got some followups
that more of an RFC
https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-i915-display-funcs-constify
(the last 7 patches there).
Those move the struct defs that are internal vtables into the display headers.
> > There's no split between things that provide service to display and vice-versa.
> > I've started looking at splitting this.
> > - tracepoints - i915_trace.h pulls in display and gt stuff, no idea if
> > we can split that,
> > but lots of things include i915_trace.h which means everyone sees
> > everyone elses guts.
>
> Yeah, I've been thinking of splitting this but haven't actually looked
> into how feasible it is. The convoluted macro stuff makes it hard to
> see how any of it really works, and in the past I did have some real
> include order problems with it.
Yeah I started looking yesterday and ran into macro pain, I might try
another run at it.
Dave.
