[PATCH 0/8] drm/mgag200: Support desktop chips

[PATCH 0/8] drm/mgag200: Support desktop chips

[Thomas Zimmermann]

This patchset puts device initialization in the correct order and
adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).

The first 7 patches prepare the driver. Desktop chips would probably
work without them, but since we're at it we can also do it right.

Patch 1 enables cached page mappings GEM buffers. SHMEM supports
this well now and the MGA device does not access the buffer memory
directly. So now corrupt display output is to be expected.

Patches 2 to 6 fix the initialization of device registers. Several
fundamental registers were only done late during device initialization.
This was probably not a problem in practice, as the VGA BIOS does the
setup iduring POST anyway. These patches move the code to the beginning
of the device initialization. If we ever have to POST a MGA device from
the driver, the corect order of operations counts.

G200SEs store a unique id in the device structure. Patch 7 moves the
value to model-specific data area. This will be helpful for patch 8.

Patch 8 adds support for desktop chips' PCI ids. all the memory and
modesetting code continues to work as before. The PLL setup code gets
an additional helper for the new HW. PCI and DAC regsiters get a few
new default values. Most significantly, the driver parses the VGA BIOS
for clock settings. It's all separate from the server code, so no
regressions are to be expected.

The new HW support is based on an earlier patch the was posted in July
2017. [1]

Tested on G200EW and G200 AGP hardware by running the fbdev console,
Weston and Gnome on Xorg.

[1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147647.html

Thomas Zimmermann (8):
  drm/mgag200: Enable caching for SHMEM pages
  drm/mgag200: Move register initialization into helper function
  drm/mgag200: Initialize PCI registers early during device setup
  drm/mgag200: Enable MGA mode during device register initialization
  drm/mgag200: Set MISC memory flags in mm init code
  drm/mgag200: Clear <page> field during MM init
  drm/mgag200: Move G200SE's unique id into model-specific data
  drm/mgag200: Add support for G200 desktop cards

 MAINTAINERS                            |   2 +-
 drivers/gpu/drm/mgag200/Kconfig        |  12 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 213 +++++++++++++++++++++++--
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 ++-
 drivers/gpu/drm/mgag200/mgag200_mm.c   |   8 +
 drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++++++++++-------
 drivers/gpu/drm/mgag200/mgag200_reg.h  |   4 +
 7 files changed, 328 insertions(+), 83 deletions(-)

--
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/8] drm/mgag200: Enable caching for SHMEM pages

[Thomas Zimmermann]

SHMEM pages use write-combine caching by default, but can also use the
platform's default page caching. Doing so may improve the performance
of I/O on the framebuffer.

Mgag200's hardware does not access framebuffer pages directly (i.e.,
via DMA), so enabling caching does not have an effect on consistency
of the framebuffer memory or the displayed data.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index e19660f4a637..7189c7745baf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -36,6 +36,7 @@ static struct drm_driver mgag200_driver = {
 	.major = DRIVER_MAJOR,
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
+	.gem_create_object = drm_gem_shmem_create_object_cached,
 	DRM_GEM_SHMEM_DRIVER_OPS,
 };
 
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/mgag200: Move register initialization into helper function

[Thomas Zimmermann]

The mgag200 driver maps registers into the address space. Move the
code into a separate helper function. No functional changes.

