Re: [PATCH] drm/mgag200: Added support for the new device G200eH5
From: Thomas Zimmermann
Date: Tue Feb 04 2025 - 03:55:54 EST
Hi
Am 03.02.25 um 14:07 schrieb Gwenael Georgeault:
Thanks for sending this patch and welcome to the kernel community
- Added the new device ID
- Added new pll algorithm
Co-authored-by: Mamadou Insa Diop <mdiop@xxxxxxxxxx>
---
drivers/gpu/drm/mgag200/Makefile | 1 +
drivers/gpu/drm/mgag200/mgag200_drv.c | 4 +
drivers/gpu/drm/mgag200/mgag200_drv.h | 7 +-
drivers/gpu/drm/mgag200/mgag200_g200eh5.c | 212 ++++++++++++++++++++++
Great, all new code located in a single file. That's how it is intended.
The code looks correct, but I have plenty of comments on the style.
4 files changed, 222 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/mgag200/mgag200_g200eh5.c
diff --git a/drivers/gpu/drm/mgag200/Makefile
b/drivers/gpu/drm/mgag200/Makefile
index 5a02203fad12..94f063c8722a 100644
--- a/drivers/gpu/drm/mgag200/Makefile
+++ b/drivers/gpu/drm/mgag200/Makefile
@@ -6,6 +6,7 @@ mgag200-y := \
mgag200_g200.o \
mgag200_g200eh.o \
mgag200_g200eh3.o \
+ mgag200_g200eh5.o \
mgag200_g200er.o \
mgag200_g200ev.o \
mgag200_g200ew3.o \
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 069fdd2dc8f6..1c257f5b5136 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -214,6 +214,7 @@ static const struct pci_device_id
mgag200_pciidlist[] = {
{ PCI_VENDOR_ID_MATROX, 0x534, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
G200_ER },
{ PCI_VENDOR_ID_MATROX, 0x536, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
G200_EW3 },
{ PCI_VENDOR_ID_MATROX, 0x538, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
G200_EH3 },
+ { PCI_VENDOR_ID_MATROX, 0x53A, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
G200_EH5 },
{0,}
};
@@ -256,6 +257,9 @@ mgag200_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
case G200_EH3:
mdev = mgag200_g200eh3_device_create(pdev, &mgag200_driver);
break;
+ case G200_EH5:
+ mdev = mgag200_g200eh5_device_create(pdev, &mgag200_driver);
+ break;
case G200_ER:
mdev = mgag200_g200er_device_create(pdev, &mgag200_driver);
break;
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 0608fc63e588..065ba09d109b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -196,6 +196,7 @@ enum mga_type {
G200_EV,
G200_EH,
G200_EH3,
+ G200_EH5,
G200_ER,
G200_EW3,
};
@@ -333,11 +334,13 @@ void mgag200_g200eh_pixpllc_atomic_update(struct
drm_crtc *crtc, struct drm_atom
struct mga_device *mgag200_g200eh_device_create(struct pci_dev *pdev,
const struct drm_driver *drv);
struct mga_device *mgag200_g200eh3_device_create(struct pci_dev *pdev,
- const struct drm_driver *drv);
+ const struct drm_driver *drv);
+struct mga_device *mgag200_g200eh5_device_create(struct pci_dev *pdev,
+ const struct drm_driver *drv);
struct mga_device *mgag200_g200er_device_create(struct pci_dev *pdev,
const struct drm_driver *drv);
struct mga_device *mgag200_g200ew3_device_create(struct pci_dev *pdev,
- const struct drm_driver *drv);
+ const struct drm_driver *drv);
/*
* mgag200_mode.c
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh5.c
b/drivers/gpu/drm/mgag200/mgag200_g200eh5.c
new file mode 100644
index 000000000000..5e39504785d8
--- /dev/null
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh5.c
The coding style in this file is off. Jocelyn already pointed out quite
a few issues. Please run checkpatch.pl on the patch file before
submitting and fix the errors and warnings. It's in the scripts/ directory.
./scripts/checkpatch.pl --strict <filename>
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0-only
Empty line here.
+#include <linux/pci.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_probe_helper.h>
+
+#include "mgag200_drv.h"
+
+/*
+ * PIXPLLC
+ */
+
+static int mgag200_g200eh5_pixpllc_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_state *new_state)
+{
+
No empty line here.
+ static u64 ulVCOMax = 10000000000ULL; // units in Hz (10 GHz)
+ static u64 ulVCOMin = 2500000000LL; // units in Hz (2.5 GHz)
+ static u64 ulPLLFreqRef = 25000000ULL; // units in Hz (25 MHz)
The camel case and hungarian notation is not used within the kernel. The
code that uses this has likely been copied from somewhere else.
Fixed-size types (e.g., u64) should be avoided IIRC; unless they are
necessary. Using 'static' is ok if you also make it 'const', so that
there are real constant. For these numbers, you could also look at
<linux/units.h> for SI multipliers.
For these constants, I'd write something like
static const unsigned long long VCO_MAX = 10 * GIGA // Hz
static const unsigned long long VCO_MAX = 2500 * MEGA // Hz
static const unsigned long long PLL_FREQ_REF = 25 * MEGA // Hz
+
+ struct drm_crtc_state *new_crtc_state =
drm_atomic_get_new_crtc_state(new_state, crtc);
This file appears to be using 4 spaces per level of indention. Indention
within the kernel is 1 tab. Each tab is equivalent to 8 space.
+ struct mgag200_crtc_state *new_mgag200_crtc_state =
to_mgag200_crtc_state(new_crtc_state);
+ long clock = new_crtc_state->mode.clock;
Multiple spaces after 'long'. There are similar cases below.
+ struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
+
+ u64 ulFDelta = 0xFFFFFFFFFFFFFFFFULL;
+
+ u16 ulMultMax = (u16)(ulVCOMax / ulPLLFreqRef); // 400 (0x190)
+ u16 ulMultMin = (u16)(ulVCOMin / ulPLLFreqRef); // 100 (0x64)
+
+ u64 ulFTmpDelta;
+ u64 ulComputedFo;
+
+ u16 ulTestM;
+ u8 ulTestDivA;
+ u8 ulTestDivB;
+ u64 ulFoHz;
+ int iDone = 0;
+
+ u8 ucM = 0;
+ u8 ucN = 0;
+ u8 ucP = 0;
+
+ ulFoHz = (u64)clock * 1000ULL;
For such conversions, you should use HZ_PER_KHZ, also found in
<linux/units.h>. Makes it clear what it does.
+
+
+ for (ulTestM = ulMultMin; ulTestM <= ulMultMax; ulTestM++) { //
This gives 100 <= M <= 400
+ for (ulTestDivA = 8; ulTestDivA > 0; ulTestDivA--) { // This
gives 1 <= A <= 8
+ for (ulTestDivB = 1; ulTestDivB <= ulTestDivA;
ulTestDivB++) {
+ // This gives 1 <= B <= A
+ ulComputedFo = (ulPLLFreqRef * ulTestM) /
+ (4 * ulTestDivA * ulTestDivB);
+
+ if (ulComputedFo > ulFoHz)
+ ulFTmpDelta = ulComputedFo - ulFoHz;
+ else
+ ulFTmpDelta = ulFoHz - ulComputedFo;
+
+ if (ulFTmpDelta < ulFDelta) {
+ ulFDelta = ulFTmpDelta;
+ ucM = (u8)(0xFF & ulTestM);
+ ucN = (u8)(
+ (0x7 & (ulTestDivA - 1))
+ | (0x70 & (0x7 & (ulTestDivB - 1)) << 4)
+ );
+ ucP = (u8)(1 & (ulTestM >> 8));
+ }
+ if (ulFDelta == 0) {
+ iDone = 1;
+ break;
+ }
+ } // End of DivB if (iDone)
+ if (iDone)
+ break;
+ } // End of DivA Loop
+
+ if (iDone)
+ break;
+ } // End of M Loop
As with all other models, frequency calculation is not easily
understandable. I haven't found a way to make these nested loops
readable, so it's OK to do this here as well.
But you should remove these '// End of' comments. This is something that
the formating should make obvious.
+
+ pixpllc->m = ucM + 1;
+ pixpllc->n = ucN + 1;
+ pixpllc->p = ucP + 1;
+ pixpllc->s = 0;
+
+ return 0;
+ }
+
+
+
+/*
+ * Mode-setting pipeline
+ */
+
+static const struct drm_plane_helper_funcs
mgag200_g200eh5_primary_plane_helper_funcs = {
+ MGAG200_PRIMARY_PLANE_HELPER_FUNCS,
+};
+
+static const struct drm_plane_funcs
mgag200_g200eh5_primary_plane_funcs = {
+ MGAG200_PRIMARY_PLANE_FUNCS,
+};
+
+static const struct drm_crtc_helper_funcs
mgag200_g200eh5_crtc_helper_funcs = {
+ MGAG200_CRTC_HELPER_FUNCS,
+};
+
+static const struct drm_crtc_funcs mgag200_g200eh5_crtc_funcs = {
+ MGAG200_CRTC_FUNCS,
+};
+
+static int mgag200_g200eh5_pipeline_init(struct mga_device *mdev)
+{
+ struct drm_device *dev = &mdev->base;
+ struct drm_plane *primary_plane = &mdev->primary_plane;
+ struct drm_crtc *crtc = &mdev->crtc;
+ int ret;
+
+ ret = drm_universal_plane_init(dev, primary_plane, 0,
+ &mgag200_g200eh5_primary_plane_funcs,
+ mgag200_primary_plane_formats,
+ mgag200_primary_plane_formats_size,
+ mgag200_primary_plane_fmtmods,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
This should be indented as shown at
https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/process/coding-style.rst#L497
The file will tell you how to format the code.
Best regards
Thomas
+ if (ret) {
+ drm_err(dev, "drm_universal_plane_init() failed: %d\n", ret);
+ return ret;
+ }
+ drm_plane_helper_add(primary_plane,
&mgag200_g200eh5_primary_plane_helper_funcs);
+ drm_plane_enable_fb_damage_clips(primary_plane);
+
+ ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+ &mgag200_g200eh5_crtc_funcs, NULL);
+ if (ret) {
+ drm_err(dev, "drm_crtc_init_with_planes() failed: %d\n", ret);
+ return ret;
+ }
+
+ drm_crtc_helper_add(crtc, &mgag200_g200eh5_crtc_helper_funcs);
+
+ /* FIXME: legacy gamma tables, but atomic gamma doesn't work
without */
+ drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
+ drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
+ ret = mgag200_vga_bmc_output_init(mdev);
+
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+/*
+ * DRM device
+ */
+
+static const struct mgag200_device_info mgag200_g200eh5_device_info =
+ MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, false, 1, 0, false);
+
+static const struct mgag200_device_funcs mgag200_g200eh5_device_funcs
= {
+ .pixpllc_atomic_check = mgag200_g200eh5_pixpllc_atomic_check,
+ .pixpllc_atomic_update = mgag200_g200eh_pixpllc_atomic_update, //
same as G200EH
+};
+
+struct mga_device *mgag200_g200eh5_device_create(struct pci_dev *pdev,
+ const struct drm_driver *drv)
+{
+
+ struct mga_device *mdev;
+ struct drm_device *dev;
+ resource_size_t vram_available;
+ int ret;
+
+ mdev = devm_drm_dev_alloc(&pdev->dev, drv, struct mga_device, base);
+
+ if (IS_ERR(mdev))
+ return mdev;
+ dev = &mdev->base;
+
+ pci_set_drvdata(pdev, dev);
+
+ ret = mgag200_init_pci_options(pdev, 0x00000120, 0x0000b000);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = mgag200_device_preinit(mdev);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = mgag200_device_init(mdev, &mgag200_g200eh5_device_info,
+ &mgag200_g200eh5_device_funcs);
+
+ if (ret)
+ return ERR_PTR(ret);
+
+ mgag200_g200eh_init_registers(mdev); // same as G200EH
+ vram_available = mgag200_device_probe_vram(mdev);
+
+ ret = mgag200_mode_config_init(mdev, vram_available);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = mgag200_g200eh5_pipeline_init(mdev);
+ if (ret)
+ return ERR_PTR(ret);
+
+ drm_mode_config_reset(dev);
+ drm_kms_helper_poll_init(dev);
+
+ return mdev;
+}
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)