Re: [PATCH 00/29] Enabling new DAL display driver for amdgpu on Carrizo and Tonga

From: Daniel Vetter <daniel@ffwll.ch>
To: Jerome Glisse <j.glisse@gmail.com>
Cc: "Cheng, Tony" <Tony.Cheng@amd.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Lysenko, Mykola" <Mykola.Lysenko@amd.com>
Subject: Re: [PATCH 00/29] Enabling new DAL display driver for amdgpu on Carrizo and Tonga
Date: Sun, 14 Feb 2016 14:23:41 +0100	[thread overview]
Message-ID: <CAKMK7uHq6L1xtEVOBS9eNev0Bg9e+OrqqWK44XTwocB35dxZRQ@mail.gmail.com> (raw)
In-Reply-To: <20160214112208.GA2868@gmail.com>

On Sun, Feb 14, 2016 at 12:22 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> There's still a bunch of legacy stuff in these patches that's on our list of things to refactor. Some of that is
>> - dc/adapter
>> - dc/asic_capability
>> - dc/audio
>> - dc/bios
>> - dc/gpio
>>
>> We should be able to cut the size of this code to about 1/3 of what it is now.
>>
>> As for the LOC we have about
>> 22k for HW programming
>> 30k legacy stuff
>> 6k  dc/calcs - autogenerated from formulas provided by HW team
>> 15k includes
>> 6k  amdgpu_dm
>> 8k  dc/core
>>
>> About 14k of those are blank lines (we have a habit of leaving lots of blank space) and 16k are comments.
>
> Cleaning up that is not enough, abstracting kernel API like kmalloc or i2c,
> or similar, is a no go. If the current drm infrastructure does not suit your
> need then you need to work on improving it to suit your need. You can not
> redevelop a whole drm layer inside your code and expect to upstream it.
>
> Linux device driver are about sharing infrastructure and trying to expose
> it through common API to userspace.
>
> So i strongly suggest that you start thinking on how to change the drm API
> to suit your need and to start discussions about those changes. If you need
> them then they will likely be usefull to others down the road.

A bit more context on why OS abstractions layers like dal here are
massive frowned upon:

First reason is that it makes live painful for everyone who tries to
refactor across the entire subsystem (which I do a lot, so that's what
I care about). For that you need to be able to understand every driver
fairly quickly, and that's only possible if all drivers use the same
concepts. Which means no abstraction layer, but directly
using&extending core drm constructs like
drm_framebuffer/crtc/encoder/bridge/connector/display_information.
Even if your dal concepts match 1:1 with those, if the link is only at
runtime or through pointers instead of through subclassing everything
becomes much more painful to understand. Note that this isn't a reason
against hw/driver specific additional concepts (all big drivers have
those), but against undue amounts of insolation between drm concepts
and driver concepts.

The other bit is the so-called midlayer design mistake,
https://lwn.net/Articles/336262/ which states that wedging an entire
such abstraction layer will also inevitably lead to bad design overall
(neglecting the practical troubles is causes for everyone else). DRM
suffered badly from the midlayer of old days due to compat with *bsd
in the core, which took years to fix up (and is still not completely
done). And we only ever merged 2 drivers who had midlayers like dal,
and both had special reasons:

- exynos was one of the very first armsoc kms drivers, and merged
pretty much for goodwill. Google was eventually fed up with the
midlayer in upstream and paid Collabora to nuke it.

- omapdrm reused the display support code already merged into upstream
in the fbdev subsytem, and that's why it had a midlayer. But Tomi is
now also in the process of removing this.

So even if all the cleanup is done (removal of duplicated code and the
OS abstraction layers) there's some really big reservations against
DAL as a concept itself. That was essentially what's Dave "why do we
need this" was all about. And to answer that you need in my opinion
better reasons "shared abstraction with $otheros/thing/whatever",
since if that's the only reason you argue against 20+ years of shared
experience in linux device driver development that this is a very bad
one.

Note again that this isn't against introducing additional
amd-speicific display concepts in the amd driver, or existing
concepts. Every driver does that. This is just against putting an
entire abstraction layer in-between drm core and your real display
driver.

The other big issue is that upstream really puts a lot of weight onto
continuity - no forking or wholesale rewrites every 1-2 years, instead
gradual change of what's already there. And I suspect the amount of
effort to get dal into a reasonable shape is on par with just adding
the new features you want to amdgpu directly one-by-one. Starting out
with amdgpu also has the added benefit that there's lots of howtos
and experience with converting an existing kms driver to atomic. We
don't really have that for DAL->atomic, and from reading through the
code I couldn't convince myself that the current code is really
atomic. Largely that's due to not using drm core concepts and
structures, but remapping things.

Anyway, I figured I type in a few more words to explain some of the
reasons behind the resistance to massive abstraction layers in kernel
device drivers.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel