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
- Added the new device ID
- Added new pll algorithm
Co-authored-by: Mamadou Insa Diop <mdiop@matrox.com>
---
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);
+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
@@ -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)
+
+ 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)
+
+ 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 (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;
+}
--
2.34.1Hi
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@matrox.com>
> ---
> 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)
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@matrox.com>
> ---
> 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;
> +}
© 2016 - 2026 Red Hat, Inc.