diff mbox series

[07/14] drm/mgag200: Replace simple-KMS with regular atomic helpers

Message ID 20220708093929.4446-8-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/mgag200: Move model-specific code into separate functions | expand

Commit Message

Thomas Zimmermann July 8, 2022, 9:39 a.m. UTC
Drop simple-KMS in favor of regular atomic helpers. Makes the code
more modular and hence better to adapt to per-model requirements.

The simple-KMS helpers provide few extra features, so the patch is
mostly about open-coding what simple-KMS does. The simple-KMS helpers
do mix up plane and CRTC state. Changing to regular atomic helpers
requires to split some of the simple-pipe functions into per-plane
and per-CRTC code

No functional changes

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |   8 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c | 385 +++++++++++++++----------
 2 files changed, 236 insertions(+), 157 deletions(-)

Comments

Thomas Zimmermann April 25, 2023, 3:03 p.m. UTC | #1
(cc'ing dri-devel, Jocelyn and Sam)

Hi Phil,

I've put dri-devel into cc, which is the developer's mailing list. It's 
the first time I hear about this bug.

Am 25.04.23 um 16:25 schrieb kernel@linuxace.com:
> Hi Thomas,
> 
> I have been trying to track down why we lost console on our Dell servers since
> switching to kernel 6.1, and finally narrowed it down to the commit referenced
> in the subject (1baf9127c482).  If I boot kernel 1baf9127c482, I will have
> no console at all on my servers.  Booting the prior kernel (4f4dc37e374c) restores
> console.  The server I am testing on has a G200EH card.
> 
> There is a bug report about this (not opened by me) here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=2171155
> 
> but I'm not sure if RedHat bugzilla is the best place to report this.  Any
> suggestions for a better place?  I'm available for any testing.  I've already
> tried simply reverting this commit from 6.1 but it does not reverse cleanly
> given all the other MGA changes made after this specific commit.  Any
> guidance you could provide is appreciated.

You cannot really revert it, as it's too old already. But could you 
please try the latest developer tree from

   git://anongit.freedesktop.org/drm/drm-tip

The branch is drm-tip. Maybe the bug has been fixed meanwhile. If this 
also doesn't work, we can take a closer look at the changes.

Best regards
Thomas

> 
> Thanks,
> Phil
Jocelyn Falempe April 25, 2023, 3:48 p.m. UTC | #2
On 25/04/2023 17:03, Thomas Zimmermann wrote:
> (cc'ing dri-devel, Jocelyn and Sam)
> 
> Hi Phil,
> 
> I've put dri-devel into cc, which is the developer's mailing list. It's 
> the first time I hear about this bug.

Thanks for pointing this to me, I will take a look at it.
Phil Oester April 25, 2023, 7:56 p.m. UTC | #3
On Tue, Apr 25, 2023 at 8:03 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>  You cannot really revert it, as it's too old already. But could you
>  please try the latest developer tree from
>
>     git://anongit.freedesktop.org/drm/drm-tip
>
>  The branch is drm-tip. Maybe the bug has been fixed meanwhile. If this
>  also doesn't work, we can take a closer look at the changes.

I tried drm-tip, and it did not help.

Thanks,
Phil
Phil Oester April 25, 2023, 11:39 p.m. UTC | #4
I'm not sure if this information is useful, but we have found one system which
works with the 6.1 kernel.  I grabbed the lspci output from a working and
non-working system, pasted below.  Same kernel and same distro.  

BAD Dell R815 AMD
0a:03.0 VGA compatible controller: Matrox Electronics Systems Ltd. MGA G200eW
        WPCM450 (rev 0a) (prog-if 00 [VGA controller])
       DeviceName: Embedded Video                           
       Subsystem: Dell Device 0444
       Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
       Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
       Latency: 32 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
       Interrupt: pin A routed to IRQ 5
       NUMA node: 0
       IOMMU group: 10
       Region 0: Memory at ec800000 (32-bit, prefetchable) [size=8M]
       Region 1: Memory at ef7fc000 (32-bit, non-prefetchable) [size=16K]
       Region 2: Memory at ee800000 (32-bit, non-prefetchable) [size=8M]
       Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
       Capabilities: [dc] Power Management version 1
               Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
               Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
00: 2b 10 32 05 07 00 90 02 0a 00 00 03 10 20 00 00
10: 08 00 80 ec 00 c0 7f ef 00 00 80 ee 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 44 04
30: 00 00 00 00 dc 00 00 00 00 00 00 00 05 01 10 20

GOOD Dell R710 INTEL
0a:03.0 VGA compatible controller: Matrox Electronics Systems Ltd. MGA G200eW
        WPCM450 (rev 0a) (prog-if 00 [VGA controller])
       DeviceName: Embedded Video                           
       Subsystem: Dell PowerEdge R710 MGA G200eW WPCM450
       Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
       Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
       Latency: 32 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
       Interrupt: pin A routed to IRQ 10
       Region 0: Memory at d5000000 (32-bit, prefetchable) [size=8M]
       Region 1: Memory at deffc000 (32-bit, non-prefetchable) [size=16K]
       Region 2: Memory at df000000 (32-bit, non-prefetchable) [size=8M]
       Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
       Capabilities: [dc] Power Management version 1
               Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
               Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
00: 2b 10 32 05 07 00 90 02 0a 00 00 03 10 20 00 00
10: 08 00 00 d5 00 c0 ff de 00 00 00 df 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 35 02
30: 00 00 00 00 dc 00 00 00 00 00 00 00 0a 01 10 20

Phil
Thomas Zimmermann April 26, 2023, 7:01 a.m. UTC | #5
Hi

Am 26.04.23 um 01:39 schrieb kernel@linuxace.com:
> I'm not sure if this information is useful, but we have found one system which
> works with the 6.1 kernel.  I grabbed the lspci output from a working and
> non-working system, pasted below.  Same kernel and same distro.
> 
> BAD Dell R815 AMD
> 0a:03.0 VGA compatible controller: Matrox Electronics Systems Ltd. MGA G200eW

Thanks for this info. That's a G200eW, which uses slightly different 
code within the driver. From the testing we've done, we didn't see the 
bug either. So it's probably something very specific to your machine or 
the G200EH model.

Best regards
Thomas

>          WPCM450 (rev 0a) (prog-if 00 [VGA controller])
>         DeviceName: Embedded Video
>         Subsystem: Dell Device 0444
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 32 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
>         Interrupt: pin A routed to IRQ 5
>         NUMA node: 0
>         IOMMU group: 10
>         Region 0: Memory at ec800000 (32-bit, prefetchable) [size=8M]
>         Region 1: Memory at ef7fc000 (32-bit, non-prefetchable) [size=16K]
>         Region 2: Memory at ee800000 (32-bit, non-prefetchable) [size=8M]
>         Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
>         Capabilities: [dc] Power Management version 1
>                 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> 00: 2b 10 32 05 07 00 90 02 0a 00 00 03 10 20 00 00
> 10: 08 00 80 ec 00 c0 7f ef 00 00 80 ee 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 44 04
> 30: 00 00 00 00 dc 00 00 00 00 00 00 00 05 01 10 20
> 
> GOOD Dell R710 INTEL
> 0a:03.0 VGA compatible controller: Matrox Electronics Systems Ltd. MGA G200eW
>          WPCM450 (rev 0a) (prog-if 00 [VGA controller])
>         DeviceName: Embedded Video
>         Subsystem: Dell PowerEdge R710 MGA G200eW WPCM450
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 32 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
>         Interrupt: pin A routed to IRQ 10
>         Region 0: Memory at d5000000 (32-bit, prefetchable) [size=8M]
>         Region 1: Memory at deffc000 (32-bit, non-prefetchable) [size=16K]
>         Region 2: Memory at df000000 (32-bit, non-prefetchable) [size=8M]
>         Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
>         Capabilities: [dc] Power Management version 1
>                 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> 00: 2b 10 32 05 07 00 90 02 0a 00 00 03 10 20 00 00
> 10: 08 00 00 d5 00 c0 ff de 00 00 00 df 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 35 02
> 30: 00 00 00 00 dc 00 00 00 00 00 00 00 0a 01 10 20
> 
> Phil
Thomas Zimmermann April 26, 2023, 10:15 a.m. UTC | #6
Am 26.04.23 um 09:01 schrieb Thomas Zimmermann:
> Hi
> 
> Am 26.04.23 um 01:39 schrieb kernel@linuxace.com:
>> I'm not sure if this information is useful, but we have found one 
>> system which
>> works with the 6.1 kernel.  I grabbed the lspci output from a working and
>> non-working system, pasted below.  Same kernel and same distro.
>>
>> BAD Dell R815 AMD
>> 0a:03.0 VGA compatible controller: Matrox Electronics Systems Ltd. MGA 
>> G200eW
> 
> Thanks for this info. That's a G200eW, which uses slightly different 
> code within the driver. From the testing we've done, we didn't see the 
> bug either. So it's probably something very specific to your machine or 
> the G200EH model.

Oh, now I got it. This G200eW is broken. The other one is good. And the 
kernel is also broken on the G200EH?

Best regards
Thomas

> 
> Best regards
> Thomas
> 
>>          WPCM450 (rev 0a) (prog-if 00 [VGA controller])
>>         DeviceName: Embedded Video
>>         Subsystem: Dell Device 0444
>>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- 
>> ParErr- Stepping- SERR- FastB2B- DisINTx-
>>         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium 
>> >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>>         Latency: 32 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
>>         Interrupt: pin A routed to IRQ 5
>>         NUMA node: 0
>>         IOMMU group: 10
>>         Region 0: Memory at ec800000 (32-bit, prefetchable) [size=8M]
>>         Region 1: Memory at ef7fc000 (32-bit, non-prefetchable) 
>> [size=16K]
>>         Region 2: Memory at ee800000 (32-bit, non-prefetchable) [size=8M]
>>         Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
>>         Capabilities: [dc] Power Management version 1
>>                 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA 
>> PME(D0-,D1-,D2-,D3hot-,D3cold-)
>>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>> 00: 2b 10 32 05 07 00 90 02 0a 00 00 03 10 20 00 00
>> 10: 08 00 80 ec 00 c0 7f ef 00 00 80 ee 00 00 00 00
>> 20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 44 04
>> 30: 00 00 00 00 dc 00 00 00 00 00 00 00 05 01 10 20
>>
>> GOOD Dell R710 INTEL
>> 0a:03.0 VGA compatible controller: Matrox Electronics Systems Ltd. MGA 
>> G200eW
>>          WPCM450 (rev 0a) (prog-if 00 [VGA controller])
>>         DeviceName: Embedded Video
>>         Subsystem: Dell PowerEdge R710 MGA G200eW WPCM450
>>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- 
>> ParErr- Stepping- SERR- FastB2B- DisINTx-
>>         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium 
>> >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>>         Latency: 32 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
>>         Interrupt: pin A routed to IRQ 10
>>         Region 0: Memory at d5000000 (32-bit, prefetchable) [size=8M]
>>         Region 1: Memory at deffc000 (32-bit, non-prefetchable) 
>> [size=16K]
>>         Region 2: Memory at df000000 (32-bit, non-prefetchable) [size=8M]
>>         Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
>>         Capabilities: [dc] Power Management version 1
>>                 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA 
>> PME(D0-,D1-,D2-,D3hot-,D3cold-)
>>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>> 00: 2b 10 32 05 07 00 90 02 0a 00 00 03 10 20 00 00
>> 10: 08 00 00 d5 00 c0 ff de 00 00 00 df 00 00 00 00
>> 20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 35 02
>> 30: 00 00 00 00 dc 00 00 00 00 00 00 00 0a 01 10 20
>>
>> Phil
>
Phil Oester April 27, 2023, 12:47 a.m. UTC | #7
On Wed, Apr 26, 2023 at 3:15 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Oh, now I got it. This G200eW is broken. The other one is good. And the
> kernel is also broken on the G200EH?

Correct, which doesn't make much sense.  I have two Dells, both of which
have 102b:0532, but only one has no video.  On your second point, I do have
a number of HP DL360p G8s with G200EH [102b:0533] which also have no video.

Phil
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index b5bccbc8820d..84579c2e1f3c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -15,11 +15,13 @@ 
 
 #include <video/vga.h>
 
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_shmem_helper.h>
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_plane.h>
 
 #include "mgag200_reg.h"
 
@@ -249,9 +251,11 @@  struct mga_device {
 	enum mga_type			type;
 
 	struct mgag200_pll pixpll;
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
 	struct mga_i2c_chan i2c;
 	struct drm_connector connector;
-	struct drm_simple_display_pipe display_pipe;
 };
 
 static inline struct mga_device *to_mga_device(struct drm_device *dev)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index fe11bb5d092e..39509dd84b23 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -24,7 +24,6 @@ 
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "mgag200_drv.h"
 
@@ -615,25 +614,108 @@  static void mgag200_handle_damage(struct mga_device *mdev, const struct iosys_ma
 }
 
 /*
- * Simple Display Pipe
+ * Primary plane
  */
 
-static const uint32_t mgag200_simple_display_pipe_formats[] = {
+static const uint32_t mgag200_primary_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_RGB565,
 	DRM_FORMAT_RGB888,
 };
 
-static const uint64_t mgag200_simple_display_pipe_fmtmods[] = {
+static const uint64_t mgag200_primary_plane_fmtmods[] = {
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
 };
 
-static enum drm_mode_status
-mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
-				       const struct drm_display_mode *mode)
+static int mgag200_primary_plane_helper_atomic_check(struct drm_plane *plane,
+						     struct drm_atomic_state *state)
 {
-	struct mga_device *mdev = to_mga_device(pipe->crtc.dev);
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_framebuffer *new_fb = new_plane_state->fb;
+	struct drm_framebuffer *fb = NULL;
+	struct drm_crtc_state *new_crtc_state;
+	struct mgag200_crtc_state *new_mgag200_crtc_state;
+	int ret;
+
+	if (!new_fb)
+		return 0;
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, true);
+	if (ret)
+		return ret;
+
+	if (!new_plane_state->visible)
+		return 0;
+
+	if (plane->state)
+		fb = plane->state->fb;
+
+	if (!fb || (fb->format != new_fb->format))
+		new_crtc_state->mode_changed = true; /* update PLL settings */
+
+	new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
+	new_mgag200_crtc_state->format = new_fb->format;
+
+	return 0;
+}
+
+static void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						       struct drm_atomic_state *old_state)
+{
+	struct drm_device *dev = plane->dev;
+	struct mga_device *mdev = to_mga_device(dev);
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_rect damage;
+
+	if (!fb)
+		return;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		mgag200_handle_damage(mdev, shadow_plane_state->data, fb, &damage);
+	}
+
+	/* Always scanout image at VRAM offset 0 */
+	mgag200_set_startadd(mdev, (u32)0);
+	mgag200_set_offset(mdev, fb);
+}
+
+static void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
+							struct drm_atomic_state *old_state)
+{ }
+
+static const struct drm_plane_helper_funcs mgag200_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = mgag200_primary_plane_helper_atomic_check,
+	.atomic_update = mgag200_primary_plane_helper_atomic_update,
+	.atomic_disable = mgag200_primary_plane_helper_atomic_disable,
+};
+
+static const struct drm_plane_funcs mgag200_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+/*
+ * CRTC
+ */
+
+static enum drm_mode_status mgag200_crtc_helper_mode_valid(struct drm_crtc *crtc,
+							   const struct drm_display_mode *mode)
+{
+	struct mga_device *mdev = to_mga_device(crtc->dev);
 	const struct mgag200_device_info *info = mdev->info;
 
 	/*
@@ -660,26 +742,70 @@  mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
 	return MODE_OK;
 }
 
-static void
-mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
-				   struct drm_crtc_state *crtc_state,
-				   struct drm_plane_state *plane_state)
+static int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc,
+					    struct drm_atomic_state *state)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_device *dev = crtc->dev;
 	struct mga_device *mdev = to_mga_device(dev);
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct mgag200_pll *pixpll = &mdev->pixpll;
+	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
+	int ret;
+
+	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
+	if (ret)
+		return ret;
+
+	if (!new_crtc_state->enable)
+		return 0;
+
+	if (new_crtc_state->mode_changed) {
+		ret = pixpll->funcs->compute(pixpll, new_crtc_state->mode.clock,
+					     &mgag200_crtc_state->pixpllc);
+		if (ret)
+			return ret;
+	}
+
+	if (new_crtc_state->color_mgmt_changed && new_crtc_state->gamma_lut) {
+		if (new_crtc_state->gamma_lut->length !=
+		    MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
+			drm_err(dev, "Wrong size for gamma_lut %zu\n",
+				new_crtc_state->gamma_lut->length);
+			return -EINVAL;
+		}
+	}
+
+	return drm_atomic_add_affected_planes(state, crtc);
+}
+
+static void mgag200_crtc_helper_atomic_flush(struct drm_crtc *crtc,
+					     struct drm_atomic_state *old_state)
+{
+	struct drm_crtc_state *crtc_state = crtc->state;
+	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
+	struct drm_device *dev = crtc->dev;
+	struct mga_device *mdev = to_mga_device(dev);
+
+	if (crtc_state->enable && crtc_state->color_mgmt_changed) {
+		const struct drm_format_info *format = mgag200_crtc_state->format;
+
+		if (crtc_state->gamma_lut)
+			mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
+		else
+			mgag200_crtc_set_gamma_linear(mdev, format);
+	}
+}
+
+static void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+					      struct drm_atomic_state *old_state)
+{
+	struct drm_device *dev = crtc->dev;
+	struct mga_device *mdev = to_mga_device(dev);
+	struct drm_crtc_state *crtc_state = crtc->state;
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
-	struct drm_framebuffer *fb = plane_state->fb;
 	const struct drm_format_info *format = mgag200_crtc_state->format;
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
-	struct drm_rect fullscreen = {
-		.x1 = 0,
-		.x2 = fb->width,
-		.y1 = 0,
-		.y2 = fb->height,
-	};
+	struct mgag200_pll *pixpll = &mdev->pixpll;
 
 	if (mdev->type == G200_WB || mdev->type == G200_EW3)
 		mgag200_g200wb_hold_bmc(mdev);
@@ -697,108 +823,50 @@  mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	else if (mdev->type == G200_EV)
 		mgag200_g200ev_set_hiprilvl(mdev);
 
-	if (mdev->type == G200_WB || mdev->type == G200_EW3)
-		mgag200_g200wb_release_bmc(mdev);
-
-	if (crtc_state->gamma_lut)
-		mgag200_crtc_set_gamma(mdev, format, crtc_state->gamma_lut->data);
-	else
-		mgag200_crtc_set_gamma_linear(mdev, format);
-
 	mgag200_enable_display(mdev);
 
-	mgag200_handle_damage(mdev, shadow_plane_state->data, fb, &fullscreen);
-
-	/* Always scanout image at VRAM offset 0 */
-	mgag200_set_startadd(mdev, (u32)0);
-	mgag200_set_offset(mdev, fb);
+	if (mdev->type == G200_WB || mdev->type == G200_EW3)
+		mgag200_g200wb_release_bmc(mdev);
 }
 