One small difference is in the handling of SDRAM/SGRAM. MGA devices
can come with either SDRAM or SGRAM. So far, the driver checked for
SDRAM, which is the common case. The patch moves this code into a
separate helper and checks for SGRAM, which is the special case. The
test itself is the same as before.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 37 ++++++++++++++++++++++-----
 drivers/gpu/drm/mgag200/mgag200_reg.h |  2 ++
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 7189c7745baf..e50c682c4702 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -44,18 +44,26 @@ static struct drm_driver mgag200_driver = {
  * DRM device
  */
 
-static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
+static bool mgag200_has_sgram(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
-	int ret, option;
+	u32 option;
+	int ret;
 
-	mdev->flags = mgag200_flags_from_driver_data(flags);
-	mdev->type = mgag200_type_from_driver_data(flags);
+	ret = pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
+	if (drm_WARN(dev, ret, "failed to read PCI config dword: %d\n", ret))
+		return false;
+
+	return !!(option & PCI_MGA_OPTION_HARDPWMSK);
+}
 
-	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
-	mdev->has_sdram = !(option & (1 << 14));
+static int mgag200_regs_init(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->base;
+
+	mdev->has_sdram = !mgag200_has_sgram(mdev);
 
-	/* BAR 0 is the framebuffer, BAR 1 contains registers */
+	/* BAR 1 contains registers */
 	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
 	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
 
@@ -69,6 +77,21 @@ static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 	if (mdev->rmmio == NULL)
 		return -ENOMEM;
 
+	return 0;
+}
+
+static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
+{
+	struct drm_device *dev = &mdev->base;
+	int ret;
+
+	mdev->flags = mgag200_flags_from_driver_data(flags);
+	mdev->type = mgag200_type_from_driver_data(flags);
+
+	ret = mgag200_regs_init(mdev);
+	if (ret)
+		return ret;
+
 	/* stash G200 SE model number for later use */
 	if (IS_G200_SE(mdev)) {
 		mdev->unique_rev_id = RREG32(0x1e24);
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index c3b7bcad52ed..a44c08bf4074 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -282,6 +282,8 @@
 #define PCI_MGA_OPTION2		0x50
 #define PCI_MGA_OPTION3		0x54
 
+#define PCI_MGA_OPTION_HARDPWMSK	BIT(14)
+
 #define RAMDAC_OFFSET		0x3c00
 
 /* TVP3026 direct registers */
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] drm/mgag200: Initialize PCI registers early during device setup

[Thomas Zimmermann]

So far, PCI option registers were initialized as part of modesetting,
which is late in the process. As these registers control fundamental
operation, they should be set early.

The patch moves the PCI option handling into device register setup,
before even the device MMIO memory is being mapped. No functional
changes made.

Moving the PCI code next to the device-register setup also allows to
remove the has_sdram field from struct mga_device. The state is now
local to the init helper.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 32 +++++++++++++++++++-
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
 drivers/gpu/drm/mgag200/mgag200_mode.c | 41 --------------------------
 3 files changed, 31 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index e50c682c4702..3dbb00045c24 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -60,8 +60,38 @@ static bool mgag200_has_sgram(struct mga_device *mdev)
 static int mgag200_regs_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
+	u32 option, option2;
+
+	switch (mdev->type) {
+	case G200_SE_A:
+	case G200_SE_B:
+		if (mgag200_has_sgram(mdev))
+			option |= PCI_MGA_OPTION_HARDPWMSK;
+		option2 = 0x00008000;
+		break;
+	case G200_WB:
+	case G200_EW3:
+		option = 0x41049120;
+		option2 = 0x0000b000;
+		break;
+	case G200_EV:
+		option = 0x00000120;
+		option2 = 0x0000b000;
+		break;
+	case G200_EH:
+	case G200_EH3:
+		option = 0x00000120;
+		option2 = 0x0000b000;
+		break;
+	default:
+		option = 0;
+		option2 = 0;
+	}
 
-	mdev->has_sdram = !mgag200_has_sgram(mdev);
+	if (option)
+		pci_write_config_dword(dev->pdev, PCI_MGA_OPTION, option);
+	if (option2)
+		pci_write_config_dword(dev->pdev, PCI_MGA_OPTION2, option2);
 
 	/* BAR 1 contains registers */
 	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 3817520bfefc..819c03cc626b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -161,7 +161,6 @@ struct mga_device {
 	size_t				vram_fb_available;
 
 	enum mga_type			type;
-	int				has_sdram;
 
 	int bpp_shifts[4];
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e0d037a7413c..3aa078e69a5a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -9,7 +9,6 @@
  */
 
 #include <linux/delay.h>
-#include <linux/pci.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic_state_helper.h>
@@ -877,45 +876,6 @@ static void mgag200_set_startadd(struct mga_device *mdev,
 	WREG_ECRT(0x00, crtcext0);
 }
 
-static void mgag200_set_pci_regs(struct mga_device *mdev)
-{
-	uint32_t option = 0, option2 = 0;
-	struct drm_device *dev = &mdev->base;
-
-	switch (mdev->type) {
-	case G200_SE_A:
-	case G200_SE_B:
-		if (mdev->has_sdram)
-			option = 0x40049120;
-		else
-			option = 0x4004d120;
-		option2 = 0x00008000;
-		break;
-	case G200_WB:
-	case G200_EW3:
-		option = 0x41049120;
-		option2 = 0x0000b000;
-		break;
-	case G200_EV:
-		option = 0x00000120;
-		option2 = 0x0000b000;
-		break;
-	case G200_EH:
-	case G200_EH3:
-		option = 0x00000120;
-		option2 = 0x0000b000;
-		break;
-	case G200_ER:
-		break;
-	}
-
-	if (option)
-		pci_write_config_dword(dev->pdev, PCI_MGA_OPTION, option);
-
-	if (option2)
-		pci_write_config_dword(dev->pdev, PCI_MGA_OPTION2, option2);
-}
-
 static void mgag200_set_dac_regs(struct mga_device *mdev)
 {
 	size_t i;
@@ -988,7 +948,6 @@ static void mgag200_init_regs(struct mga_device *mdev)
 {
 	u8 crtc11, crtcext3, crtcext4, misc;
 
-	mgag200_set_pci_regs(mdev);
 	mgag200_set_dac_regs(mdev);
 
 	WREG_SEQ(2, 0x0f);
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm/mgag200: Enable MGA mode during device register initialization

[Thomas Zimmermann]

MGA cards can run in traditional VGA mode or an enhanced MGA mode; with
the latter being required for KMS. So far, MGA mode was enabled during
modesetting. As it's fundamental for device operation, the patch moves
it next to the device register setup.

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 3dbb00045c24..ac9ac5b6d587 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -61,6 +61,7 @@ static int mgag200_regs_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
 	u32 option, option2;
+	u8 crtcext3;
 
 	switch (mdev->type) {
 	case G200_SE_A:
@@ -107,6 +108,10 @@ static int mgag200_regs_init(struct mga_device *mdev)
 	if (mdev->rmmio == NULL)
 		return -ENOMEM;
 
+	RREG_ECRT(0x03, crtcext3);
+	crtcext3 |= MGAREG_CRTCEXT3_MGAMODE;
+	WREG_ECRT(0x03, crtcext3);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 3aa078e69a5a..7161b1651aa0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -946,7 +946,7 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
 
 static void mgag200_init_regs(struct mga_device *mdev)
 {
-	u8 crtc11, crtcext3, crtcext4, misc;
+	u8 crtc11, crtcext4, misc;
 
 	mgag200_set_dac_regs(mdev);
 
@@ -961,12 +961,8 @@ static void mgag200_init_regs(struct mga_device *mdev)
 	WREG_CRT(14, 0);
 	WREG_CRT(15, 0);
 
-	RREG_ECRT(0x03, crtcext3);
-
-	crtcext3 |= BIT(7); /* enable MGA mode */
 	crtcext4 = 0x00;
 
-	WREG_ECRT(0x03, crtcext3);
 	WREG_ECRT(0x04, crtcext4);
 
 	RREG_CRT(0x11, crtc11);
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index a44c08bf4074..977be0565c06 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -256,6 +256,8 @@
 #define MGAREG_CRTCEXT1_VSYNCOFF	BIT(5)
 #define MGAREG_CRTCEXT1_HSYNCOFF	BIT(4)
 
+#define MGAREG_CRTCEXT3_MGAMODE		BIT(7)
+
 /* Cursor X and Y position */
 #define MGA_CURPOSXL 0x3c0c
 #define MGA_CURPOSXH 0x3c0d
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm/mgag200: Set MISC memory flags in mm init code

[Thomas Zimmermann]

The modesetting code initialized several memory-related flags in the
MISC register. Move this code to MM initialization.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mm.c   | 6 ++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index 7b69392bcb89..1b1918839e1e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -90,9 +90,15 @@ static void mgag200_mm_release(struct drm_device *dev, void *ptr)
 int mgag200_mm_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
+	u8 misc;
 	resource_size_t start, len;
 	int ret;
 
+	misc = RREG8(MGA_MISC_IN);
+	misc |= MGAREG_MISC_RAMMAPEN |
+		MGAREG_MISC_HIGH_PG_SEL;
+	WREG8(MGA_MISC_OUT, misc);
+
 	/* BAR 0 is VRAM */
 	start = pci_resource_start(dev->pdev, 0);
 	len = pci_resource_len(dev->pdev, 0);
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 7161b1651aa0..66818ee10694 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -978,9 +978,7 @@ static void mgag200_init_regs(struct mga_device *mdev)
 		WREG_ECRT(0x34, 0x5);
 
 	misc = RREG8(MGA_MISC_IN);
-	misc |= MGAREG_MISC_IOADSEL |
-		MGAREG_MISC_RAMMAPEN |
-		MGAREG_MISC_HIGH_PG_SEL;
+	misc |= MGAREG_MISC_IOADSEL;
 	WREG8(MGA_MISC_OUT, misc);
 }
 
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/8] drm/mgag200: Clear <page> field during MM init

[Thomas Zimmermann]

The modesetting code initialized the memory-related register CRTCEXT4.
Move this code to MM initialization.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mm.c   | 2 ++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +-----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index 1b1918839e1e..641f1aa992be 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -94,6 +94,8 @@ int mgag200_mm_init(struct mga_device *mdev)
 	resource_size_t start, len;
 	int ret;
 
+	WREG_ECRT(0x04, 0x00);
+
 	misc = RREG8(MGA_MISC_IN);
 	misc |= MGAREG_MISC_RAMMAPEN |
 		MGAREG_MISC_HIGH_PG_SEL;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 66818ee10694..4fa64cf884cb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -946,7 +946,7 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
 
 static void mgag200_init_regs(struct mga_device *mdev)
 {
-	u8 crtc11, crtcext4, misc;
+	u8 crtc11, misc;
 
 	mgag200_set_dac_regs(mdev);
 
@@ -961,10 +961,6 @@ static void mgag200_init_regs(struct mga_device *mdev)
 	WREG_CRT(14, 0);
 	WREG_CRT(15, 0);
 
-	crtcext4 = 0x00;
-
-	WREG_ECRT(0x04, crtcext4);
-
 	RREG_CRT(0x11, crtc11);
 	crtc11 &= ~(MGAREG_CRTC11_CRTCPROTECT |
 		    MGAREG_CRTC11_VINTEN |
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm/mgag200: Move G200SE's unique id into model-specific data

[Thomas Zimmermann]

The unique revision id is only useful for G200SE devices. Store the
value in model-specific data within struct mga_device. While at it,
the patch also adds an init helper for the value.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 19 +++++++++++++------
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  8 ++++++--
 drivers/gpu/drm/mgag200/mgag200_mode.c | 18 +++++++++++-------
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index ac9ac5b6d587..f7652e16365c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -115,6 +115,17 @@ static int mgag200_regs_init(struct mga_device *mdev)
 	return 0;
 }
 
+static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->base;
+
+	/* stash G200 SE model number for later use */
+	mdev->model.g200se.unique_rev_id = RREG32(0x1e24);
+
+	drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
+		mdev->model.g200se.unique_rev_id);
+}
+
 static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 {
 	struct drm_device *dev = &mdev->base;
@@ -127,12 +138,8 @@ static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 	if (ret)
 		return ret;
 
-	/* stash G200 SE model number for later use */
-	if (IS_G200_SE(mdev)) {
-		mdev->unique_rev_id = RREG32(0x1e24);
-		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
-			mdev->unique_rev_id);
-	}
+	if (IS_G200_SE(mdev))
+		mgag200_g200se_init_unique_id(mdev);
 
 	ret = mgag200_mm_init(mdev);
 	if (ret)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 819c03cc626b..048efe635aff 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -166,8 +166,12 @@ struct mga_device {
 
 	int fb_mtrr;
 
-	/* SE model number stored in reg 0x1e24 */
-	u32 unique_rev_id;
+	union {
+		struct {
+			/* SE model number stored in reg 0x1e24 */
+			u32 unique_rev_id;
+		} g200se;
+	} model;
 
 	struct mga_connector connector;
 	struct drm_simple_display_pipe display_pipe;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 4fa64cf884cb..752409c7f326 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -112,6 +112,7 @@ static inline void mga_wait_busy(struct mga_device *mdev)
 
 static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 {
+	u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
 	unsigned int vcomax, vcomin, pllreffreq;
 	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
@@ -121,7 +122,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 	unsigned int fvv;
 	unsigned int i;
 
-	if (mdev->unique_rev_id <= 0x03) {
+	if (unique_rev_id <= 0x03) {
 
 		m = n = p = 0;
 		vcomax = 320000;
@@ -219,7 +220,7 @@ static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
 	WREG_DAC(MGA1064_PIX_PLLC_N, n);
 	WREG_DAC(MGA1064_PIX_PLLC_P, p);
 
-	if (mdev->unique_rev_id >= 0x04) {
+	if (unique_rev_id >= 0x04) {
 		WREG_DAC(0x1a, 0x09);
 		msleep(20);
 		WREG_DAC(0x1a, 0x01);
@@ -1183,12 +1184,13 @@ static void mgag200_g200se_set_hiprilvl(struct mga_device *mdev,
 					const struct drm_display_mode *mode,
 					const struct drm_framebuffer *fb)
 {
+	u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
 	unsigned int hiprilvl;
 	u8 crtcext6;
 
-	if  (mdev->unique_rev_id >= 0x04) {
+	if  (unique_rev_id >= 0x04) {
 		hiprilvl = 0;
-	} else if (mdev->unique_rev_id >= 0x02) {
+	} else if (unique_rev_id >= 0x02) {
 		unsigned int bpp;
 		unsigned long mb;
 
@@ -1213,7 +1215,7 @@ static void mgag200_g200se_set_hiprilvl(struct mga_device *mdev,
 		else
 			hiprilvl = 5;
 
-	} else if (mdev->unique_rev_id >= 0x01) {
+	} else if (unique_rev_id >= 0x01) {
 		hiprilvl = 3;
 	} else {
 		hiprilvl = 4;
@@ -1337,7 +1339,9 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 	int bpp = 32;
 
 	if (IS_G200_SE(mdev)) {
-		if (mdev->unique_rev_id == 0x01) {
+		u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
+
+		if (unique_rev_id == 0x01) {
 			if (mode->hdisplay > 1600)
 				return MODE_VIRTUAL_X;
 			if (mode->vdisplay > 1200)
@@ -1345,7 +1349,7 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
 				> (24400 * 1024))
 				return MODE_BANDWIDTH;
-		} else if (mdev->unique_rev_id == 0x02) {
+		} else if (unique_rev_id == 0x02) {
 			if (mode->hdisplay > 1920)
 				return MODE_VIRTUAL_X;
 			if (mode->vdisplay > 1200)
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Thomas Zimmermann]

This patch adds support for G200 desktop cards. We can reuse the whole
memory and modesetting code. A few PCI and DAC register values have to
be updated accordingly.

The most significant change is in the PLL setup. The get the clock limits
and reference clocks, parses the device's BIOS. With no BIOS found, safe
defaults are being used.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Co-developed-by: Egbert Eich <eich@suse.com>
Signed-off-by: Egbert Eich <eich@suse.com>
Co-developed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 MAINTAINERS                            |   2 +-
 drivers/gpu/drm/mgag200/Kconfig        |  12 +--
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
 drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
 5 files changed, 220 insertions(+), 9 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 415954a98934..4c6f96e2b79b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
 F:	drivers/gpu/drm/mga/
 F:	include/uapi/drm/mga_drm.h
 
-DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
+DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
 M:	Dave Airlie <airlied@redhat.com>
 S:	Odd Fixes
 F:	drivers/gpu/drm/mgag200/
diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
index 93be766715c9..eec59658a938 100644
--- a/drivers/gpu/drm/mgag200/Kconfig
+++ b/drivers/gpu/drm/mgag200/Kconfig
@@ -1,13 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config DRM_MGAG200
-	tristate "Kernel modesetting driver for MGA G200 server engines"
+	tristate "Matrox G200"
 	depends on DRM && PCI && MMU
 	select DRM_GEM_SHMEM_HELPER
 	select DRM_KMS_HELPER
 	help
-	 This is a KMS driver for the MGA G200 server chips, it
-	 does not support the original MGA G200 or any of the desktop
-	 chips. It requires 0.3.0 of the modesetting userspace driver,
-	 and a version of mga driver that will fail on KMS enabled
-	 devices.
-
+	 This is a KMS driver for Matrox G200 chips. It supports the original
+	 MGA G200 desktop chips and the server variants. It requires 0.3.0
+	 of the modesetting userspace driver, and a version of mga driver
+	 that will fail on KMS enabled devices.
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index f7652e16365c..419817d6e2cd 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
 	u8 crtcext3;
 
 	switch (mdev->type) {
+	case G200_PCI:
+	case G200_AGP:
+		if (mgag200_has_sgram(mdev))
+			option = 0x4049cd21;
+		else
+			option = 0x40499121;
+		option2 = 0x00008000;
+		break;
 	case G200_SE_A:
 	case G200_SE_B:
 		if (mgag200_has_sgram(mdev))
@@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
 	return 0;
 }
 
+static void mgag200_g200_interpret_bios(struct mga_device *mdev,
+					unsigned char __iomem *bios,
+					size_t size)
+{
+	static const unsigned int expected_length[6] = {
+		0, 64, 64, 64, 128, 128
+	};
+
+	struct drm_device *dev = &mdev->base;
+	unsigned char __iomem *pins;
+	unsigned int pins_len, version;
+	int offset;
+	int tmp;
+
+	if (size < MGA_BIOS_OFFSET + 1)
+		return;
+
+	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
+	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
+		return;
+
+	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
+
+	if (offset + 5 > size)
+		return;
+
+	pins = bios + offset;
+	if (pins[0] == 0x2e && pins[1] == 0x41) {
+		version = pins[5];
+		pins_len = pins[2];
+	} else {
+		version = 1;
+		pins_len = pins[0] + (pins[1] << 8);
+	}
+
+	if (version < 1 || version > 5) {
+		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
+		return;
+	}
+	if (pins_len != expected_length[version]) {
+		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
+			 pins_len, expected_length[version]);
+		return;
+	}
+
+	if (offset + pins_len > size)
+		return;
+
+	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
+		    version, pins_len);
+
+	switch (version) {
+	case 1:
+		tmp = pins[24] + (pins[25] << 8);
+		if (tmp)
+			mdev->model.g200.pclk_max = tmp * 10;
+		break;
+	case 2:
+		if (pins[41] != 0xff)
+			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
+		break;
+	case 3:
+		if (pins[36] != 0xff)
+			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
+		if (pins[52] & 0x20)
+			mdev->model.g200.ref_clk = 14318;
+		break;
+	case 4:
+		if (pins[39] != 0xff)
+			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
+		if (pins[92] & 0x01)
+			mdev->model.g200.ref_clk = 14318;
+		break;
+	case 5:
+		tmp = pins[4] ? 8000 : 6000;
+		if (pins[123] != 0xff)
+			mdev->model.g200.pclk_min = pins[123] * tmp;
+		if (pins[38] != 0xff)
+			mdev->model.g200.pclk_max = pins[38] * tmp;
+		if (pins[110] & 0x01)
+			mdev->model.g200.ref_clk = 14318;
+		break;
+	default:
+		break;
+	}
+}
+
+static void mgag200_g200_init_refclk(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->base;
+	unsigned char __iomem *bios;
+	size_t size;
+
+	mdev->model.g200.pclk_min = 50000;
+	mdev->model.g200.pclk_max = 230000;
+	mdev->model.g200.ref_clk = 27050;
+
+	bios = pci_map_rom(dev->pdev, &size);
+	if (!bios)
+		return;
+
+	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
+		mgag200_g200_interpret_bios(mdev, bios, size);
+
+	pci_unmap_rom(dev->pdev, bios);
+
+	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
+		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
+		    mdev->model.g200.ref_clk);
+}
+
 static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
@@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 	if (ret)
 		return ret;
 
-	if (IS_G200_SE(mdev))
+	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
+		mgag200_g200_init_refclk(mdev);
+	else if (IS_G200_SE(mdev))
 		mgag200_g200se_init_unique_id(mdev);
 
 	ret = mgag200_mm_init(mdev);
@@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned long flags)
  */
 
 static const struct pci_device_id mgag200_pciidlist[] = {
+	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI },
+	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP },
 	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
 	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B },
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 048efe635aff..54061a61e9ca 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -38,6 +38,8 @@
 #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
 #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) + (reg))
 
+#define MGA_BIOS_OFFSET 0x7ffc
+
 #define ATTR_INDEX 0x1fc0
 #define ATTR_DATA 0x1fc1
 
@@ -129,6 +131,8 @@ struct mga_mc {
 };
 
 enum mga_type {
+	G200_PCI,
+	G200_AGP,
 	G200_SE_A,
 	G200_SE_B,
 	G200_WB,
@@ -167,12 +171,18 @@ struct mga_device {
 	int fb_mtrr;
 
 	union {
+		struct {
+			long ref_clk;
+			long pclk_min;
+			long pclk_max;
+		} g200;
 		struct {
 			/* SE model number stored in reg 0x1e24 */
 			u32 unique_rev_id;
 		} g200se;
 	} model;
 
+
 	struct mga_connector connector;
 	struct drm_simple_display_pipe display_pipe;
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 752409c7f326..bc11552415f5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device *mdev)
 	} while ((status & 0x01) && time_before(jiffies, timeout));
 }
 
+/*
+ * PLL setup
+ */
+
+static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
+{
+	struct drm_device *dev = &mdev->base;
+	const int post_div_max = 7;
+	const int in_div_min = 1;
+	const int in_div_max = 6;
+	const int feed_div_min = 7;
+	const int feed_div_max = 127;
+	u8 testm, testn;
+	u8 n = 0, m = 0, p, s;
+	long f_vco;
+	long computed;
+	long delta, tmp_delta;
+	long ref_clk = mdev->model.g200.ref_clk;
+	long p_clk_min = mdev->model.g200.pclk_min;
+	long p_clk_max =  mdev->model.g200.pclk_max;
+
+	if (clock > p_clk_max) {
+		drm_err(dev, "Pixel Clock %ld too high\n", clock);
+		return 1;
+	}
+
+	if (clock <  p_clk_min >> 3)
+		clock = p_clk_min >> 3;
+
+	f_vco = clock;
+	for (p = 0;
+	     p <= post_div_max && f_vco < p_clk_min;
+	     p = (p << 1) + 1, f_vco <<= 1)
+		;
+
+	delta = clock;
+
+	for (testm = in_div_min; testm <= in_div_max; testm++) {
+		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
+			computed = ref_clk * (testn + 1) / (testm + 1);
+			if (computed < f_vco)
+				tmp_delta = f_vco - computed;
+			else
+				tmp_delta  = computed - f_vco;
+			if (tmp_delta < delta) {
+				delta = tmp_delta;
+				m = testm;
+				n = testn;
+			}
+		}
+	}
+	f_vco = ref_clk * (n + 1) / (m + 1);
+	if (f_vco < 100000)
+		s = 0;
+	else if (f_vco < 140000)
+		s = 1;
+	else if (f_vco < 180000)
+		s = 2;
+	else
+		s = 3;
+
+	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
+		    clock, f_vco, m, n, p, s);
+
+	WREG_DAC(MGA1064_PIX_PLLC_M, m);
+	WREG_DAC(MGA1064_PIX_PLLC_N, n);
+	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
+
+	return 0;
+}
+
 #define P_ARRAY_SIZE 9
 
 static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
@@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev, long clock)
 	u8 misc;
 
 	switch(mdev->type) {
+	case G200_PCI:
+	case G200_AGP:
+		return mgag200_g200_set_plls(mdev, clock);
 	case G200_SE_A:
 	case G200_SE_B:
 		return mga_g200se_set_plls(mdev, clock);
@@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
 	};
 
 	switch (mdev->type) {
+	case G200_PCI:
+	case G200_AGP:
+		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
+		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
+		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
+		break;
 	case G200_SE_A:
 	case G200_SE_B:
 		dacvalue[MGA1064_VREF_CTL] = 0x03;
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7240 bytes --]

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on next-20200715]
[cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master drm/drm-next v5.8-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/drm-mgag200-Support-desktop-chips/20200715-230422
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-s002-20200715 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-49-g707c5017-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/mgag200/mgag200_drv.c:143:17: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:143:36: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:143:55: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:144:17: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:144:36: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:144:55: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:147:23: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:147:57: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:153:17: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:153:36: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:154:31: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:155:32: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:158:32: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:158:43: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:179:27: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:179:39: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:184:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:185:58: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:188:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:189:58: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:190:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:194:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:195:57: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:196:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:200:27: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:201:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:202:57: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:203:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:204:57: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:205:25: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:227:30: sparse: sparse: dereference of noderef expression
   drivers/gpu/drm/mgag200/mgag200_drv.c:227:49: sparse: sparse: dereference of noderef expression

vim +143 drivers/gpu/drm/mgag200/mgag200_drv.c

   125	
   126	static void mgag200_g200_interpret_bios(struct mga_device *mdev,
   127						unsigned char __iomem *bios,
   128						size_t size)
   129	{
   130		static const unsigned int expected_length[6] = {
   131			0, 64, 64, 64, 128, 128
   132		};
   133	
   134		struct drm_device *dev = &mdev->base;
   135		unsigned char __iomem *pins;
   136		unsigned int pins_len, version;
   137		int offset;
   138		int tmp;
   139	
   140		if (size < MGA_BIOS_OFFSET + 1)
   141			return;
   142	
 > 143		if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
   144		    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
   145			return;
   146	
   147		offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
   148	
   149		if (offset + 5 > size)
   150			return;
   151	
   152		pins = bios + offset;
   153		if (pins[0] == 0x2e && pins[1] == 0x41) {
   154			version = pins[5];
   155			pins_len = pins[2];
   156		} else {
   157			version = 1;
   158			pins_len = pins[0] + (pins[1] << 8);
   159		}
   160	
   161		if (version < 1 || version > 5) {
   162			drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
   163			return;
   164		}
   165		if (pins_len != expected_length[version]) {
   166			drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
   167				 pins_len, expected_length[version]);
   168			return;
   169		}
   170	
   171		if (offset + pins_len > size)
   172			return;
   173	
   174		drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
   175			    version, pins_len);
   176	
   177		switch (version) {
   178		case 1:
   179			tmp = pins[24] + (pins[25] << 8);
   180			if (tmp)
   181				mdev->model.g200.pclk_max = tmp * 10;
   182			break;
   183		case 2:
   184			if (pins[41] != 0xff)
   185				mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
   186			break;
   187		case 3:
   188			if (pins[36] != 0xff)
   189				mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
   190			if (pins[52] & 0x20)
   191				mdev->model.g200.ref_clk = 14318;
   192			break;
   193		case 4:
   194			if (pins[39] != 0xff)
   195				mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
   196			if (pins[92] & 0x01)
   197				mdev->model.g200.ref_clk = 14318;
   198			break;
   199		case 5:
   200			tmp = pins[4] ? 8000 : 6000;
   201			if (pins[123] != 0xff)
   202				mdev->model.g200.pclk_min = pins[123] * tmp;
   203			if (pins[38] != 0xff)
   204				mdev->model.g200.pclk_max = pins[38] * tmp;
   205			if (pins[110] & 0x01)
   206				mdev->model.g200.ref_clk = 14318;
   207			break;
   208		default:
   209			break;
   210		}
   211	}
   212	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35942 bytes --]

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Lyude Paul]

On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> This patch adds support for G200 desktop cards. We can reuse the whole
> memory and modesetting code. A few PCI and DAC register values have to
> be updated accordingly.
> 
> The most significant change is in the PLL setup. The get the clock limits
> and reference clocks, parses the device's BIOS. With no BIOS found, safe
> defaults are being used.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Co-developed-by: Egbert Eich <eich@suse.com>
> Signed-off-by: Egbert Eich <eich@suse.com>
> Co-developed-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  MAINTAINERS                            |   2 +-
>  drivers/gpu/drm/mgag200/Kconfig        |  12 +--
>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
>  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
>  5 files changed, 220 insertions(+), 9 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 415954a98934..4c6f96e2b79b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
>  F:	drivers/gpu/drm/mga/
>  F:	include/uapi/drm/mga_drm.h
>  
> -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
>  M:	Dave Airlie <airlied@redhat.com>
>  S:	Odd Fixes
>  F:	drivers/gpu/drm/mgag200/
> diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
> index 93be766715c9..eec59658a938 100644
> --- a/drivers/gpu/drm/mgag200/Kconfig
> +++ b/drivers/gpu/drm/mgag200/Kconfig
> @@ -1,13 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config DRM_MGAG200
> -	tristate "Kernel modesetting driver for MGA G200 server engines"
> +	tristate "Matrox G200"
>  	depends on DRM && PCI && MMU
>  	select DRM_GEM_SHMEM_HELPER
>  	select DRM_KMS_HELPER
>  	help
> -	 This is a KMS driver for the MGA G200 server chips, it
> -	 does not support the original MGA G200 or any of the desktop
> -	 chips. It requires 0.3.0 of the modesetting userspace driver,
> -	 and a version of mga driver that will fail on KMS enabled
> -	 devices.
> -
> +	 This is a KMS driver for Matrox G200 chips. It supports the original
> +	 MGA G200 desktop chips and the server variants. It requires 0.3.0
> +	 of the modesetting userspace driver, and a version of mga driver
> +	 that will fail on KMS enabled devices.
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index f7652e16365c..419817d6e2cd 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
>  	u8 crtcext3;
>  
>  	switch (mdev->type) {
> +	case G200_PCI:
> +	case G200_AGP:
> +		if (mgag200_has_sgram(mdev))
> +			option = 0x4049cd21;
> +		else
> +			option = 0x40499121;
> +		option2 = 0x00008000;
> +		break;
>  	case G200_SE_A:
>  	case G200_SE_B:
>  		if (mgag200_has_sgram(mdev))
> @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
>  	return 0;
>  }
>  
> +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> +					unsigned char __iomem *bios,
> +					size_t size)
> +{
> +	static const unsigned int expected_length[6] = {
> +		0, 64, 64, 64, 128, 128
> +	};
> +
> +	struct drm_device *dev = &mdev->base;
> +	unsigned char __iomem *pins;

huh, never realized you could write directly to unsigned char __iomem pointers!

> +	unsigned int pins_len, version;
> +	int offset;
> +	int tmp;
> +
> +	if (size < MGA_BIOS_OFFSET + 1)
> +		return;
> +
> +	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> +	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> +		return;
> +
> +	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> +
> +	if (offset + 5 > size)
> +		return;
> +
> +	pins = bios + offset;
> +	if (pins[0] == 0x2e && pins[1] == 0x41) {
> +		version = pins[5];
> +		pins_len = pins[2];
> +	} else {
> +		version = 1;
> +		pins_len = pins[0] + (pins[1] << 8);
> +	}
> +
> +	if (version < 1 || version > 5) {
> +		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);

Did you maybe mean pins or PINS here? or is PInS some weird abbreviation matrox
uses?

> +		return;
> +	}
> +	if (pins_len != expected_length[version]) {
> +		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
> +			 pins_len, expected_length[version]);
> +		return;
> +	}
> +
> +	if (offset + pins_len > size)
> +		return;
> +
> +	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
> +		    version, pins_len);
> +
> +	switch (version) {
> +	case 1:
> +		tmp = pins[24] + (pins[25] << 8);
> +		if (tmp)
> +			mdev->model.g200.pclk_max = tmp * 10;
> +		break;
> +	case 2:
> +		if (pins[41] != 0xff)
> +			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
> +		break;
> +	case 3:
> +		if (pins[36] != 0xff)
> +			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
> +		if (pins[52] & 0x20)
> +			mdev->model.g200.ref_clk = 14318;
> +		break;
> +	case 4:
> +		if (pins[39] != 0xff)
> +			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
> +		if (pins[92] & 0x01)
> +			mdev->model.g200.ref_clk = 14318;
> +		break;
> +	case 5:
> +		tmp = pins[4] ? 8000 : 6000;
> +		if (pins[123] != 0xff)
> +			mdev->model.g200.pclk_min = pins[123] * tmp;
> +		if (pins[38] != 0xff)
> +			mdev->model.g200.pclk_max = pins[38] * tmp;
> +		if (pins[110] & 0x01)
> +			mdev->model.g200.ref_clk = 14318;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void mgag200_g200_init_refclk(struct mga_device *mdev)
> +{
> +	struct drm_device *dev = &mdev->base;
> +	unsigned char __iomem *bios;
> +	size_t size;
> +
> +	mdev->model.g200.pclk_min = 50000;
> +	mdev->model.g200.pclk_max = 230000;
> +	mdev->model.g200.ref_clk = 27050;
> +
> +	bios = pci_map_rom(dev->pdev, &size);
> +	if (!bios)
> +		return;
> +
> +	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
> +		mgag200_g200_interpret_bios(mdev, bios, size);
> +
> +	pci_unmap_rom(dev->pdev, bios);
> +
> +	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
> +		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
> +		    mdev->model.g200.ref_clk);
> +}
> +
>  static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
>  {
>  	struct drm_device *dev = &mdev->base;
> @@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device *mdev,
> unsigned long flags)
>  	if (ret)
>  		return ret;
>  
> -	if (IS_G200_SE(mdev))
> +	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
> +		mgag200_g200_init_refclk(mdev);
> +	else if (IS_G200_SE(mdev))
>  		mgag200_g200se_init_unique_id(mdev);
>  
>  	ret = mgag200_mm_init(mdev);
> @@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned long
> flags)
>   */
>  
>  static const struct pci_device_id mgag200_pciidlist[] = {
> +	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI },
> +	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP },
>  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
>  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B
> },
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
> b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 048efe635aff..54061a61e9ca 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -38,6 +38,8 @@
>  #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
>  #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) + (reg))
>  
> +#define MGA_BIOS_OFFSET 0x7ffc
> +
>  #define ATTR_INDEX 0x1fc0
>  #define ATTR_DATA 0x1fc1
>  
> @@ -129,6 +131,8 @@ struct mga_mc {
>  };
>  
>  enum mga_type {
> +	G200_PCI,
> +	G200_AGP,
>  	G200_SE_A,
>  	G200_SE_B,
>  	G200_WB,
> @@ -167,12 +171,18 @@ struct mga_device {
>  	int fb_mtrr;
>  
>  	union {
> +		struct {
> +			long ref_clk;
> +			long pclk_min;
> +			long pclk_max;
> +		} g200;
>  		struct {
>  			/* SE model number stored in reg 0x1e24 */
>  			u32 unique_rev_id;
>  		} g200se;
>  	} model;
>  
> +
>  	struct mga_connector connector;
>  	struct drm_simple_display_pipe display_pipe;
>  };
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 752409c7f326..bc11552415f5 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device *mdev)
>  	} while ((status & 0x01) && time_before(jiffies, timeout));
>  }
>  
> +/*
> + * PLL setup
> + */
> +
> +static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
> +{
> +	struct drm_device *dev = &mdev->base;
> +	const int post_div_max = 7;
> +	const int in_div_min = 1;
> +	const int in_div_max = 6;
> +	const int feed_div_min = 7;
> +	const int feed_div_max = 127;
> +	u8 testm, testn;
> +	u8 n = 0, m = 0, p, s;
> +	long f_vco;
> +	long computed;
> +	long delta, tmp_delta;
> +	long ref_clk = mdev->model.g200.ref_clk;
> +	long p_clk_min = mdev->model.g200.pclk_min;
> +	long p_clk_max =  mdev->model.g200.pclk_max;
> +
> +	if (clock > p_clk_max) {
> +		drm_err(dev, "Pixel Clock %ld too high\n", clock);
> +		return 1;
> +	}
> +
> +	if (clock <  p_clk_min >> 3)

Looks like there's a stray space after the <. You could also just use max()
here, but I'll leave that up to you

> +		clock = p_clk_min >> 3;
> +
> +	f_vco = clock;
> +	for (p = 0;
> +	     p <= post_div_max && f_vco < p_clk_min;
> +	     p = (p << 1) + 1, f_vco <<= 1)
> +		;
> +
> +	delta = clock;
> +
> +	for (testm = in_div_min; testm <= in_div_max; testm++) {
> +		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
> +			computed = ref_clk * (testn + 1) / (testm + 1);
> +			if (computed < f_vco)
> +				tmp_delta = f_vco - computed;
> +			else
> +				tmp_delta  = computed - f_vco;

Another stray space before the =

With those nitpicks addressed, this series is:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> +			if (tmp_delta < delta) {
> +				delta = tmp_delta;
> +				m = testm;
> +				n = testn;
> +			}
> +		}
> +	}
> +	f_vco = ref_clk * (n + 1) / (m + 1);
> +	if (f_vco < 100000)
> +		s = 0;
> +	else if (f_vco < 140000)
> +		s = 1;
> +	else if (f_vco < 180000)
> +		s = 2;
> +	else
> +		s = 3;
> +
> +	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
> +		    clock, f_vco, m, n, p, s);
> +
> +	WREG_DAC(MGA1064_PIX_PLLC_M, m);
> +	WREG_DAC(MGA1064_PIX_PLLC_N, n);
> +	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
> +
> +	return 0;
> +}
> +
>  #define P_ARRAY_SIZE 9
>  
>  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
> @@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev,
> long clock)
>  	u8 misc;
>  
>  	switch(mdev->type) {
> +	case G200_PCI:
> +	case G200_AGP:
> +		return mgag200_g200_set_plls(mdev, clock);
>  	case G200_SE_A:
>  	case G200_SE_B:
>  		return mga_g200se_set_plls(mdev, clock);
> @@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
>  	};
>  
>  	switch (mdev->type) {
> +	case G200_PCI:
> +	case G200_AGP:
> +		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
> +		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
> +		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
> +		break;
>  	case G200_SE_A:
>  	case G200_SE_B:
>  		dacvalue[MGA1064_VREF_CTL] = 0x03;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Sam Ravnborg]

