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

[Dave Airlie <airlied@gmail.com>]

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

One problem area I've noticed after hacking on making display more
contained, was for
lots of things dev_priv can go away, however you'd have to duplicate
all the GRAPHICS_
and IS_* macros which might get messy quick, having access to core
stuff like device_info
and params might need more thought.


> The problem with all that is that we'd then essentially need to
> backpointers everywhere in all display structures: One to the
> drm_device provided by base structs, and the other to the i915_display
> struct, which is what our code uses. This way display would stay
> display. In a way this would then look similarly-ish to amdgpu's DC.
>
> But I'm honestly not sure how much that would improve anything, and
> whether it's worth all the churn to make drm/i915/display more
> self-contained.

It might make display more manageable or future proof if we have
distinct interfaces into it, instead of the free for all we have now.

Dave.