-static void
-mgag200_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+					       struct drm_atomic_state *old_state)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
 	struct mga_device *mdev = to_mga_device(crtc->dev);
 
-	mgag200_disable_display(mdev);
-}
-
-static int
-mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
-				  struct drm_plane_state *plane_state,
-				  struct drm_crtc_state *crtc_state)
-{
-	struct drm_plane *plane = plane_state->plane;
-	struct drm_device *dev = plane->dev;
-	struct mga_device *mdev = to_mga_device(dev);
-	struct mgag200_pll *pixpll = &mdev->pixpll;
-	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
-	struct drm_framebuffer *new_fb = plane_state->fb;
-	struct drm_framebuffer *fb = NULL;
-	int ret;
-
-	if (!new_fb)
-		return 0;
-
-	if (plane->state)
-		fb = plane->state->fb;
-
-	if (!fb || (fb->format != new_fb->format))
-		crtc_state->mode_changed = true; /* update PLL settings */
-
-	mgag200_crtc_state->format = new_fb->format;
+	if (mdev->type == G200_WB || mdev->type == G200_EW3)
+		mgag200_g200wb_hold_bmc(mdev);
 
-	if (crtc_state->mode_changed) {
-		ret = pixpll->funcs->compute(pixpll, crtc_state->mode.clock,
-					     &mgag200_crtc_state->pixpllc);
-		if (ret)
-			return ret;
-	}
+	mgag200_disable_display(mdev);
 
-	if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
-		if (crtc_state->gamma_lut->length !=
-		    MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
-			drm_err(dev, "Wrong size for gamma_lut %zu\n",
-				crtc_state->gamma_lut->length);
-			return -EINVAL;
-		}
-	}
-	return 0;
+	if (mdev->type == G200_WB || mdev->type == G200_EW3)
+		mgag200_g200wb_release_bmc(mdev);
 }
 