Hi Lyude.

On Thu, Jul 16, 2020 at 06:43:40PM -0400, Lyude Paul wrote:
> On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> > This patch adds support for G200 desktop cards. We can reuse the whole
> > memory and modesetting code. A few PCI and DAC register values have to
> > be updated accordingly.
> > 
> > The most significant change is in the PLL setup. The get the clock limits
> > and reference clocks, parses the device's BIOS. With no BIOS found, safe
> > defaults are being used.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Co-developed-by: Egbert Eich <eich@suse.com>
> > Signed-off-by: Egbert Eich <eich@suse.com>
> > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  MAINTAINERS                            |   2 +-
> >  drivers/gpu/drm/mgag200/Kconfig        |  12 +--
> >  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
> >  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
> >  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
> >  5 files changed, 220 insertions(+), 9 deletions(-)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 415954a98934..4c6f96e2b79b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
> >  F:	drivers/gpu/drm/mga/
> >  F:	include/uapi/drm/mga_drm.h
> >  
> > -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> > +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
> >  M:	Dave Airlie <airlied@redhat.com>
> >  S:	Odd Fixes
> >  F:	drivers/gpu/drm/mgag200/
> > diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
> > index 93be766715c9..eec59658a938 100644
> > --- a/drivers/gpu/drm/mgag200/Kconfig
> > +++ b/drivers/gpu/drm/mgag200/Kconfig
> > @@ -1,13 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  config DRM_MGAG200
> > -	tristate "Kernel modesetting driver for MGA G200 server engines"
> > +	tristate "Matrox G200"
> >  	depends on DRM && PCI && MMU
> >  	select DRM_GEM_SHMEM_HELPER
> >  	select DRM_KMS_HELPER
> >  	help
> > -	 This is a KMS driver for the MGA G200 server chips, it
> > -	 does not support the original MGA G200 or any of the desktop
> > -	 chips. It requires 0.3.0 of the modesetting userspace driver,
> > -	 and a version of mga driver that will fail on KMS enabled
> > -	 devices.
> > -
> > +	 This is a KMS driver for Matrox G200 chips. It supports the original
> > +	 MGA G200 desktop chips and the server variants. It requires 0.3.0
> > +	 of the modesetting userspace driver, and a version of mga driver
> > +	 that will fail on KMS enabled devices.
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > index f7652e16365c..419817d6e2cd 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
> >  	u8 crtcext3;
> >  
> >  	switch (mdev->type) {
> > +	case G200_PCI:
> > +	case G200_AGP:
> > +		if (mgag200_has_sgram(mdev))
> > +			option = 0x4049cd21;
> > +		else
> > +			option = 0x40499121;
> > +		option2 = 0x00008000;
> > +		break;
> >  	case G200_SE_A:
> >  	case G200_SE_B:
> >  		if (mgag200_has_sgram(mdev))
> > @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
> >  	return 0;
> >  }
> >  
> > +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> > +					unsigned char __iomem *bios,
> > +					size_t size)
> > +{
> > +	static const unsigned int expected_length[6] = {
> > +		0, 64, 64, 64, 128, 128
> > +	};
> > +
> > +	struct drm_device *dev = &mdev->base;
> > +	unsigned char __iomem *pins;
> 
> huh, never realized you could write directly to unsigned char __iomem pointers!
You cannot :-)

