[Intel-gfx] [PATCH 01/10] drm/i915: move display funcs into a display struct.

[Ville Syrjälä]

On Tue, Sep 07, 2021 at 07:52:17PM +1000, Dave Airlie wrote:
> On Tue, 7 Sept 2021 at 18:15, Daniel Vetter <daniel at ffwll.ch> wrote:
> >
> > On Mon, Sep 6, 2021 at 9:45 PM Dave Airlie <airlied at gmail.com> wrote:
> > > On Mon, 6 Sept 2021 at 18:18, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> > > > On Mon, 06 Sep 2021, Dave Airlie <airlied at gmail.com> wrote:
> > > > > From: Dave Airlie <airlied at redhat.com>
> > > > >
> > > > > 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.

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

-- 
Ville Syrjälä
Intel
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.