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