It works for some architectures but not for all.
On sparc64, for instance, this will fail.
The right thing is to use the accessors in io.h

sparse will help you finding such illegal uses of __iomem *.
The good thing is that the pointers are annotated __iomem here.

Thomas:
Please run sparse and fix the warnings using the right accessors
for this patch.

	Sam

> 
> > +	unsigned int pins_len, version;
> > +	int offset;
> > +	int tmp;
> > +
> > +	if (size < MGA_BIOS_OFFSET + 1)
> > +		return;
> > +
> > +	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> > +	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> > +		return;
> > +
> > +	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> > +
> > +	if (offset + 5 > size)
> > +		return;
> > +
> > +	pins = bios + offset;
> > +	if (pins[0] == 0x2e && pins[1] == 0x41) {
> > +		version = pins[5];
> > +		pins_len = pins[2];
> > +	} else {
> > +		version = 1;
> > +		pins_len = pins[0] + (pins[1] << 8);
> > +	}
> > +
> > +	if (version < 1 || version > 5) {
> > +		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
> 
> Did you maybe mean pins or PINS here? or is PInS some weird abbreviation matrox
> uses?
> 
> > +		return;
> > +	}
> > +	if (pins_len != expected_length[version]) {
> > +		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
> > +			 pins_len, expected_length[version]);
> > +		return;
> > +	}
> > +
> > +	if (offset + pins_len > size)
> > +		return;
> > +
> > +	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
> > +		    version, pins_len);
> > +
> > +	switch (version) {
> > +	case 1:
> > +		tmp = pins[24] + (pins[25] << 8);
> > +		if (tmp)
> > +			mdev->model.g200.pclk_max = tmp * 10;
> > +		break;
> > +	case 2:
> > +		if (pins[41] != 0xff)
> > +			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
> > +		break;
> > +	case 3:
> > +		if (pins[36] != 0xff)
> > +			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
> > +		if (pins[52] & 0x20)
> > +			mdev->model.g200.ref_clk = 14318;
> > +		break;
> > +	case 4:
> > +		if (pins[39] != 0xff)
> > +			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
> > +		if (pins[92] & 0x01)
> > +			mdev->model.g200.ref_clk = 14318;
> > +		break;
> > +	case 5:
> > +		tmp = pins[4] ? 8000 : 6000;
> > +		if (pins[123] != 0xff)
> > +			mdev->model.g200.pclk_min = pins[123] * tmp;
> > +		if (pins[38] != 0xff)
> > +			mdev->model.g200.pclk_max = pins[38] * tmp;
> > +		if (pins[110] & 0x01)
> > +			mdev->model.g200.ref_clk = 14318;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> > +static void mgag200_g200_init_refclk(struct mga_device *mdev)
> > +{
> > +	struct drm_device *dev = &mdev->base;
> > +	unsigned char __iomem *bios;
> > +	size_t size;
> > +
> > +	mdev->model.g200.pclk_min = 50000;
> > +	mdev->model.g200.pclk_max = 230000;
> > +	mdev->model.g200.ref_clk = 27050;
> > +
> > +	bios = pci_map_rom(dev->pdev, &size);
> > +	if (!bios)
> > +		return;
> > +
> > +	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
> > +		mgag200_g200_interpret_bios(mdev, bios, size);
> > +
> > +	pci_unmap_rom(dev->pdev, bios);
> > +
> > +	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
> > +		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
> > +		    mdev->model.g200.ref_clk);
> > +}
> > +
> >  static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
> >  {
> >  	struct drm_device *dev = &mdev->base;
> > @@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device *mdev,
> > unsigned long flags)
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (IS_G200_SE(mdev))
> > +	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
> > +		mgag200_g200_init_refclk(mdev);
> > +	else if (IS_G200_SE(mdev))
> >  		mgag200_g200se_init_unique_id(mdev);
> >  
> >  	ret = mgag200_mm_init(mdev);
> > @@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned long
> > flags)
> >   */
> >  
> >  static const struct pci_device_id mgag200_pciidlist[] = {
> > +	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI },
> > +	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP },
> >  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> >  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
> >  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B
> > },
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > index 048efe635aff..54061a61e9ca 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > @@ -38,6 +38,8 @@
> >  #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
> >  #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) + (reg))
> >  
> > +#define MGA_BIOS_OFFSET 0x7ffc
> > +
> >  #define ATTR_INDEX 0x1fc0
> >  #define ATTR_DATA 0x1fc1
> >  
> > @@ -129,6 +131,8 @@ struct mga_mc {
> >  };
> >  
> >  enum mga_type {
> > +	G200_PCI,
> > +	G200_AGP,
> >  	G200_SE_A,
> >  	G200_SE_B,
> >  	G200_WB,
> > @@ -167,12 +171,18 @@ struct mga_device {
> >  	int fb_mtrr;
> >  
> >  	union {
> > +		struct {
> > +			long ref_clk;
> > +			long pclk_min;
> > +			long pclk_max;
> > +		} g200;
> >  		struct {
> >  			/* SE model number stored in reg 0x1e24 */
> >  			u32 unique_rev_id;
> >  		} g200se;
> >  	} model;
> >  
> > +
> >  	struct mga_connector connector;
> >  	struct drm_simple_display_pipe display_pipe;
> >  };
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > index 752409c7f326..bc11552415f5 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > @@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device *mdev)
> >  	} while ((status & 0x01) && time_before(jiffies, timeout));
> >  }
> >  
> > +/*
> > + * PLL setup
> > + */
> > +
> > +static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
> > +{
> > +	struct drm_device *dev = &mdev->base;
> > +	const int post_div_max = 7;
> > +	const int in_div_min = 1;
> > +	const int in_div_max = 6;
> > +	const int feed_div_min = 7;
> > +	const int feed_div_max = 127;
> > +	u8 testm, testn;
> > +	u8 n = 0, m = 0, p, s;
> > +	long f_vco;
> > +	long computed;
> > +	long delta, tmp_delta;
> > +	long ref_clk = mdev->model.g200.ref_clk;
> > +	long p_clk_min = mdev->model.g200.pclk_min;
> > +	long p_clk_max =  mdev->model.g200.pclk_max;
> > +
> > +	if (clock > p_clk_max) {
> > +		drm_err(dev, "Pixel Clock %ld too high\n", clock);
> > +		return 1;
> > +	}
> > +
> > +	if (clock <  p_clk_min >> 3)
> 
> Looks like there's a stray space after the <. You could also just use max()
> here, but I'll leave that up to you
> 
> > +		clock = p_clk_min >> 3;
> > +
> > +	f_vco = clock;
> > +	for (p = 0;
> > +	     p <= post_div_max && f_vco < p_clk_min;
> > +	     p = (p << 1) + 1, f_vco <<= 1)
> > +		;
> > +
> > +	delta = clock;
> > +
> > +	for (testm = in_div_min; testm <= in_div_max; testm++) {
> > +		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
> > +			computed = ref_clk * (testn + 1) / (testm + 1);
> > +			if (computed < f_vco)
> > +				tmp_delta = f_vco - computed;
> > +			else
> > +				tmp_delta  = computed - f_vco;
> 
> Another stray space before the =
> 
> With those nitpicks addressed, this series is:
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> 
> > +			if (tmp_delta < delta) {
> > +				delta = tmp_delta;
> > +				m = testm;
> > +				n = testn;
> > +			}
> > +		}
> > +	}
> > +	f_vco = ref_clk * (n + 1) / (m + 1);
> > +	if (f_vco < 100000)
> > +		s = 0;
> > +	else if (f_vco < 140000)
> > +		s = 1;
> > +	else if (f_vco < 180000)
> > +		s = 2;
> > +	else
> > +		s = 3;
> > +
> > +	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
> > +		    clock, f_vco, m, n, p, s);
> > +
> > +	WREG_DAC(MGA1064_PIX_PLLC_M, m);
> > +	WREG_DAC(MGA1064_PIX_PLLC_N, n);
> > +	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
> > +
> > +	return 0;
> > +}
> > +
> >  #define P_ARRAY_SIZE 9
> >  
> >  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
> > @@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev,
> > long clock)
> >  	u8 misc;
> >  
> >  	switch(mdev->type) {
> > +	case G200_PCI:
> > +	case G200_AGP:
> > +		return mgag200_g200_set_plls(mdev, clock);
> >  	case G200_SE_A:
> >  	case G200_SE_B:
> >  		return mga_g200se_set_plls(mdev, clock);
> > @@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
> >  	};
> >  
> >  	switch (mdev->type) {
> > +	case G200_PCI:
> > +	case G200_AGP:
> > +		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
> > +		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
> > +		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
> > +		break;
> >  	case G200_SE_A:
> >  	case G200_SE_B:
> >  		dacvalue[MGA1064_VREF_CTL] = 0x03;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 12869 bytes --]

Hi

