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

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Airlie <airlied@gmail.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>,
	 Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	 "Syrjala, Ville" <ville.syrjala@intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	 Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH 01/10] drm/i915: move display funcs into a display struct.
Date: Tue, 7 Sep 2021 10:14:51 +0200	[thread overview]
Message-ID: <CAKMK7uHeujMMWoo-AJuuksg7HBpQu0oqMEbgDHUx8_rDv2GDng@mail.gmail.com> (raw)
In-Reply-To: <CAPM=9ty4y9new=adp+cmKCP0fuGrMgJOCWGSV-nmzorN3nACKw@mail.gmail.com>

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.

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

> Has anyone any non-anecdotal knowledge of how bad the latter problem
> actually is?
> Other drivers seem to do a lot of it and nobody has complained about it.
>
> I'm happy to move to a pointer for it all to be honest,
> Dave.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch