On 03/02/2025 14:07, Gwenael Georgeault wrote:
- Added the new device ID
- Added new pll algorithm
Hi,
Thanks for this patch.
Overall it looks good, I have a few comments below:
Best regards,
--
Jocelyn
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 ++++++++++++++++++++++
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);
The alignment was correct, don't modify indentation on lines you don't
change.
+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);
Here too
/*
* 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
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#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)
+{
+
+ 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)
We don't use type prefix in the kernel, and usually variables are all
lowercase.
Also those 3 variables don't need to be static. (Used like this, it
means it will keep it's value when you call this function again, which
is not needed as they are constant).
+
+ struct drm_crtc_state *new_crtc_state =
drm_atomic_get_new_crtc_state(new_state, crtc);
+ struct mgag200_crtc_state *new_mgag200_crtc_state =
to_mgag200_crtc_state(new_crtc_state);
+ long clock = new_crtc_state->mode.clock;
+ 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)
Here the type prefix is wrong, as it's not an unsigned long. But like
all variables, just remove the prefix, and rename them to lowercase
(either multmax or mult_max).
+
+ u64 ulFTmpDelta;
+ u64 ulComputedFo;
+
+ u16 ulTestM;
+ u8 ulTestDivA;
+ u8 ulTestDivB;
+ u64 ulFoHz;
+ int iDone = 0;
iDone is useless, if you find the right parameters, and ulFDelta is 0,
then you won't find a better solution, because you can't have
(ulFTmpDelta < ulFDelta).
It's a small optimisation to avoid some loops, but the PLL are
configured only once, so it's really not worth it.
+
+ u8 ucM = 0;
+ u8 ucN = 0;
+ u8 ucP = 0;
+
+ ulFoHz = (u64)clock * 1000ULL;
+
+
+ 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
+
+ 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);
+ 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;
+}