Am 17.07.20 um 00:43 schrieb Lyude Paul:
> On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
>> This patch adds support for G200 desktop cards. We can reuse the whole
>> memory and modesetting code. A few PCI and DAC register values have to
>> be updated accordingly.
>>
>> The most significant change is in the PLL setup. The get the clock limits
>> and reference clocks, parses the device's BIOS. With no BIOS found, safe
>> defaults are being used.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Co-developed-by: Egbert Eich <eich@suse.com>
>> Signed-off-by: Egbert Eich <eich@suse.com>
>> Co-developed-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> ---
>>  MAINTAINERS                            |   2 +-
>>  drivers/gpu/drm/mgag200/Kconfig        |  12 +--
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
>>  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
>>  5 files changed, 220 insertions(+), 9 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 415954a98934..4c6f96e2b79b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
>>  F:	drivers/gpu/drm/mga/
>>  F:	include/uapi/drm/mga_drm.h
>>  
>> -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
>> +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
>>  M:	Dave Airlie <airlied@redhat.com>
>>  S:	Odd Fixes
>>  F:	drivers/gpu/drm/mgag200/
>> diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
>> index 93be766715c9..eec59658a938 100644
>> --- a/drivers/gpu/drm/mgag200/Kconfig
>> +++ b/drivers/gpu/drm/mgag200/Kconfig
>> @@ -1,13 +1,11 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  config DRM_MGAG200
>> -	tristate "Kernel modesetting driver for MGA G200 server engines"
>> +	tristate "Matrox G200"
>>  	depends on DRM && PCI && MMU
>>  	select DRM_GEM_SHMEM_HELPER
>>  	select DRM_KMS_HELPER
>>  	help
>> -	 This is a KMS driver for the MGA G200 server chips, it
>> -	 does not support the original MGA G200 or any of the desktop
>> -	 chips. It requires 0.3.0 of the modesetting userspace driver,
>> -	 and a version of mga driver that will fail on KMS enabled
>> -	 devices.
>> -
>> +	 This is a KMS driver for Matrox G200 chips. It supports the original
>> +	 MGA G200 desktop chips and the server variants. It requires 0.3.0
>> +	 of the modesetting userspace driver, and a version of mga driver
>> +	 that will fail on KMS enabled devices.
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index f7652e16365c..419817d6e2cd 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
>>  	u8 crtcext3;
>>  
>>  	switch (mdev->type) {
>> +	case G200_PCI:
>> +	case G200_AGP:
>> +		if (mgag200_has_sgram(mdev))
>> +			option = 0x4049cd21;
>> +		else
>> +			option = 0x40499121;
>> +		option2 = 0x00008000;
>> +		break;
>>  	case G200_SE_A:
>>  	case G200_SE_B:
>>  		if (mgag200_has_sgram(mdev))
>> @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
>>  	return 0;
>>  }
>>  
>> +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
>> +					unsigned char __iomem *bios,
>> +					size_t size)
>> +{
>> +	static const unsigned int expected_length[6] = {
>> +		0, 64, 64, 64, 128, 128
>> +	};
>> +
>> +	struct drm_device *dev = &mdev->base;
>> +	unsigned char __iomem *pins;
> 
> huh, never realized you could write directly to unsigned char __iomem pointers!

I took the patch as-is, but this probably wouldn't work on all
architectures.

> 
>> +	unsigned int pins_len, version;
>> +	int offset;
>> +	int tmp;
>> +
>> +	if (size < MGA_BIOS_OFFSET + 1)
>> +		return;
>> +
>> +	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
>> +	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
>> +		return;
>> +
>> +	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
>> +
>> +	if (offset + 5 > size)
>> +		return;
>> +
>> +	pins = bios + offset;
>> +	if (pins[0] == 0x2e && pins[1] == 0x41) {
>> +		version = pins[5];
>> +		pins_len = pins[2];
>> +	} else {
>> +		version = 1;
>> +		pins_len = pins[0] + (pins[1] << 8);
>> +	}
>> +
>> +	if (version < 1 || version > 5) {
>> +		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
> 
> Did you maybe mean pins or PINS here? or is PInS some weird abbreviation matrox
> uses?

It's the name of a data structure


https://gitlab.freedesktop.org/xorg/driver/xf86-video-mga/-/blob/master/mga_PInS.txt

I have no idea what it stands for.

> 
>> +		return;
>> +	}
>> +	if (pins_len != expected_length[version]) {
>> +		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
>> +			 pins_len, expected_length[version]);
>> +		return;
>> +	}
>> +
>> +	if (offset + pins_len > size)
>> +		return;
>> +
>> +	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
>> +		    version, pins_len);
>> +
>> +	switch (version) {
>> +	case 1:
>> +		tmp = pins[24] + (pins[25] << 8);
>> +		if (tmp)
>> +			mdev->model.g200.pclk_max = tmp * 10;
>> +		break;
>> +	case 2:
>> +		if (pins[41] != 0xff)
>> +			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
>> +		break;
>> +	case 3:
>> +		if (pins[36] != 0xff)
>> +			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
>> +		if (pins[52] & 0x20)
>> +			mdev->model.g200.ref_clk = 14318;
>> +		break;
>> +	case 4:
>> +		if (pins[39] != 0xff)
>> +			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
>> +		if (pins[92] & 0x01)
>> +			mdev->model.g200.ref_clk = 14318;
>> +		break;
>> +	case 5:
>> +		tmp = pins[4] ? 8000 : 6000;
>> +		if (pins[123] != 0xff)
>> +			mdev->model.g200.pclk_min = pins[123] * tmp;
>> +		if (pins[38] != 0xff)
>> +			mdev->model.g200.pclk_max = pins[38] * tmp;
>> +		if (pins[110] & 0x01)
>> +			mdev->model.g200.ref_clk = 14318;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>> +static void mgag200_g200_init_refclk(struct mga_device *mdev)
>> +{
>> +	struct drm_device *dev = &mdev->base;
>> +	unsigned char __iomem *bios;
>> +	size_t size;
>> +
>> +	mdev->model.g200.pclk_min = 50000;
>> +	mdev->model.g200.pclk_max = 230000;
>> +	mdev->model.g200.ref_clk = 27050;
>> +
>> +	bios = pci_map_rom(dev->pdev, &size);
>> +	if (!bios)
>> +		return;
>> +
>> +	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
>> +		mgag200_g200_interpret_bios(mdev, bios, size);
>> +
>> +	pci_unmap_rom(dev->pdev, bios);
>> +
>> +	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
>> +		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
>> +		    mdev->model.g200.ref_clk);
>> +}
>> +
>>  static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
>>  {
>>  	struct drm_device *dev = &mdev->base;
>> @@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device *mdev,
>> unsigned long flags)
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (IS_G200_SE(mdev))
>> +	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
>> +		mgag200_g200_init_refclk(mdev);
>> +	else if (IS_G200_SE(mdev))
>>  		mgag200_g200se_init_unique_id(mdev);
>>  
>>  	ret = mgag200_mm_init(mdev);
>> @@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned long
>> flags)
>>   */
>>  
>>  static const struct pci_device_id mgag200_pciidlist[] = {
>> +	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI },
>> +	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP },
>>  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>>  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
>>  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B
>> },
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 048efe635aff..54061a61e9ca 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -38,6 +38,8 @@
>>  #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
>>  #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) + (reg))
>>  
>> +#define MGA_BIOS_OFFSET 0x7ffc
>> +
>>  #define ATTR_INDEX 0x1fc0
>>  #define ATTR_DATA 0x1fc1
>>  
>> @@ -129,6 +131,8 @@ struct mga_mc {
>>  };
>>  
>>  enum mga_type {
>> +	G200_PCI,
>> +	G200_AGP,
>>  	G200_SE_A,
>>  	G200_SE_B,
>>  	G200_WB,
>> @@ -167,12 +171,18 @@ struct mga_device {
>>  	int fb_mtrr;
>>  
>>  	union {
>> +		struct {
>> +			long ref_clk;
>> +			long pclk_min;
>> +			long pclk_max;
>> +		} g200;
>>  		struct {
>>  			/* SE model number stored in reg 0x1e24 */
>>  			u32 unique_rev_id;
>>  		} g200se;
>>  	} model;
>>  
>> +
>>  	struct mga_connector connector;
>>  	struct drm_simple_display_pipe display_pipe;
>>  };
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 752409c7f326..bc11552415f5 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device *mdev)
>>  	} while ((status & 0x01) && time_before(jiffies, timeout));
>>  }
>>  
>> +/*
>> + * PLL setup
>> + */
>> +
>> +static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
>> +{
>> +	struct drm_device *dev = &mdev->base;
>> +	const int post_div_max = 7;
>> +	const int in_div_min = 1;
>> +	const int in_div_max = 6;
>> +	const int feed_div_min = 7;
>> +	const int feed_div_max = 127;
>> +	u8 testm, testn;
>> +	u8 n = 0, m = 0, p, s;
>> +	long f_vco;
>> +	long computed;
>> +	long delta, tmp_delta;
>> +	long ref_clk = mdev->model.g200.ref_clk;
>> +	long p_clk_min = mdev->model.g200.pclk_min;
>> +	long p_clk_max =  mdev->model.g200.pclk_max;
>> +
>> +	if (clock > p_clk_max) {
>> +		drm_err(dev, "Pixel Clock %ld too high\n", clock);
>> +		return 1;
>> +	}
>> +
>> +	if (clock <  p_clk_min >> 3)
> 
> Looks like there's a stray space after the <. You could also just use max()
> here, but I'll leave that up to you
> 
>> +		clock = p_clk_min >> 3;
>> +
>> +	f_vco = clock;
>> +	for (p = 0;
>> +	     p <= post_div_max && f_vco < p_clk_min;
>> +	     p = (p << 1) + 1, f_vco <<= 1)
>> +		;
>> +
>> +	delta = clock;
>> +
>> +	for (testm = in_div_min; testm <= in_div_max; testm++) {
>> +		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
>> +			computed = ref_clk * (testn + 1) / (testm + 1);
>> +			if (computed < f_vco)
>> +				tmp_delta = f_vco - computed;
>> +			else
>> +				tmp_delta  = computed - f_vco;
> 
> Another stray space before the =
> 
> With those nitpicks addressed, this series is:
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks a lot.

The other patches are probably uncontoversial. Let's see what happens to
this one. :)

Best regards
Thomas

> 
>> +			if (tmp_delta < delta) {
>> +				delta = tmp_delta;
>> +				m = testm;
>> +				n = testn;
>> +			}
>> +		}
>> +	}
>> +	f_vco = ref_clk * (n + 1) / (m + 1);
>> +	if (f_vco < 100000)
>> +		s = 0;
>> +	else if (f_vco < 140000)
>> +		s = 1;
>> +	else if (f_vco < 180000)
>> +		s = 2;
>> +	else
>> +		s = 3;
>> +
>> +	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
>> +		    clock, f_vco, m, n, p, s);
>> +
>> +	WREG_DAC(MGA1064_PIX_PLLC_M, m);
>> +	WREG_DAC(MGA1064_PIX_PLLC_N, n);
>> +	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
>> +
>> +	return 0;
>> +}
>> +
>>  #define P_ARRAY_SIZE 9
>>  
>>  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
>> @@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device *mdev,
>> long clock)
>>  	u8 misc;
>>  
>>  	switch(mdev->type) {
>> +	case G200_PCI:
>> +	case G200_AGP:
>> +		return mgag200_g200_set_plls(mdev, clock);
>>  	case G200_SE_A:
>>  	case G200_SE_B:
>>  		return mga_g200se_set_plls(mdev, clock);
>> @@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device *mdev)
>>  	};
>>  
>>  	switch (mdev->type) {
>> +	case G200_PCI:
>> +	case G200_AGP:
>> +		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
>> +		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
>> +		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
>> +		break;
>>  	case G200_SE_A:
>>  	case G200_SE_B:
>>  		dacvalue[MGA1064_VREF_CTL] = 0x03;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Lyude Paul]

On Mon, 2020-07-20 at 09:04 +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 17.07.20 um 00:43 schrieb Lyude Paul:
> > On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> > > This patch adds support for G200 desktop cards. We can reuse the whole
> > > memory and modesetting code. A few PCI and DAC register values have to
> > > be updated accordingly.
> > > 
> > > The most significant change is in the PLL setup. The get the clock
> > > limits
> > > and reference clocks, parses the device's BIOS. With no BIOS found, safe
> > > defaults are being used.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Co-developed-by: Egbert Eich <eich@suse.com>
> > > Signed-off-by: Egbert Eich <eich@suse.com>
> > > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  MAINTAINERS                            |   2 +-
> > >  drivers/gpu/drm/mgag200/Kconfig        |  12 +--
> > >  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
> > >  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
> > >  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
> > >  5 files changed, 220 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 415954a98934..4c6f96e2b79b 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
> > >  F:	drivers/gpu/drm/mga/
> > >  F:	include/uapi/drm/mga_drm.h
> > >  
> > > -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> > > +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
> > >  M:	Dave Airlie <airlied@redhat.com>
> > >  S:	Odd Fixes
> > >  F:	drivers/gpu/drm/mgag200/
> > > diff --git a/drivers/gpu/drm/mgag200/Kconfig
> > > b/drivers/gpu/drm/mgag200/Kconfig
> > > index 93be766715c9..eec59658a938 100644
> > > --- a/drivers/gpu/drm/mgag200/Kconfig
> > > +++ b/drivers/gpu/drm/mgag200/Kconfig
> > > @@ -1,13 +1,11 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > >  config DRM_MGAG200
> > > -	tristate "Kernel modesetting driver for MGA G200 server engines"
> > > +	tristate "Matrox G200"
> > >  	depends on DRM && PCI && MMU
> > >  	select DRM_GEM_SHMEM_HELPER
> > >  	select DRM_KMS_HELPER
> > >  	help
> > > -	 This is a KMS driver for the MGA G200 server chips, it
> > > -	 does not support the original MGA G200 or any of the desktop
> > > -	 chips. It requires 0.3.0 of the modesetting userspace driver,
> > > -	 and a version of mga driver that will fail on KMS enabled
> > > -	 devices.
> > > -
> > > +	 This is a KMS driver for Matrox G200 chips. It supports the original
> > > +	 MGA G200 desktop chips and the server variants. It requires 0.3.0
> > > +	 of the modesetting userspace driver, and a version of mga driver
> > > +	 that will fail on KMS enabled devices.
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > index f7652e16365c..419817d6e2cd 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
> > >  	u8 crtcext3;
> > >  
> > >  	switch (mdev->type) {
> > > +	case G200_PCI:
> > > +	case G200_AGP:
> > > +		if (mgag200_has_sgram(mdev))
> > > +			option = 0x4049cd21;
> > > +		else
> > > +			option = 0x40499121;
> > > +		option2 = 0x00008000;
> > > +		break;
> > >  	case G200_SE_A:
> > >  	case G200_SE_B:
> > >  		if (mgag200_has_sgram(mdev))
> > > @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device
> > > *mdev)
> > >  	return 0;
> > >  }
> > >  
> > > +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> > > +					unsigned char __iomem *bios,
> > > +					size_t size)
> > > +{
> > > +	static const unsigned int expected_length[6] = {
> > > +		0, 64, 64, 64, 128, 128
> > > +	};
> > > +
> > > +	struct drm_device *dev = &mdev->base;
> > > +	unsigned char __iomem *pins;
> > 
> > huh, never realized you could write directly to unsigned char __iomem
> > pointers!
> 
> I took the patch as-is, but this probably wouldn't work on all
> architectures.

Something occurred to me just now - do we actually care? I don't think I've
ever seen mgag200 on anything other then x86_64 systems

> 
> > > +	unsigned int pins_len, version;
> > > +	int offset;
> > > +	int tmp;
> > > +
> > > +	if (size < MGA_BIOS_OFFSET + 1)
> > > +		return;
> > > +
> > > +	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> > > +	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> > > +		return;
> > > +
> > > +	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> > > +
> > > +	if (offset + 5 > size)
> > > +		return;
> > > +
> > > +	pins = bios + offset;
> > > +	if (pins[0] == 0x2e && pins[1] == 0x41) {
> > > +		version = pins[5];
> > > +		pins_len = pins[2];
> > > +	} else {
> > > +		version = 1;
> > > +		pins_len = pins[0] + (pins[1] << 8);
> > > +	}
> > > +
> > > +	if (version < 1 || version > 5) {
> > > +		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
> > 
> > Did you maybe mean pins or PINS here? or is PInS some weird abbreviation
> > matrox
> > uses?
> 
> It's the name of a data structure
> 
> 
> https://gitlab.freedesktop.org/xorg/driver/xf86-video-mga/-/blob/master/mga_PInS.txt
> 
> I have no idea what it stands for.
> 
> > > +		return;
> > > +	}
> > > +	if (pins_len != expected_length[version]) {
> > > +		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
> > > +			 pins_len, expected_length[version]);
> > > +		return;
> > > +	}
> > > +
> > > +	if (offset + pins_len > size)
> > > +		return;
> > > +
> > > +	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
> > > +		    version, pins_len);
> > > +
> > > +	switch (version) {
> > > +	case 1:
> > > +		tmp = pins[24] + (pins[25] << 8);
> > > +		if (tmp)
> > > +			mdev->model.g200.pclk_max = tmp * 10;
> > > +		break;
> > > +	case 2:
> > > +		if (pins[41] != 0xff)
> > > +			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
> > > +		break;
> > > +	case 3:
> > > +		if (pins[36] != 0xff)
> > > +			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
> > > +		if (pins[52] & 0x20)
> > > +			mdev->model.g200.ref_clk = 14318;
> > > +		break;
> > > +	case 4:
> > > +		if (pins[39] != 0xff)
> > > +			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
> > > +		if (pins[92] & 0x01)
> > > +			mdev->model.g200.ref_clk = 14318;
> > > +		break;
> > > +	case 5:
> > > +		tmp = pins[4] ? 8000 : 6000;
> > > +		if (pins[123] != 0xff)
> > > +			mdev->model.g200.pclk_min = pins[123] * tmp;
> > > +		if (pins[38] != 0xff)
> > > +			mdev->model.g200.pclk_max = pins[38] * tmp;
> > > +		if (pins[110] & 0x01)
> > > +			mdev->model.g200.ref_clk = 14318;
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +}
> > > +
> > > +static void mgag200_g200_init_refclk(struct mga_device *mdev)
> > > +{
> > > +	struct drm_device *dev = &mdev->base;
> > > +	unsigned char __iomem *bios;
> > > +	size_t size;
> > > +
> > > +	mdev->model.g200.pclk_min = 50000;
> > > +	mdev->model.g200.pclk_max = 230000;
> > > +	mdev->model.g200.ref_clk = 27050;
> > > +
> > > +	bios = pci_map_rom(dev->pdev, &size);
> > > +	if (!bios)
> > > +		return;
> > > +
> > > +	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
> > > +		mgag200_g200_interpret_bios(mdev, bios, size);
> > > +
> > > +	pci_unmap_rom(dev->pdev, bios);
> > > +
> > > +	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
> > > +		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
> > > +		    mdev->model.g200.ref_clk);
> > > +}
> > > +
> > >  static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
> > >  {
> > >  	struct drm_device *dev = &mdev->base;
> > > @@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device
> > > *mdev,
> > > unsigned long flags)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (IS_G200_SE(mdev))
> > > +	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
> > > +		mgag200_g200_init_refclk(mdev);
> > > +	else if (IS_G200_SE(mdev))
> > >  		mgag200_g200se_init_unique_id(mdev);
> > >  
> > >  	ret = mgag200_mm_init(mdev);
> > > @@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned
> > > long
> > > flags)
> > >   */
> > >  
> > >  static const struct pci_device_id mgag200_pciidlist[] = {
> > > +	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI
> > > },
> > > +	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP
> > > },
> > >  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > >  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
> > >  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B
> > > },
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > index 048efe635aff..54061a61e9ca 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > @@ -38,6 +38,8 @@
> > >  #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
> > >  #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) +
> > > (reg))
> > >  
> > > +#define MGA_BIOS_OFFSET 0x7ffc
> > > +
> > >  #define ATTR_INDEX 0x1fc0
> > >  #define ATTR_DATA 0x1fc1
> > >  
> > > @@ -129,6 +131,8 @@ struct mga_mc {
> > >  };
> > >  
> > >  enum mga_type {
> > > +	G200_PCI,
> > > +	G200_AGP,
> > >  	G200_SE_A,
> > >  	G200_SE_B,
> > >  	G200_WB,
> > > @@ -167,12 +171,18 @@ struct mga_device {
> > >  	int fb_mtrr;
> > >  
> > >  	union {
> > > +		struct {
> > > +			long ref_clk;
> > > +			long pclk_min;
> > > +			long pclk_max;
> > > +		} g200;
> > >  		struct {
> > >  			/* SE model number stored in reg 0x1e24 */
> > >  			u32 unique_rev_id;
> > >  		} g200se;
> > >  	} model;
> > >  
> > > +
> > >  	struct mga_connector connector;
> > >  	struct drm_simple_display_pipe display_pipe;
> > >  };
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > index 752409c7f326..bc11552415f5 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > @@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device
> > > *mdev)
> > >  	} while ((status & 0x01) && time_before(jiffies, timeout));
> > >  }
> > >  
> > > +/*
> > > + * PLL setup
> > > + */
> > > +
> > > +static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
> > > +{
> > > +	struct drm_device *dev = &mdev->base;
> > > +	const int post_div_max = 7;
> > > +	const int in_div_min = 1;
> > > +	const int in_div_max = 6;
> > > +	const int feed_div_min = 7;
> > > +	const int feed_div_max = 127;
> > > +	u8 testm, testn;
> > > +	u8 n = 0, m = 0, p, s;
> > > +	long f_vco;
> > > +	long computed;
> > > +	long delta, tmp_delta;
> > > +	long ref_clk = mdev->model.g200.ref_clk;
> > > +	long p_clk_min = mdev->model.g200.pclk_min;
> > > +	long p_clk_max =  mdev->model.g200.pclk_max;
> > > +
> > > +	if (clock > p_clk_max) {
> > > +		drm_err(dev, "Pixel Clock %ld too high\n", clock);
> > > +		return 1;
> > > +	}
> > > +
> > > +	if (clock <  p_clk_min >> 3)
> > 
> > Looks like there's a stray space after the <. You could also just use
> > max()
> > here, but I'll leave that up to you
> > 
> > > +		clock = p_clk_min >> 3;
> > > +
> > > +	f_vco = clock;
> > > +	for (p = 0;
> > > +	     p <= post_div_max && f_vco < p_clk_min;
> > > +	     p = (p << 1) + 1, f_vco <<= 1)
> > > +		;
> > > +
> > > +	delta = clock;
> > > +
> > > +	for (testm = in_div_min; testm <= in_div_max; testm++) {
> > > +		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
> > > +			computed = ref_clk * (testn + 1) / (testm + 1);
> > > +			if (computed < f_vco)
> > > +				tmp_delta = f_vco - computed;
> > > +			else
> > > +				tmp_delta  = computed - f_vco;
> > 
> > Another stray space before the =
> > 
> > With those nitpicks addressed, this series is:
> > 
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> 
> Thanks a lot.
> 
> The other patches are probably uncontoversial. Let's see what happens to
> this one. :)
> 
> Best regards
> Thomas
> 
> > > +			if (tmp_delta < delta) {
> > > +				delta = tmp_delta;
> > > +				m = testm;
> > > +				n = testn;
> > > +			}
> > > +		}
> > > +	}
> > > +	f_vco = ref_clk * (n + 1) / (m + 1);
> > > +	if (f_vco < 100000)
> > > +		s = 0;
> > > +	else if (f_vco < 140000)
> > > +		s = 1;
> > > +	else if (f_vco < 180000)
> > > +		s = 2;
> > > +	else
> > > +		s = 3;
> > > +
> > > +	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
> > > +		    clock, f_vco, m, n, p, s);
> > > +
> > > +	WREG_DAC(MGA1064_PIX_PLLC_M, m);
> > > +	WREG_DAC(MGA1064_PIX_PLLC_N, n);
> > > +	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  #define P_ARRAY_SIZE 9
> > >  
> > >  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
> > > @@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device
> > > *mdev,
> > > long clock)
> > >  	u8 misc;
> > >  
> > >  	switch(mdev->type) {
> > > +	case G200_PCI:
> > > +	case G200_AGP:
> > > +		return mgag200_g200_set_plls(mdev, clock);
> > >  	case G200_SE_A:
> > >  	case G200_SE_B:
> > >  		return mga_g200se_set_plls(mdev, clock);
> > > @@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device
> > > *mdev)
> > >  	};
> > >  
> > >  	switch (mdev->type) {
> > > +	case G200_PCI:
> > > +	case G200_AGP:
> > > +		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
> > > +		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
> > > +		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
> > > +		break;
> > >  	case G200_SE_A:
> > >  	case G200_SE_B:
> > >  		dacvalue[MGA1064_VREF_CTL] = 0x03;
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Sam Ravnborg]