-static void
-mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
-				   struct drm_plane_state *old_state)
-{
-	struct drm_plane *plane = &pipe->plane;
-	struct drm_crtc *crtc = &pipe->crtc;
-	struct drm_device *dev = plane->dev;
-	struct mga_device *mdev = to_mga_device(dev);
-	struct drm_plane_state *state = plane->state;
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
-	struct drm_framebuffer *fb = state->fb;
-	struct drm_rect damage;
-	struct drm_atomic_helper_damage_iter iter;
+static const struct drm_crtc_helper_funcs mgag200_crtc_helper_funcs = {
+	.mode_valid = mgag200_crtc_helper_mode_valid,
+	.atomic_check = mgag200_crtc_helper_atomic_check,
+	.atomic_flush = mgag200_crtc_helper_atomic_flush,
+	.atomic_enable = mgag200_crtc_helper_atomic_enable,
+	.atomic_disable = mgag200_crtc_helper_atomic_disable,
+};
 
-	if (!fb)
-		return;
+static void mgag200_crtc_reset(struct drm_crtc *crtc)
+{
+	struct mgag200_crtc_state *mgag200_crtc_state;
 
-	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
-		mgag200_crtc_set_gamma(mdev, fb->format, crtc->state->gamma_lut->data);
+	if (crtc->state)
+		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
 
-	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
-	drm_atomic_for_each_plane_damage(&iter, &damage) {
-		mgag200_handle_damage(mdev, shadow_plane_state->data, fb, &damage);
-	}
-	/* Always scanout image at VRAM offset 0 */
-	mgag200_set_startadd(mdev, (u32)0);
-	mgag200_set_offset(mdev, fb);
+	mgag200_crtc_state = kzalloc(sizeof(*mgag200_crtc_state), GFP_KERNEL);
+	if (mgag200_crtc_state)
+		__drm_atomic_helper_crtc_reset(crtc, &mgag200_crtc_state->base);
+	else
+		__drm_atomic_helper_crtc_reset(crtc, NULL);
 }
 
-static struct drm_crtc_state *
-mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe *pipe)
+static struct drm_crtc_state *mgag200_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
 {
-	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_crtc_state *crtc_state = crtc->state;
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	struct mgag200_crtc_state *new_mgag200_crtc_state;
@@ -818,8 +886,8 @@  mgag200_simple_display_pipe_duplicate_crtc_state(struct drm_simple_display_pipe
 	return &new_mgag200_crtc_state->base;
 }
 
-static void mgag200_simple_display_pipe_destroy_crtc_state(struct drm_simple_display_pipe *pipe,
-							   struct drm_crtc_state *crtc_state)
+static void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc,
+					      struct drm_crtc_state *crtc_state)
 {
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 
@@ -827,33 +895,21 @@  static void mgag200_simple_display_pipe_destroy_crtc_state(struct drm_simple_dis
 	kfree(mgag200_crtc_state);
 }
 
-static void mgag200_simple_display_pipe_reset_crtc(struct drm_simple_display_pipe *pipe)
-{
-	struct drm_crtc *crtc = &pipe->crtc;
-	struct mgag200_crtc_state *mgag200_crtc_state;
-
-	if (crtc->state) {
-		mgag200_simple_display_pipe_destroy_crtc_state(pipe, crtc->state);
-		crtc->state = NULL; /* must be set to NULL here */
-	}
+static const struct drm_crtc_funcs mgag200_crtc_funcs = {
+	.reset = mgag200_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = mgag200_crtc_atomic_duplicate_state,
+	.atomic_destroy_state = mgag200_crtc_atomic_destroy_state,
+};
 
-	mgag200_crtc_state = kzalloc(sizeof(*mgag200_crtc_state), GFP_KERNEL);
-	if (!mgag200_crtc_state)
-		return;
-	__drm_atomic_helper_crtc_reset(crtc, &mgag200_crtc_state->base);
-}
+/*
+ * Encoder
+ */
 
-static const struct drm_simple_display_pipe_funcs
-mgag200_simple_display_pipe_funcs = {
-	.mode_valid = mgag200_simple_display_pipe_mode_valid,
-	.enable	    = mgag200_simple_display_pipe_enable,
-	.disable    = mgag200_simple_display_pipe_disable,
-	.check	    = mgag200_simple_display_pipe_check,
-	.update	    = mgag200_simple_display_pipe_update,
-	.reset_crtc = mgag200_simple_display_pipe_reset_crtc,
-	.duplicate_crtc_state = mgag200_simple_display_pipe_duplicate_crtc_state,
-	.destroy_crtc_state = mgag200_simple_display_pipe_destroy_crtc_state,
-	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
 };
 
 /*
@@ -1000,12 +1056,49 @@  static int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vra
 static int mgag200_pipeline_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
+	struct drm_plane *primary_plane = &mdev->primary_plane;
+	struct drm_crtc *crtc = &mdev->crtc;
+	struct drm_encoder *encoder = &mdev->encoder;
 	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
-	struct drm_simple_display_pipe *pipe = &mdev->display_pipe;
-	size_t format_count = ARRAY_SIZE(mgag200_simple_display_pipe_formats);
 	int ret;
 
+	ret = mgag200_pixpll_init(&mdev->pixpll, mdev);
+	if (ret)
+		return ret;
+
+	ret = drm_universal_plane_init(dev, primary_plane, 0,
+				       &mgag200_primary_plane_funcs,
+				       mgag200_primary_plane_formats,
+				       ARRAY_SIZE(mgag200_primary_plane_formats),
+				       mgag200_primary_plane_fmtmods,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret) {
+		drm_err(dev, "drm_universal_plane_init() failed: %d\n", ret);
+		return ret;
+	}
+	drm_plane_helper_add(primary_plane, &mgag200_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL, &mgag200_crtc_funcs, NULL);
+	if (ret) {
+		drm_err(dev, "drm_crtc_init_with_planes() failed: %d\n", ret);
+		return ret;
+	}
+	drm_crtc_helper_add(crtc, &mgag200_crtc_helper_funcs);
+
+	/* FIXME: legacy gamma tables, but atomic gamma doesn't work without */
+	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
+	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
+
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+	ret = drm_encoder_init(dev, encoder, &mgag200_dac_encoder_funcs,
+			       DRM_MODE_ENCODER_DAC, NULL);
+	if (ret) {
+		drm_err(dev, "drm_encoder_init() failed: %d\n", ret);
+		return ret;
+	}
+
 	ret = mgag200_i2c_init(mdev, i2c);
 	if (ret) {
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
@@ -1022,30 +1115,12 @@  static int mgag200_pipeline_init(struct mga_device *mdev)
 	}
 	drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs);
 
-	ret = mgag200_pixpll_init(&mdev->pixpll, mdev);
-	if (ret)
-		return ret;
-
-	ret = drm_simple_display_pipe_init(dev, pipe,
-					   &mgag200_simple_display_pipe_funcs,
-					   mgag200_simple_display_pipe_formats,
-					   format_count,
-					   mgag200_simple_display_pipe_fmtmods,
-					   connector);
+	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret) {
-		drm_err(dev,
-			"drm_simple_display_pipe_init() failed, error %d\n",
-			ret);
+		drm_err(dev, "drm_connector_attach_encoder() failed: %d\n", ret);
 		return ret;
 	}
 
-	drm_plane_enable_fb_damage_clips(&pipe->plane);
-
-	/* FIXME: legacy gamma tables, but atomic gamma doesn't work without */
-	drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
-
-	drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE);
-
 	return 0;
 }