On Mon, Jul 20, 2020 at 03:18:56PM -0400, Lyude Paul wrote:
> On Mon, 2020-07-20 at 09:04 +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 17.07.20 um 00:43 schrieb Lyude Paul:
> > > On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> > > > This patch adds support for G200 desktop cards. We can reuse the whole
> > > > memory and modesetting code. A few PCI and DAC register values have to
> > > > be updated accordingly.
> > > > 
> > > > The most significant change is in the PLL setup. The get the clock
> > > > limits
> > > > and reference clocks, parses the device's BIOS. With no BIOS found, safe
> > > > defaults are being used.
> > > > 
> > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Co-developed-by: Egbert Eich <eich@suse.com>
> > > > Signed-off-by: Egbert Eich <eich@suse.com>
> > > > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > >  MAINTAINERS                            |   2 +-
> > > >  drivers/gpu/drm/mgag200/Kconfig        |  12 +--
> > > >  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
> > > >  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
> > > >  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
> > > >  5 files changed, 220 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 415954a98934..4c6f96e2b79b 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
> > > >  F:	drivers/gpu/drm/mga/
> > > >  F:	include/uapi/drm/mga_drm.h
> > > >  
> > > > -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> > > > +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
> > > >  M:	Dave Airlie <airlied@redhat.com>
> > > >  S:	Odd Fixes
> > > >  F:	drivers/gpu/drm/mgag200/
> > > > diff --git a/drivers/gpu/drm/mgag200/Kconfig
> > > > b/drivers/gpu/drm/mgag200/Kconfig
> > > > index 93be766715c9..eec59658a938 100644
> > > > --- a/drivers/gpu/drm/mgag200/Kconfig
> > > > +++ b/drivers/gpu/drm/mgag200/Kconfig
> > > > @@ -1,13 +1,11 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > >  config DRM_MGAG200
> > > > -	tristate "Kernel modesetting driver for MGA G200 server engines"
> > > > +	tristate "Matrox G200"
> > > >  	depends on DRM && PCI && MMU
> > > >  	select DRM_GEM_SHMEM_HELPER
> > > >  	select DRM_KMS_HELPER
> > > >  	help
> > > > -	 This is a KMS driver for the MGA G200 server chips, it
> > > > -	 does not support the original MGA G200 or any of the desktop
> > > > -	 chips. It requires 0.3.0 of the modesetting userspace driver,
> > > > -	 and a version of mga driver that will fail on KMS enabled
> > > > -	 devices.
> > > > -
> > > > +	 This is a KMS driver for Matrox G200 chips. It supports the original
> > > > +	 MGA G200 desktop chips and the server variants. It requires 0.3.0
> > > > +	 of the modesetting userspace driver, and a version of mga driver
> > > > +	 that will fail on KMS enabled devices.
> > > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > index f7652e16365c..419817d6e2cd 100644
> > > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
> > > >  	u8 crtcext3;
> > > >  
> > > >  	switch (mdev->type) {
> > > > +	case G200_PCI:
> > > > +	case G200_AGP:
> > > > +		if (mgag200_has_sgram(mdev))
> > > > +			option = 0x4049cd21;
> > > > +		else
> > > > +			option = 0x40499121;
> > > > +		option2 = 0x00008000;
> > > > +		break;
> > > >  	case G200_SE_A:
> > > >  	case G200_SE_B:
> > > >  		if (mgag200_has_sgram(mdev))
> > > > @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device
> > > > *mdev)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> > > > +					unsigned char __iomem *bios,
> > > > +					size_t size)
> > > > +{
> > > > +	static const unsigned int expected_length[6] = {
> > > > +		0, 64, 64, 64, 128, 128
> > > > +	};
> > > > +
> > > > +	struct drm_device *dev = &mdev->base;
> > > > +	unsigned char __iomem *pins;
> > > 
> > > huh, never realized you could write directly to unsigned char __iomem
> > > pointers!
> > 
> > I took the patch as-is, but this probably wouldn't work on all
> > architectures.
> 
> Something occurred to me just now - do we actually care? I don't think I've
> ever seen mgag200 on anything other then x86_64 systems

For starters to silence the sparse warnings.
Also other parts of the driver uses the io{read,write} functions
so to be consistent this code shoudl also do it.
And to avoid having bad code floating around.

It is not as it is a big deal to fix it neither does it hurt
performance.
But then it is up to Thomas in the end.

	Sam


> 
> > 
> > > > +	unsigned int pins_len, version;
> > > > +	int offset;
> > > > +	int tmp;
> > > > +
> > > > +	if (size < MGA_BIOS_OFFSET + 1)
> > > > +		return;
> > > > +
> > > > +	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> > > > +	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> > > > +		return;
> > > > +
> > > > +	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> > > > +
> > > > +	if (offset + 5 > size)
> > > > +		return;
> > > > +
> > > > +	pins = bios + offset;
> > > > +	if (pins[0] == 0x2e && pins[1] == 0x41) {
> > > > +		version = pins[5];
> > > > +		pins_len = pins[2];
> > > > +	} else {
> > > > +		version = 1;
> > > > +		pins_len = pins[0] + (pins[1] << 8);
> > > > +	}
> > > > +
> > > > +	if (version < 1 || version > 5) {
> > > > +		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
> > > 
> > > Did you maybe mean pins or PINS here? or is PInS some weird abbreviation
> > > matrox
> > > uses?
> > 
> > It's the name of a data structure
> > 
> > 
> > https://gitlab.freedesktop.org/xorg/driver/xf86-video-mga/-/blob/master/mga_PInS.txt
> > 
> > I have no idea what it stands for.
> > 
> > > > +		return;
> > > > +	}
> > > > +	if (pins_len != expected_length[version]) {
> > > > +		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
> > > > +			 pins_len, expected_length[version]);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	if (offset + pins_len > size)
> > > > +		return;
> > > > +
> > > > +	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
> > > > +		    version, pins_len);
> > > > +
> > > > +	switch (version) {
> > > > +	case 1:
> > > > +		tmp = pins[24] + (pins[25] << 8);
> > > > +		if (tmp)
> > > > +			mdev->model.g200.pclk_max = tmp * 10;
> > > > +		break;
> > > > +	case 2:
> > > > +		if (pins[41] != 0xff)
> > > > +			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
> > > > +		break;
> > > > +	case 3:
> > > > +		if (pins[36] != 0xff)
> > > > +			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
> > > > +		if (pins[52] & 0x20)
> > > > +			mdev->model.g200.ref_clk = 14318;
> > > > +		break;
> > > > +	case 4:
> > > > +		if (pins[39] != 0xff)
> > > > +			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
> > > > +		if (pins[92] & 0x01)
> > > > +			mdev->model.g200.ref_clk = 14318;
> > > > +		break;
> > > > +	case 5:
> > > > +		tmp = pins[4] ? 8000 : 6000;
> > > > +		if (pins[123] != 0xff)
> > > > +			mdev->model.g200.pclk_min = pins[123] * tmp;
> > > > +		if (pins[38] != 0xff)
> > > > +			mdev->model.g200.pclk_max = pins[38] * tmp;
> > > > +		if (pins[110] & 0x01)
> > > > +			mdev->model.g200.ref_clk = 14318;
> > > > +		break;
> > > > +	default:
> > > > +		break;
> > > > +	}
> > > > +}
> > > > +
> > > > +static void mgag200_g200_init_refclk(struct mga_device *mdev)
> > > > +{
> > > > +	struct drm_device *dev = &mdev->base;
> > > > +	unsigned char __iomem *bios;
> > > > +	size_t size;
> > > > +
> > > > +	mdev->model.g200.pclk_min = 50000;
> > > > +	mdev->model.g200.pclk_max = 230000;
> > > > +	mdev->model.g200.ref_clk = 27050;
> > > > +
> > > > +	bios = pci_map_rom(dev->pdev, &size);
> > > > +	if (!bios)
> > > > +		return;
> > > > +
> > > > +	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
> > > > +		mgag200_g200_interpret_bios(mdev, bios, size);
> > > > +
> > > > +	pci_unmap_rom(dev->pdev, bios);
> > > > +
> > > > +	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
> > > > +		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
> > > > +		    mdev->model.g200.ref_clk);
> > > > +}
> > > > +
> > > >  static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
> > > >  {
> > > >  	struct drm_device *dev = &mdev->base;
> > > > @@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device
> > > > *mdev,
> > > > unsigned long flags)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	if (IS_G200_SE(mdev))
> > > > +	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
> > > > +		mgag200_g200_init_refclk(mdev);
> > > > +	else if (IS_G200_SE(mdev))
> > > >  		mgag200_g200se_init_unique_id(mdev);
> > > >  
> > > >  	ret = mgag200_mm_init(mdev);
> > > > @@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned
> > > > long
> > > > flags)
> > > >   */
> > > >  
> > > >  static const struct pci_device_id mgag200_pciidlist[] = {
> > > > +	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI
> > > > },
> > > > +	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP
> > > > },
> > > >  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > > >  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
> > > >  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B
> > > > },
> > > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > > b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > > index 048efe635aff..54061a61e9ca 100644
> > > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > > > @@ -38,6 +38,8 @@
> > > >  #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
> > > >  #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) +
> > > > (reg))
> > > >  
> > > > +#define MGA_BIOS_OFFSET 0x7ffc
> > > > +
> > > >  #define ATTR_INDEX 0x1fc0
> > > >  #define ATTR_DATA 0x1fc1
> > > >  
> > > > @@ -129,6 +131,8 @@ struct mga_mc {
> > > >  };
> > > >  
> > > >  enum mga_type {
> > > > +	G200_PCI,
> > > > +	G200_AGP,
> > > >  	G200_SE_A,
> > > >  	G200_SE_B,
> > > >  	G200_WB,
> > > > @@ -167,12 +171,18 @@ struct mga_device {
> > > >  	int fb_mtrr;
> > > >  
> > > >  	union {
> > > > +		struct {
> > > > +			long ref_clk;
> > > > +			long pclk_min;
> > > > +			long pclk_max;
> > > > +		} g200;
> > > >  		struct {
> > > >  			/* SE model number stored in reg 0x1e24 */
> > > >  			u32 unique_rev_id;
> > > >  		} g200se;
> > > >  	} model;
> > > >  
> > > > +
> > > >  	struct mga_connector connector;
> > > >  	struct drm_simple_display_pipe display_pipe;
> > > >  };
> > > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > > b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > > index 752409c7f326..bc11552415f5 100644
> > > > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > > > @@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device
> > > > *mdev)
> > > >  	} while ((status & 0x01) && time_before(jiffies, timeout));
> > > >  }
> > > >  
> > > > +/*
> > > > + * PLL setup
> > > > + */
> > > > +
> > > > +static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
> > > > +{
> > > > +	struct drm_device *dev = &mdev->base;
> > > > +	const int post_div_max = 7;
> > > > +	const int in_div_min = 1;
> > > > +	const int in_div_max = 6;
> > > > +	const int feed_div_min = 7;
> > > > +	const int feed_div_max = 127;
> > > > +	u8 testm, testn;
> > > > +	u8 n = 0, m = 0, p, s;
> > > > +	long f_vco;
> > > > +	long computed;
> > > > +	long delta, tmp_delta;
> > > > +	long ref_clk = mdev->model.g200.ref_clk;
> > > > +	long p_clk_min = mdev->model.g200.pclk_min;
> > > > +	long p_clk_max =  mdev->model.g200.pclk_max;
> > > > +
> > > > +	if (clock > p_clk_max) {
> > > > +		drm_err(dev, "Pixel Clock %ld too high\n", clock);
> > > > +		return 1;
> > > > +	}
> > > > +
> > > > +	if (clock <  p_clk_min >> 3)
> > > 
> > > Looks like there's a stray space after the <. You could also just use
> > > max()
> > > here, but I'll leave that up to you
> > > 
> > > > +		clock = p_clk_min >> 3;
> > > > +
> > > > +	f_vco = clock;
> > > > +	for (p = 0;
> > > > +	     p <= post_div_max && f_vco < p_clk_min;
> > > > +	     p = (p << 1) + 1, f_vco <<= 1)
> > > > +		;
> > > > +
> > > > +	delta = clock;
> > > > +
> > > > +	for (testm = in_div_min; testm <= in_div_max; testm++) {
> > > > +		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
> > > > +			computed = ref_clk * (testn + 1) / (testm + 1);
> > > > +			if (computed < f_vco)
> > > > +				tmp_delta = f_vco - computed;
> > > > +			else
> > > > +				tmp_delta  = computed - f_vco;
> > > 
> > > Another stray space before the =
> > > 
> > > With those nitpicks addressed, this series is:
> > > 
> > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > 
> > Thanks a lot.
> > 
> > The other patches are probably uncontoversial. Let's see what happens to
> > this one. :)
> > 
> > Best regards
> > Thomas
> > 
> > > > +			if (tmp_delta < delta) {
> > > > +				delta = tmp_delta;
> > > > +				m = testm;
> > > > +				n = testn;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +	f_vco = ref_clk * (n + 1) / (m + 1);
> > > > +	if (f_vco < 100000)
> > > > +		s = 0;
> > > > +	else if (f_vco < 140000)
> > > > +		s = 1;
> > > > +	else if (f_vco < 180000)
> > > > +		s = 2;
> > > > +	else
> > > > +		s = 3;
> > > > +
> > > > +	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
> > > > +		    clock, f_vco, m, n, p, s);
> > > > +
> > > > +	WREG_DAC(MGA1064_PIX_PLLC_M, m);
> > > > +	WREG_DAC(MGA1064_PIX_PLLC_N, n);
> > > > +	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  #define P_ARRAY_SIZE 9
> > > >  
> > > >  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
> > > > @@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device
> > > > *mdev,
> > > > long clock)
> > > >  	u8 misc;
> > > >  
> > > >  	switch(mdev->type) {
> > > > +	case G200_PCI:
> > > > +	case G200_AGP:
> > > > +		return mgag200_g200_set_plls(mdev, clock);
> > > >  	case G200_SE_A:
> > > >  	case G200_SE_B:
> > > >  		return mga_g200se_set_plls(mdev, clock);
> > > > @@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device
> > > > *mdev)
> > > >  	};
> > > >  
> > > >  	switch (mdev->type) {
> > > > +	case G200_PCI:
> > > > +	case G200_AGP:
> > > > +		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
> > > > +		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
> > > > +		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
> > > > +		break;
> > > >  	case G200_SE_A:
> > > >  	case G200_SE_B:
> > > >  		dacvalue[MGA1064_VREF_CTL] = 0x03;
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Associate Software Engineer at Red Hat
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 14444 bytes --]

Hi

Am 20.07.20 um 21:18 schrieb Lyude Paul:
> On Mon, 2020-07-20 at 09:04 +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 17.07.20 um 00:43 schrieb Lyude Paul:
>>> On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
>>>> This patch adds support for G200 desktop cards. We can reuse the whole
>>>> memory and modesetting code. A few PCI and DAC register values have to
>>>> be updated accordingly.
>>>>
>>>> The most significant change is in the PLL setup. The get the clock
>>>> limits
>>>> and reference clocks, parses the device's BIOS. With no BIOS found, safe
>>>> defaults are being used.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Co-developed-by: Egbert Eich <eich@suse.com>
>>>> Signed-off-by: Egbert Eich <eich@suse.com>
>>>> Co-developed-by: Takashi Iwai <tiwai@suse.de>
>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>> ---
>>>>  MAINTAINERS                            |   2 +-
>>>>  drivers/gpu/drm/mgag200/Kconfig        |  12 +--
>>>>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 ++++++++++++++++++++++++-
>>>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
>>>>  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 ++++++++++++++++
>>>>  5 files changed, 220 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 415954a98934..4c6f96e2b79b 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -5406,7 +5406,7 @@ S:	Orphan / Obsolete
>>>>  F:	drivers/gpu/drm/mga/
>>>>  F:	include/uapi/drm/mga_drm.h
>>>>  
>>>> -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
>>>> +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
>>>>  M:	Dave Airlie <airlied@redhat.com>
>>>>  S:	Odd Fixes
>>>>  F:	drivers/gpu/drm/mgag200/
>>>> diff --git a/drivers/gpu/drm/mgag200/Kconfig
>>>> b/drivers/gpu/drm/mgag200/Kconfig
>>>> index 93be766715c9..eec59658a938 100644
>>>> --- a/drivers/gpu/drm/mgag200/Kconfig
>>>> +++ b/drivers/gpu/drm/mgag200/Kconfig
>>>> @@ -1,13 +1,11 @@
>>>>  # SPDX-License-Identifier: GPL-2.0-only
>>>>  config DRM_MGAG200
>>>> -	tristate "Kernel modesetting driver for MGA G200 server engines"
>>>> +	tristate "Matrox G200"
>>>>  	depends on DRM && PCI && MMU
>>>>  	select DRM_GEM_SHMEM_HELPER
>>>>  	select DRM_KMS_HELPER
>>>>  	help
>>>> -	 This is a KMS driver for the MGA G200 server chips, it
>>>> -	 does not support the original MGA G200 or any of the desktop
>>>> -	 chips. It requires 0.3.0 of the modesetting userspace driver,
>>>> -	 and a version of mga driver that will fail on KMS enabled
>>>> -	 devices.
>>>> -
>>>> +	 This is a KMS driver for Matrox G200 chips. It supports the original
>>>> +	 MGA G200 desktop chips and the server variants. It requires 0.3.0
>>>> +	 of the modesetting userspace driver, and a version of mga driver
>>>> +	 that will fail on KMS enabled devices.
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> b/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> index f7652e16365c..419817d6e2cd 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
>>>>  	u8 crtcext3;
>>>>  
>>>>  	switch (mdev->type) {
>>>> +	case G200_PCI:
>>>> +	case G200_AGP:
>>>> +		if (mgag200_has_sgram(mdev))
>>>> +			option = 0x4049cd21;
>>>> +		else
>>>> +			option = 0x40499121;
>>>> +		option2 = 0x00008000;
>>>> +		break;
>>>>  	case G200_SE_A:
>>>>  	case G200_SE_B:
>>>>  		if (mgag200_has_sgram(mdev))
>>>> @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device
>>>> *mdev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
>>>> +					unsigned char __iomem *bios,
>>>> +					size_t size)
>>>> +{
>>>> +	static const unsigned int expected_length[6] = {
>>>> +		0, 64, 64, 64, 128, 128
>>>> +	};
>>>> +
>>>> +	struct drm_device *dev = &mdev->base;
>>>> +	unsigned char __iomem *pins;
>>>
>>> huh, never realized you could write directly to unsigned char __iomem
>>> pointers!
>>
>> I took the patch as-is, but this probably wouldn't work on all
>> architectures.
> 
> Something occurred to me just now - do we actually care? I don't think I've
> ever seen mgag200 on anything other then x86_64 systems

That's no big deal. As Sam mentioned, it fixes some warnings and does
not cause overhead. Besides, we have a bug in fbdev that is caused by
direct I/O within framebuffer memory. So I'll better do it right here.

Wrt architecture, the MGAs have a PowerPC mode where they interpret
certain I/O as big endian IIRC. They where standard on some old Macs.
(?) I guess it's not really relevant any longer.

Best regards
Thomas

> 
>>
>>>> +	unsigned int pins_len, version;
>>>> +	int offset;
>>>> +	int tmp;
>>>> +
>>>> +	if (size < MGA_BIOS_OFFSET + 1)
>>>> +		return;
>>>> +
>>>> +	if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
>>>> +	    bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
>>>> +		return;
>>>> +
>>>> +	offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
>>>> +
>>>> +	if (offset + 5 > size)
>>>> +		return;
>>>> +
>>>> +	pins = bios + offset;
>>>> +	if (pins[0] == 0x2e && pins[1] == 0x41) {
>>>> +		version = pins[5];
>>>> +		pins_len = pins[2];
>>>> +	} else {
>>>> +		version = 1;
>>>> +		pins_len = pins[0] + (pins[1] << 8);
>>>> +	}
>>>> +
>>>> +	if (version < 1 || version > 5) {
>>>> +		drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
>>>
>>> Did you maybe mean pins or PINS here? or is PInS some weird abbreviation
>>> matrox
>>> uses?
>>
>> It's the name of a data structure
>>
>>
>> https://gitlab.freedesktop.org/xorg/driver/xf86-video-mga/-/blob/master/mga_PInS.txt
>>
>> I have no idea what it stands for.
>>
>>>> +		return;
>>>> +	}
>>>> +	if (pins_len != expected_length[version]) {
>>>> +		drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
>>>> +			 pins_len, expected_length[version]);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (offset + pins_len > size)
>>>> +		return;
>>>> +
>>>> +	drm_dbg_kms(dev, "MATROX BIOS PInS version %d size: %d found\n",
>>>> +		    version, pins_len);
>>>> +
>>>> +	switch (version) {
>>>> +	case 1:
>>>> +		tmp = pins[24] + (pins[25] << 8);
>>>> +		if (tmp)
>>>> +			mdev->model.g200.pclk_max = tmp * 10;
>>>> +		break;
>>>> +	case 2:
>>>> +		if (pins[41] != 0xff)
>>>> +			mdev->model.g200.pclk_max = (pins[41] + 100) * 1000;
>>>> +		break;
>>>> +	case 3:
>>>> +		if (pins[36] != 0xff)
>>>> +			mdev->model.g200.pclk_max = (pins[36] + 100) * 1000;
>>>> +		if (pins[52] & 0x20)
>>>> +			mdev->model.g200.ref_clk = 14318;
>>>> +		break;
>>>> +	case 4:
>>>> +		if (pins[39] != 0xff)
>>>> +			mdev->model.g200.pclk_max = pins[39] * 4 * 1000;
>>>> +		if (pins[92] & 0x01)
>>>> +			mdev->model.g200.ref_clk = 14318;
>>>> +		break;
>>>> +	case 5:
>>>> +		tmp = pins[4] ? 8000 : 6000;
>>>> +		if (pins[123] != 0xff)
>>>> +			mdev->model.g200.pclk_min = pins[123] * tmp;
>>>> +		if (pins[38] != 0xff)
>>>> +			mdev->model.g200.pclk_max = pins[38] * tmp;
>>>> +		if (pins[110] & 0x01)
>>>> +			mdev->model.g200.ref_clk = 14318;
>>>> +		break;
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +}
>>>> +
>>>> +static void mgag200_g200_init_refclk(struct mga_device *mdev)
>>>> +{
>>>> +	struct drm_device *dev = &mdev->base;
>>>> +	unsigned char __iomem *bios;
>>>> +	size_t size;
>>>> +
>>>> +	mdev->model.g200.pclk_min = 50000;
>>>> +	mdev->model.g200.pclk_max = 230000;
>>>> +	mdev->model.g200.ref_clk = 27050;
>>>> +
>>>> +	bios = pci_map_rom(dev->pdev, &size);
>>>> +	if (!bios)
>>>> +		return;
>>>> +
>>>> +	if (size != 0 && bios[0] == 0x55 && bios[1] == 0xaa)
>>>> +		mgag200_g200_interpret_bios(mdev, bios, size);
>>>> +
>>>> +	pci_unmap_rom(dev->pdev, bios);
>>>> +
>>>> +	drm_dbg_kms(dev, "pclk_min: %ld pclk_max: %ld ref_clk: %ld\n",
>>>> +		    mdev->model.g200.pclk_min, mdev->model.g200.pclk_max,
>>>> +		    mdev->model.g200.ref_clk);
>>>> +}
>>>> +
>>>>  static void mgag200_g200se_init_unique_id(struct mga_device *mdev)
>>>>  {
>>>>  	struct drm_device *dev = &mdev->base;
>>>> @@ -138,7 +257,9 @@ static int mgag200_device_init(struct mga_device
>>>> *mdev,
>>>> unsigned long flags)
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	if (IS_G200_SE(mdev))
>>>> +	if (mdev->type == G200_PCI || mdev->type == G200_AGP)
>>>> +		mgag200_g200_init_refclk(mdev);
>>>> +	else if (IS_G200_SE(mdev))
>>>>  		mgag200_g200se_init_unique_id(mdev);
>>>>  
>>>>  	ret = mgag200_mm_init(mdev);
>>>> @@ -182,6 +303,8 @@ mgag200_device_create(struct pci_dev *pdev, unsigned
>>>> long
>>>> flags)
>>>>   */
>>>>  
>>>>  static const struct pci_device_id mgag200_pciidlist[] = {
>>>> +	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI
>>>> },
>>>> +	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP
>>>> },
>>>>  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>>>>  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
>>>>  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B
>>>> },
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> b/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> index 048efe635aff..54061a61e9ca 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> @@ -38,6 +38,8 @@
>>>>  #define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg))
>>>>  #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) +
>>>> (reg))
>>>>  
>>>> +#define MGA_BIOS_OFFSET 0x7ffc
>>>> +
>>>>  #define ATTR_INDEX 0x1fc0
>>>>  #define ATTR_DATA 0x1fc1
>>>>  
>>>> @@ -129,6 +131,8 @@ struct mga_mc {
>>>>  };
>>>>  
>>>>  enum mga_type {
>>>> +	G200_PCI,
>>>> +	G200_AGP,
>>>>  	G200_SE_A,
>>>>  	G200_SE_B,
>>>>  	G200_WB,
>>>> @@ -167,12 +171,18 @@ struct mga_device {
>>>>  	int fb_mtrr;
>>>>  
>>>>  	union {
>>>> +		struct {
>>>> +			long ref_clk;
>>>> +			long pclk_min;
>>>> +			long pclk_max;
>>>> +		} g200;
>>>>  		struct {
>>>>  			/* SE model number stored in reg 0x1e24 */
>>>>  			u32 unique_rev_id;
>>>>  		} g200se;
>>>>  	} model;
>>>>  
>>>> +
>>>>  	struct mga_connector connector;
>>>>  	struct drm_simple_display_pipe display_pipe;
>>>>  };
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> index 752409c7f326..bc11552415f5 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> @@ -108,6 +108,77 @@ static inline void mga_wait_busy(struct mga_device
>>>> *mdev)
>>>>  	} while ((status & 0x01) && time_before(jiffies, timeout));
>>>>  }
>>>>  
>>>> +/*
>>>> + * PLL setup
>>>> + */
>>>> +
>>>> +static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
>>>> +{
>>>> +	struct drm_device *dev = &mdev->base;
>>>> +	const int post_div_max = 7;
>>>> +	const int in_div_min = 1;
>>>> +	const int in_div_max = 6;
>>>> +	const int feed_div_min = 7;
>>>> +	const int feed_div_max = 127;
>>>> +	u8 testm, testn;
>>>> +	u8 n = 0, m = 0, p, s;
>>>> +	long f_vco;
>>>> +	long computed;
>>>> +	long delta, tmp_delta;
>>>> +	long ref_clk = mdev->model.g200.ref_clk;
>>>> +	long p_clk_min = mdev->model.g200.pclk_min;
>>>> +	long p_clk_max =  mdev->model.g200.pclk_max;
>>>> +
>>>> +	if (clock > p_clk_max) {
>>>> +		drm_err(dev, "Pixel Clock %ld too high\n", clock);
>>>> +		return 1;
>>>> +	}
>>>> +
>>>> +	if (clock <  p_clk_min >> 3)
>>>
>>> Looks like there's a stray space after the <. You could also just use
>>> max()
>>> here, but I'll leave that up to you
>>>
>>>> +		clock = p_clk_min >> 3;
>>>> +
>>>> +	f_vco = clock;
>>>> +	for (p = 0;
>>>> +	     p <= post_div_max && f_vco < p_clk_min;
>>>> +	     p = (p << 1) + 1, f_vco <<= 1)
>>>> +		;
>>>> +
>>>> +	delta = clock;
>>>> +
>>>> +	for (testm = in_div_min; testm <= in_div_max; testm++) {
>>>> +		for (testn = feed_div_min; testn <= feed_div_max; testn++) {
>>>> +			computed = ref_clk * (testn + 1) / (testm + 1);
>>>> +			if (computed < f_vco)
>>>> +				tmp_delta = f_vco - computed;
>>>> +			else
>>>> +				tmp_delta  = computed - f_vco;
>>>
>>> Another stray space before the =
>>>
>>> With those nitpicks addressed, this series is:
>>>
>>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>>
>> Thanks a lot.
>>
>> The other patches are probably uncontoversial. Let's see what happens to
>> this one. :)
>>
>> Best regards
>> Thomas
>>
>>>> +			if (tmp_delta < delta) {
>>>> +				delta = tmp_delta;
>>>> +				m = testm;
>>>> +				n = testn;
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +	f_vco = ref_clk * (n + 1) / (m + 1);
>>>> +	if (f_vco < 100000)
>>>> +		s = 0;
>>>> +	else if (f_vco < 140000)
>>>> +		s = 1;
>>>> +	else if (f_vco < 180000)
>>>> +		s = 2;
>>>> +	else
>>>> +		s = 3;
>>>> +
>>>> +	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
>>>> +		    clock, f_vco, m, n, p, s);
>>>> +
>>>> +	WREG_DAC(MGA1064_PIX_PLLC_M, m);
>>>> +	WREG_DAC(MGA1064_PIX_PLLC_N, n);
>>>> +	WREG_DAC(MGA1064_PIX_PLLC_P, (p | (s << 3)));
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  #define P_ARRAY_SIZE 9
>>>>  
>>>>  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
>>>> @@ -717,6 +788,9 @@ static int mgag200_crtc_set_plls(struct mga_device
>>>> *mdev,
>>>> long clock)
>>>>  	u8 misc;
>>>>  
>>>>  	switch(mdev->type) {
>>>> +	case G200_PCI:
>>>> +	case G200_AGP:
>>>> +		return mgag200_g200_set_plls(mdev, clock);
>>>>  	case G200_SE_A:
>>>>  	case G200_SE_B:
>>>>  		return mga_g200se_set_plls(mdev, clock);
>>>> @@ -894,6 +968,12 @@ static void mgag200_set_dac_regs(struct mga_device
>>>> *mdev)
>>>>  	};
>>>>  
>>>>  	switch (mdev->type) {
>>>> +	case G200_PCI:
>>>> +	case G200_AGP:
>>>> +		dacvalue[MGA1064_SYS_PLL_M] = 0x04;
>>>> +		dacvalue[MGA1064_SYS_PLL_N] = 0x2D;
>>>> +		dacvalue[MGA1064_SYS_PLL_P] = 0x19;
>>>> +		break;
>>>>  	case G200_SE_A:
>>>>  	case G200_SE_B:
>>>>  		dacvalue[MGA1064_VREF_CTL] = 0x03;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/mgag200: Support desktop chips

[Dave Airlie]

On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> This patchset puts device initialization in the correct order and
> adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).

why? :-)

I'm pretty sure I NAKed the previous version because the userspace
experience for these old cards was probably better with
xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go
ahead. I know SuSE use these for testing, but apart from that do we
really think we have any users for this?

Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/mgag200: Support desktop chips

[Daniel Vetter]

On Wed, Jul 15, 2020 at 9:56 PM Dave Airlie <airlied@gmail.com> wrote:
>
> On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > This patchset puts device initialization in the correct order and
> > adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).
>
> why? :-)
>
> I'm pretty sure I NAKed the previous version because the userspace
> experience for these old cards was probably better with
> xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go
> ahead. I know SuSE use these for testing, but apart from that do we
> really think we have any users for this?

I'm more of the "why not" kind ... if you don't want this driver,
don't enable it. Maybe worst case the physical card driver ids should
be a Kconfig option or so. But if the goal is to stomp fbdev into the
ground I think we should be ok with having drivers for anything. Even
if it's kinda horrible :-)

Of course you're not going to get any kind of acceleration, but then
modern desktops don't accelerate if you have anything less than maybe
gles2 anyway, and that entire idea of a reasonable 2d api that's
actually generally useful died a hundred times already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/mgag200: Support desktop chips

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1535 bytes --]

Hi

Am 15.07.20 um 21:56 schrieb Dave Airlie:
> On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> This patchset puts device initialization in the correct order and
>> adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).
> 
> why? :-)

With G200 support in place, we can add also support for the newer cards
in the G-Series up to the G550. Believe it or not, the G550 for PCIe is
still being actively marketed and manufactured by Matrox. [1] Even the
predecessor chips G450 was only EOLed in Oct 2016. [2] So while the
chips might be 20yrs old, the devices are still current.

Best regards
Thomas

[1]
https://matrox.com/graphics/en/products/graphics_cards/g_series/g550pcie/?productTabs=1
[2] https://www.matrox.com/graphics/en/products/legacy/g_series/g450pci/

> 
> I'm pretty sure I NAKed the previous version because the userspace
> experience for these old cards was probably better with
> xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go
> ahead. I know SuSE use these for testing, but apart from that do we
> really think we have any users for this?
> 
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/mgag200: Support desktop chips

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1901 bytes --]



Am 16.07.20 um 07:44 schrieb Thomas Zimmermann:
> Hi
> 
> Am 15.07.20 um 21:56 schrieb Dave Airlie:
>> On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>
>>> This patchset puts device initialization in the correct order and
>>> adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).
>>
>> why? :-)
> 
> With G200 support in place, we can add also support for the newer cards
> in the G-Series up to the G550. Believe it or not, the G550 for PCIe is
> still being actively marketed and manufactured by Matrox. [1] Even the
> predecessor chips G450 was only EOLed in Oct 2016. [2] So while the
> chips might be 20yrs old, the devices are still current.
> 
> Best regards
> Thomas
> 
> [1]
> https://matrox.com/graphics/en/products/graphics_cards/g_series/g550pcie/?productTabs=1
> [2] https://www.matrox.com/graphics/en/products/legacy/g_series/g450pci/
> 
>>
>> I'm pretty sure I NAKed the previous version because the userspace
>> experience for these old cards was probably better with
>> xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go
>> ahead. I know SuSE use these for testing, but apart from that do we
>> really think we have any users for this?

Well, I got at least one email from someone thanking me for this patch. :)

>>
>> Dave.
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/mgag200: Support desktop chips

[Egbert Eich]

Dave Airlie writes:
 > On Thu, 16 Jul 2020 at 00:59, Thomas Zimmermann <tzimmermann@suse.de> wrote:
 > >
 > > This patchset puts device initialization in the correct order and
 > > adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).
 > 
 > why? :-)
 > 
 > I'm pretty sure I NAKed the previous version because the userspace
 > experience for these old cards was probably better with
 > xorg-x11-drv-mga, but hey maybe it isn't anymore and we should go
 > ahead. I know SuSE use these for testing, but apart from that do we
 > really think we have any users for this?
 > 

The reason I had added this was to enable me to test the
basic functionality of this driver with a test box at
my desk rather than having to rely on some remote screen
setup to some server half way around the world ;)

And I'm actually not sure if the user experience with
this card with the xorg-x11-drv-mga is as good as it
used to be in its prime time days.

Cheers,
	Egbert.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] drm/mgag200: Support desktop chips

[Lyude Paul]

Will try to look over this tomorrow, jfyi

On Wed, 2020-07-15 at 16:58 +0200, Thomas Zimmermann wrote:
> This patchset puts device initialization in the correct order and
> adds support for G200 Desktop chips (PCI ids 0x520 and 0x521).
> 
> The first 7 patches prepare the driver. Desktop chips would probably
> work without them, but since we're at it we can also do it right.
> 
> Patch 1 enables cached page mappings GEM buffers. SHMEM supports
> this well now and the MGA device does not access the buffer memory
> directly. So now corrupt display output is to be expected.
> 
> Patches 2 to 6 fix the initialization of device registers. Several
> fundamental registers were only done late during device initialization.
> This was probably not a problem in practice, as the VGA BIOS does the
> setup iduring POST anyway. These patches move the code to the beginning
> of the device initialization. If we ever have to POST a MGA device from
> the driver, the corect order of operations counts.
> 
> G200SEs store a unique id in the device structure. Patch 7 moves the
> value to model-specific data area. This will be helpful for patch 8.
> 
> Patch 8 adds support for desktop chips' PCI ids. all the memory and
> modesetting code continues to work as before. The PLL setup code gets
> an additional helper for the new HW. PCI and DAC regsiters get a few
> new default values. Most significantly, the driver parses the VGA BIOS
> for clock settings. It's all separate from the server code, so no
> regressions are to be expected.
> 
> The new HW support is based on an earlier patch the was posted in July
> 2017. [1]
> 
> Tested on G200EW and G200 AGP hardware by running the fbdev console,
> Weston and Gnome on Xorg.
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147647.html
> 
> Thomas Zimmermann (8):
>   drm/mgag200: Enable caching for SHMEM pages
>   drm/mgag200: Move register initialization into helper function
>   drm/mgag200: Initialize PCI registers early during device setup
>   drm/mgag200: Enable MGA mode during device register initialization
>   drm/mgag200: Set MISC memory flags in mm init code
>   drm/mgag200: Clear <page> field during MM init
>   drm/mgag200: Move G200SE's unique id into model-specific data
>   drm/mgag200: Add support for G200 desktop cards
> 
>  MAINTAINERS                            |   2 +-
>  drivers/gpu/drm/mgag200/Kconfig        |  12 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 213 +++++++++++++++++++++++--
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 ++-
>  drivers/gpu/drm/mgag200/mgag200_mm.c   |   8 +
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 153 +++++++++++-------
>  drivers/gpu/drm/mgag200/mgag200_reg.h  |   4 +
>  7 files changed, 328 insertions(+), 83 deletions(-)
> 
> --
> 2.27.0
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel