[PATCH 0/7] drm/mgag200: Various cleanups

[PATCH 0/7] drm/mgag200: Various cleanups

[Thomas Zimmermann]

Various small fixes and clean-up patches for mgag200. Tested on
Matrix G200EH hardware.

Thomas Zimmermann (7):
  drm:/mgag200: Acquire I/O lock while reading EDID
  drm/mgag200: Fail on I2C initialization errors
  drm/mgag200: Implement connector's get_modes with helper
  drm/mgag200: Switch I2C code to managed cleanup
  drm/mgag200: Remove struct mga_connector
  drm/mgag200: Test memory requirements in
    drm_mode_config_funcs.mode_valid
  drm/mgag200: Split up connector's mode_valid helper

 drivers/gpu/drm/drm_probe_helper.c     |  33 ++++
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  13 +-
 drivers/gpu/drm/mgag200/mgag200_i2c.c  |  32 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c | 259 +++++++++++--------------
 include/drm/drm_probe_helper.h         |   2 +
 5 files changed, 166 insertions(+), 173 deletions(-)


base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3
-- 
2.36.0

[PATCH 1/7] drm:/mgag200: Acquire I/O lock while reading EDID

[Thomas Zimmermann]

DDC operation conflicts with concurrent mode setting. Acquire the
driver's I/O lock in get_modes to prevent this. This change should
have been part of commit 931e3f3a0e99 ("drm/mgag200: Protect
concurrent access to I/O registers with lock"), but apparently got
lost somewhere.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 931e3f3a0e99 ("drm/mgag200: Protect concurrent access to I/O registers with lock")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index abde7655477d..4ad8d62c5631 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -667,16 +667,26 @@ static void mgag200_disable_display(struct mga_device *mdev)
 
 static int mga_vga_get_modes(struct drm_connector *connector)
 {
+	struct mga_device *mdev = to_mga_device(connector->dev);
 	struct mga_connector *mga_connector = to_mga_connector(connector);
 	struct edid *edid;
 	int ret = 0;
 
+	/*
+	 * Protect access to I/O registers from concurrent modesetting
+	 * by acquiring the I/O-register lock.
+	 */
+	mutex_lock(&mdev->rmmio_lock);
+
 	edid = drm_get_edid(connector, &mga_connector->i2c->adapter);
 	if (edid) {
 		drm_connector_update_edid_property(connector, edid);
 		ret = drm_add_edid_modes(connector, edid);
 		kfree(edid);
 	}
+
+	mutex_unlock(&mdev->rmmio_lock);
+
 	return ret;
 }
 
-- 
2.36.0

[PATCH 2/7] drm/mgag200: Fail on I2C initialization errors

[Thomas Zimmermann]

Initialization of the I2C adapter was allowed to fail. The mgag200
driver would have continued without DDC support. Had this happened in
practice, it would have led to segmentation faults in the connector
code. Resolve this problem by failing driver initialization on I2C-
related errors.

v2:
	* initialize 'ret' before drm_err() (kernel test robot)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_i2c.c  | 13 ++++++++-----
 drivers/gpu/drm/mgag200/mgag200_mode.c |  7 +++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index ac8e34eef513..31e2c641a781 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -120,7 +120,7 @@ struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
 
 	i2c = kzalloc(sizeof(struct mga_i2c_chan), GFP_KERNEL);
 	if (!i2c)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	i2c->data = data;
 	i2c->clock = clock;
@@ -142,11 +142,14 @@ struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
 	i2c->bit.getscl		= mga_gpio_getscl;
 
 	ret = i2c_bit_add_bus(&i2c->adapter);
-	if (ret) {
-		kfree(i2c);
-		i2c = NULL;
-	}
+	if (ret)
+		goto err_kfree;
+
 	return i2c;
+
+err_kfree:
+	kfree(i2c);
+	return ERR_PTR(ret);
 }
 
 void mgag200_i2c_destroy(struct mga_i2c_chan *i2c)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 4ad8d62c5631..88c095184308 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -825,8 +825,11 @@ static int mgag200_vga_connector_init(struct mga_device *mdev)
 	int ret;
 
 	i2c = mgag200_i2c_create(dev);
-	if (!i2c)
-		drm_warn(dev, "failed to add DDC bus\n");
+	if (IS_ERR(i2c)) {
+		ret = PTR_ERR(i2c)
+		drm_err(dev, "failed to add DDC bus: %d\n", ret);
+		return ret;
+	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mga_vga_connector_funcs,
-- 
2.36.0

[PATCH 3/7] drm/mgag200: Implement connector's get_modes with helper

[Thomas Zimmermann]

Provide drm_connector_helper_get_modes_from_ddc() to implement the
connector's get_modes callback. The new helper updates the connector
from DDC-provided EDID data.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c     | 33 ++++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 17 ++++---------
 include/drm/drm_probe_helper.h         |  2 ++
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 682359512996..916bfb30ca19 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -964,3 +964,36 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 	return changed;
 }
 EXPORT_SYMBOL(drm_helper_hpd_irq_event);
+
+/**
+ * drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID
+ *                                           property from the connector's
+ *                                           DDC channel
+ * @connector: The connector
+ *
+ * Returns:
+ * The number of detected display modes.
+ *
+ * Uses a connector's DDC channel to retrieve EDID data and update the
+ * connector's EDID property and display modes. Drivers can use this
+ * function to implement struct &drm_connector_helper_funcs.get_modes
+ * for connectors with a DDC channel.
+ */
+int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
+{
+	struct edid *edid;
+	int ret = 0;
+
+	if (!connector->ddc)
+		return 0;
+
+	edid = drm_get_edid(connector, connector->ddc);
+	if (edid) {
+		drm_connector_update_edid_property(connector, edid);
+		ret = drm_add_edid_modes(connector, edid);
+		kfree(edid);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 88c095184308..e09814528f04 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -665,26 +665,17 @@ static void mgag200_disable_display(struct mga_device *mdev)
  * Connector
  */
 
-static int mga_vga_get_modes(struct drm_connector *connector)
+static int mgag200_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct mga_device *mdev = to_mga_device(connector->dev);
-	struct mga_connector *mga_connector = to_mga_connector(connector);
-	struct edid *edid;
-	int ret = 0;
+	int ret;
 
 	/*
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
 	mutex_lock(&mdev->rmmio_lock);
-
-	edid = drm_get_edid(connector, &mga_connector->i2c->adapter);
-	if (edid) {
-		drm_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		kfree(edid);
-	}
-
+	ret = drm_connector_helper_get_modes_from_ddc(connector);
 	mutex_unlock(&mdev->rmmio_lock);
 
 	return ret;
@@ -804,7 +795,7 @@ static void mga_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
-	.get_modes  = mga_vga_get_modes,
+	.get_modes  = mgag200_vga_connector_helper_get_modes,
 	.mode_valid = mga_vga_mode_valid,
 };
 
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 48300aa6ca71..c80cab7a53b7 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -26,4 +26,6 @@ void drm_kms_helper_poll_disable(struct drm_device *dev);
 void drm_kms_helper_poll_enable(struct drm_device *dev);
 bool drm_kms_helper_is_poll_worker(void);
 
+int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
+
 #endif
-- 
2.36.0

[PATCH 4/7] drm/mgag200: Switch I2C code to managed cleanup

[Thomas Zimmermann]

Store the I2C state within struct mga_device and switch I2C to
managed release. Simplifies the related code and lets us remove
mga_connector_destroy().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++--
 drivers/gpu/drm/mgag200/mgag200_i2c.c  | 33 +++++++++-----------------
 drivers/gpu/drm/mgag200/mgag200_mode.c | 24 ++++---------------
 3 files changed, 18 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index a18384c41fc4..5bdd09432114 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -179,7 +179,6 @@ struct mga_i2c_chan {
 
 struct mga_connector {
 	struct drm_connector base;
-	struct mga_i2c_chan *i2c;
 };
 
 struct mga_mc {
@@ -239,6 +238,7 @@ struct mga_device {
 
 	struct mga_connector connector;
 	struct mgag200_pll pixpll;
+	struct mga_i2c_chan i2c;
 	struct drm_simple_display_pipe display_pipe;
 };
 
@@ -251,8 +251,7 @@ static inline struct mga_device *to_mga_device(struct drm_device *dev)
 int mgag200_modeset_init(struct mga_device *mdev);
 
 				/* mgag200_i2c.c */
-struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
-void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
+int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c);
 
 				/* mgag200_mm.c */
 int mgag200_mm_init(struct mga_device *mdev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index 31e2c641a781..f57b33917152 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -86,10 +86,16 @@ static int mga_gpio_getscl(void *data)
 	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
 }
 
-struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
+static void mgag200_i2c_release(void *res)
 {
-	struct mga_device *mdev = to_mga_device(dev);
-	struct mga_i2c_chan *i2c;
+	struct mga_i2c_chan *i2c = res;
+
+	i2c_del_adapter(&i2c->adapter);
+}
+
+int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
+{
+	struct drm_device *dev = &mdev->base;
 	int ret;
 	int data, clock;
 
@@ -118,10 +124,6 @@ struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
 		break;
 	}
 
-	i2c = kzalloc(sizeof(struct mga_i2c_chan), GFP_KERNEL);
-	if (!i2c)
-		return ERR_PTR(-ENOMEM);
-
 	i2c->data = data;
 	i2c->clock = clock;
 	i2c->adapter.owner = THIS_MODULE;
@@ -143,20 +145,7 @@ struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
 
 	ret = i2c_bit_add_bus(&i2c->adapter);
 	if (ret)
-		goto err_kfree;
+		return ret;
 
-	return i2c;
-
-err_kfree:
-	kfree(i2c);
-	return ERR_PTR(ret);
-}
-
-void mgag200_i2c_destroy(struct mga_i2c_chan *i2c)
-{
-	if (!i2c)
-		return;
-	i2c_del_adapter(&i2c->adapter);
-	kfree(i2c);
+	return devm_add_action_or_reset(dev->dev, mgag200_i2c_release, i2c);
 }
-
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e09814528f04..a6d89b33b9ca 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -787,13 +787,6 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
-static void mga_connector_destroy(struct drm_connector *connector)
-{
-	struct mga_connector *mga_connector = to_mga_connector(connector);
-	mgag200_i2c_destroy(mga_connector->i2c);
-	drm_connector_cleanup(connector);
-}
-
 static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
 	.get_modes  = mgag200_vga_connector_helper_get_modes,
 	.mode_valid = mga_vga_mode_valid,
@@ -802,7 +795,7 @@ static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs =
 static const struct drm_connector_funcs mga_vga_connector_funcs = {
 	.reset                  = drm_atomic_helper_connector_reset,
 	.fill_modes             = drm_helper_probe_single_connector_modes,
-	.destroy                = mga_connector_destroy,
+	.destroy                = drm_connector_cleanup,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
 };
@@ -812,12 +805,11 @@ static int mgag200_vga_connector_init(struct mga_device *mdev)
 	struct drm_device *dev = &mdev->base;
 	struct mga_connector *mconnector = &mdev->connector;
 	struct drm_connector *connector = &mconnector->base;
-	struct mga_i2c_chan *i2c;
+	struct mga_i2c_chan *i2c = &mdev->i2c;
 	int ret;
 
-	i2c = mgag200_i2c_create(dev);
-	if (IS_ERR(i2c)) {
-		ret = PTR_ERR(i2c)
+	ret = mgag200_i2c_init(mdev, i2c);
+	if (ret) {
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
@@ -827,16 +819,10 @@ static int mgag200_vga_connector_init(struct mga_device *mdev)
 					  DRM_MODE_CONNECTOR_VGA,
 					  &i2c->adapter);
 	if (ret)
-		goto err_mgag200_i2c_destroy;
+		return ret;
 	drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs);
 
-	mconnector->i2c = i2c;
-
 	return 0;
-
-err_mgag200_i2c_destroy:
-	mgag200_i2c_destroy(i2c);
-	return ret;
 }
 
 /*
-- 
2.36.0

[PATCH 5/7] drm/mgag200: Remove struct mga_connector

[Thomas Zimmermann]

struct mga_connector has outlived its purpose. Inline the rsp init
helper into the mode-config code and remove the data structure. No
functional changes.

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 5bdd09432114..5634fc003ca4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -168,8 +168,6 @@ static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_s
 	return container_of(base, struct mgag200_crtc_state, base);
 }
 
-#define to_mga_connector(x) container_of(x, struct mga_connector, base)
-
 struct mga_i2c_chan {
 	struct i2c_adapter adapter;
 	struct drm_device *dev;
@@ -177,10 +175,6 @@ struct mga_i2c_chan {
 	int data, clock;
 };
 
-struct mga_connector {
-	struct drm_connector base;
-};
-
 struct mga_mc {
 	resource_size_t			vram_size;
 	resource_size_t			vram_base;
@@ -236,9 +230,9 @@ struct mga_device {
 		} g200se;
 	} model;
 
-	struct mga_connector connector;
 	struct mgag200_pll pixpll;
 	struct mga_i2c_chan i2c;
+	struct drm_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 a6d89b33b9ca..eb4d585709a4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -800,31 +800,6 @@ static const struct drm_connector_funcs mga_vga_connector_funcs = {
 	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
 };
 
-static int mgag200_vga_connector_init(struct mga_device *mdev)
-{
-	struct drm_device *dev = &mdev->base;
-	struct mga_connector *mconnector = &mdev->connector;
-	struct drm_connector *connector = &mconnector->base;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
-	int ret;
-
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
-		drm_err(dev, "failed to add DDC bus: %d\n", ret);
-		return ret;
-	}
-
-	ret = drm_connector_init_with_ddc(dev, connector,
-					  &mga_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
-	if (ret)
-		return ret;
-	drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs);
-
-	return 0;
-}
-
 /*
  * Simple Display Pipe
  */
@@ -1063,7 +1038,8 @@ static unsigned int mgag200_preferred_depth(struct mga_device *mdev)
 int mgag200_modeset_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
-	struct drm_connector *connector = &mdev->connector.base;
+	struct mga_i2c_chan *i2c = &mdev->i2c;
+	struct drm_connector *connector = &mdev->connector;
 	struct drm_simple_display_pipe *pipe = &mdev->display_pipe;
 	size_t format_count = ARRAY_SIZE(mgag200_simple_display_pipe_formats);
 	int ret;
@@ -1086,14 +1062,22 @@ int mgag200_modeset_init(struct mga_device *mdev)
 
 	dev->mode_config.funcs = &mgag200_mode_config_funcs;
 
-	ret = mgag200_vga_connector_init(mdev);
+	ret = mgag200_i2c_init(mdev, i2c);
 	if (ret) {
-		drm_err(dev,
-			"mgag200_vga_connector_init() failed, error %d\n",
-			ret);
+		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
+	ret = drm_connector_init_with_ddc(dev, connector,
+					  &mga_vga_connector_funcs,
+					  DRM_MODE_CONNECTOR_VGA,
+					  &i2c->adapter);
+	if (ret) {
+		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
+		return ret;
+	}
+	drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs);
+
 	ret = mgag200_pixpll_init(&mdev->pixpll, mdev);
 	if (ret)
 		return ret;
-- 
2.36.0

[PATCH 6/7] drm/mgag200: Test memory requirements in drm_mode_config_funcs.mode_valid

[Thomas Zimmermann]

Test for a mode's memory requirements in the device-wide mode_valid
helper. For simplicify, always assume a 32-bit color format. While
some rejected modes would work with less colors, implementing this
is probably not worth the effort.

Also remove the memory-related test from the connector's mode_valid
helper. The test uses the bpp value that users can specify on the
kernel's command line. This value is unrelated and the test would
belong into atomic_check.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 35 +++++++++++++++-----------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index eb4d585709a4..92d3ae9489f0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -772,18 +772,6 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 		return MODE_BAD;
 	}
 
-	/* Validate the mode input by the user */
-	if (connector->cmdline_mode.specified) {
-		if (connector->cmdline_mode.bpp_specified)
-			bpp = connector->cmdline_mode.bpp;
-	}
-
-	if ((mode->hdisplay * mode->vdisplay * (bpp/8)) > mdev->vram_fb_available) {
-		if (connector->cmdline_mode.specified)
-			connector->cmdline_mode.specified = false;
-		return MODE_BAD;
-	}
-
 	return MODE_OK;
 }
 
@@ -1021,9 +1009,28 @@ static const uint64_t mgag200_simple_display_pipe_fmtmods[] = {
  * Mode config
  */
 
+static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *dev,
+							   const struct drm_display_mode *mode)
+{
+	static const unsigned int max_bpp = 4; // DRM_FORMAT_XRGB8888
+	struct mga_device *mdev = to_mga_device(dev);
+	unsigned long fbsize, fbpages, max_fbpages;
+
+	max_fbpages = mdev->vram_fb_available >> PAGE_SHIFT;
+
+	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
+	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
+
+	if (fbpages > max_fbpages)
+		return MODE_MEM;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs mgag200_mode_config_funcs = {
-	.fb_create     = drm_gem_fb_create_with_dirty,
-	.atomic_check  = drm_atomic_helper_check,
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = mgag200_mode_config_mode_valid,
+	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
-- 
2.36.0

[PATCH 7/7] drm/mgag200: Split up connector's mode_valid helper

[Thomas Zimmermann]

Split up the connector's mode_valid helper into a simple-pipe and a
mode-config helper. The simple-pipe helper tests for display-size
limits while the mode-config helper tests for memory-bandwidth limits.

Also add the mgag200_ prefix to mga_vga_calculate_mode_bandwidth() and
comment on the function's purpose.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 146 ++++++++++++-------------
 1 file changed, 69 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 92d3ae9489f0..a808827d7a2c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -681,38 +681,27 @@ static int mgag200_vga_connector_helper_get_modes(struct drm_connector *connecto
 	return ret;
 }
 
-static uint32_t mga_vga_calculate_mode_bandwidth(struct drm_display_mode *mode,
-							int bits_per_pixel)
-{
-	uint32_t total_area, divisor;
-	uint64_t active_area, pixels_per_second, bandwidth;
-	uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
-
-	divisor = 1024;
-
-	if (!mode->htotal || !mode->vtotal || !mode->clock)
-		return 0;
-
-	active_area = mode->hdisplay * mode->vdisplay;
-	total_area = mode->htotal * mode->vtotal;
-
-	pixels_per_second = active_area * mode->clock * 1000;
-	do_div(pixels_per_second, total_area);
-
-	bandwidth = pixels_per_second * bytes_per_pixel * 100;
-	do_div(bandwidth, divisor);
+static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
+	.get_modes  = mgag200_vga_connector_helper_get_modes,
+};
 
-	return (uint32_t)(bandwidth);
-}
+static const struct drm_connector_funcs mga_vga_connector_funcs = {
+	.reset                  = drm_atomic_helper_connector_reset,
+	.fill_modes             = drm_helper_probe_single_connector_modes,
+	.destroy                = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
+};
 
-#define MODE_BANDWIDTH	MODE_BAD
+/*
+ * Simple Display Pipe
+ */
 
-static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
-				 struct drm_display_mode *mode)
+static enum drm_mode_status
+mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+				       const struct drm_display_mode *mode)
 {
-	struct drm_device *dev = connector->dev;
-	struct mga_device *mdev = to_mga_device(dev);
-	int bpp = 32;
+	struct mga_device *mdev = to_mga_device(pipe->crtc.dev);
 
 	if (IS_G200_SE(mdev)) {
 		u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
@@ -722,42 +711,17 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 				return MODE_VIRTUAL_X;
 			if (mode->vdisplay > 1200)
 				return MODE_VIRTUAL_Y;
-			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
-				> (24400 * 1024))
-				return MODE_BANDWIDTH;
 		} else if (unique_rev_id == 0x02) {
 			if (mode->hdisplay > 1920)
 				return MODE_VIRTUAL_X;
 			if (mode->vdisplay > 1200)
 				return MODE_VIRTUAL_Y;
-			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
-				> (30100 * 1024))
-				return MODE_BANDWIDTH;
-		} else {
-			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
-				> (55000 * 1024))
-				return MODE_BANDWIDTH;
 		}
 	} else if (mdev->type == G200_WB) {
 		if (mode->hdisplay > 1280)
 			return MODE_VIRTUAL_X;
 		if (mode->vdisplay > 1024)
 			return MODE_VIRTUAL_Y;
-		if (mga_vga_calculate_mode_bandwidth(mode, bpp) >
-		    (31877 * 1024))
-			return MODE_BANDWIDTH;
-	} else if (mdev->type == G200_EV &&
-		(mga_vga_calculate_mode_bandwidth(mode, bpp)
-			> (32700 * 1024))) {
-		return MODE_BANDWIDTH;
-	} else if (mdev->type == G200_EH &&
-		(mga_vga_calculate_mode_bandwidth(mode, bpp)
-			> (37500 * 1024))) {
-		return MODE_BANDWIDTH;
-	} else if (mdev->type == G200_ER &&
-		(mga_vga_calculate_mode_bandwidth(mode,
-			bpp) > (55000 * 1024))) {
-		return MODE_BANDWIDTH;
 	}
 
 	if ((mode->hdisplay % 8) != 0 || (mode->hsync_start % 8) != 0 ||
@@ -775,30 +739,6 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
-static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
-	.get_modes  = mgag200_vga_connector_helper_get_modes,
-	.mode_valid = mga_vga_mode_valid,
-};
-
-static const struct drm_connector_funcs mga_vga_connector_funcs = {
-	.reset                  = drm_atomic_helper_connector_reset,
-	.fill_modes             = drm_helper_probe_single_connector_modes,
-	.destroy                = drm_connector_cleanup,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
-};
-
-/*
- * Simple Display Pipe
- */
-
-static enum drm_mode_status
-mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
-				       const struct drm_display_mode *mode)
-{
-	return MODE_OK;
-}
-
 static void
 mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
 		      struct drm_rect *clip, const struct iosys_map *map)
@@ -1009,6 +949,31 @@ static const uint64_t mgag200_simple_display_pipe_fmtmods[] = {
  * Mode config
  */
 
+/* Calculates a mode's required memory bandwidth (in KiB/sec). */
+static uint32_t mgag200_calculate_mode_bandwidth(const struct drm_display_mode *mode,
+						 unsigned int bits_per_pixel)
+{
+	uint32_t total_area, divisor;
+	uint64_t active_area, pixels_per_second, bandwidth;
+	uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
+
+	divisor = 1024;
+
+	if (!mode->htotal || !mode->vtotal || !mode->clock)
+		return 0;
+
+	active_area = mode->hdisplay * mode->vdisplay;
+	total_area = mode->htotal * mode->vtotal;
+
+	pixels_per_second = active_area * mode->clock * 1000;
+	do_div(pixels_per_second, total_area);
+
+	bandwidth = pixels_per_second * bytes_per_pixel * 100;
+	do_div(bandwidth, divisor);
+
+	return (uint32_t)bandwidth;
+}
+
 static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *dev,
 							   const struct drm_display_mode *mode)
 {
@@ -1024,6 +989,33 @@ static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *de
 	if (fbpages > max_fbpages)
 		return MODE_MEM;
 
+	if (IS_G200_SE(mdev)) {
+		u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
+
+		if (unique_rev_id == 0x01) {
+			if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (24400 * 1024))
+				return MODE_BAD;
+		} else if (unique_rev_id == 0x02) {
+			if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (30100 * 1024))
+				return MODE_BAD;
+		} else {
+			if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (55000 * 1024))
+				return MODE_BAD;
+		}
+	} else if (mdev->type == G200_WB) {
+		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (31877 * 1024))
+			return MODE_BAD;
+	} else if (mdev->type == G200_EV) {
+		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (32700 * 1024))
+			return MODE_BAD;
+	} else if (mdev->type == G200_EH) {
+		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (37500 * 1024))
+			return MODE_BAD;
+	} else if (mdev->type == G200_ER) {
+		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (55000 * 1024))
+			return MODE_BAD;
+	}
+
 	return MODE_OK;
 }
 
-- 
2.36.0

Re: [PATCH 7/7] drm/mgag200: Split up connector's mode_valid helper

[Jocelyn Falempe]

On 09/05/2022 12:35, Thomas Zimmermann wrote:
> Split up the connector's mode_valid helper into a simple-pipe and a
> mode-config helper. The simple-pipe helper tests for display-size
> limits while the mode-config helper tests for memory-bandwidth limits.
> 
> Also add the mgag200_ prefix to mga_vga_calculate_mode_bandwidth() and
> comment on the function's purpose.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 146 ++++++++++++-------------
>   1 file changed, 69 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 92d3ae9489f0..a808827d7a2c 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -681,38 +681,27 @@ static int mgag200_vga_connector_helper_get_modes(struct drm_connector *connecto
>   	return ret;
>   }
>   
> -static uint32_t mga_vga_calculate_mode_bandwidth(struct drm_display_mode *mode,
> -							int bits_per_pixel)
> -{
> -	uint32_t total_area, divisor;
> -	uint64_t active_area, pixels_per_second, bandwidth;
> -	uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
> -
> -	divisor = 1024;
> -
> -	if (!mode->htotal || !mode->vtotal || !mode->clock)
> -		return 0;
> -
> -	active_area = mode->hdisplay * mode->vdisplay;
> -	total_area = mode->htotal * mode->vtotal;
> -
> -	pixels_per_second = active_area * mode->clock * 1000;
> -	do_div(pixels_per_second, total_area);
> -
> -	bandwidth = pixels_per_second * bytes_per_pixel * 100;
> -	do_div(bandwidth, divisor);
> +static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
> +	.get_modes  = mgag200_vga_connector_helper_get_modes,
> +};
>   
> -	return (uint32_t)(bandwidth);
> -}
> +static const struct drm_connector_funcs mga_vga_connector_funcs = {
> +	.reset                  = drm_atomic_helper_connector_reset,
> +	.fill_modes             = drm_helper_probe_single_connector_modes,
> +	.destroy                = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> +};
>   
> -#define MODE_BANDWIDTH	MODE_BAD
> +/*
> + * Simple Display Pipe
> + */
>   
> -static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
> -				 struct drm_display_mode *mode)
> +static enum drm_mode_status
> +mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> +				       const struct drm_display_mode *mode)
>   {
> -	struct drm_device *dev = connector->dev;
> -	struct mga_device *mdev = to_mga_device(dev);
> -	int bpp = 32;
> +	struct mga_device *mdev = to_mga_device(pipe->crtc.dev);
>   
>   	if (IS_G200_SE(mdev)) {
>   		u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
> @@ -722,42 +711,17 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
>   				return MODE_VIRTUAL_X;
>   			if (mode->vdisplay > 1200)
>   				return MODE_VIRTUAL_Y;
> -			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
> -				> (24400 * 1024))
> -				return MODE_BANDWIDTH;
>   		} else if (unique_rev_id == 0x02) {
>   			if (mode->hdisplay > 1920)
>   				return MODE_VIRTUAL_X;
>   			if (mode->vdisplay > 1200)
>   				return MODE_VIRTUAL_Y;
> -			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
> -				> (30100 * 1024))
> -				return MODE_BANDWIDTH;
> -		} else {
> -			if (mga_vga_calculate_mode_bandwidth(mode, bpp)
> -				> (55000 * 1024))
> -				return MODE_BANDWIDTH;
>   		}
>   	} else if (mdev->type == G200_WB) {
>   		if (mode->hdisplay > 1280)
>   			return MODE_VIRTUAL_X;
>   		if (mode->vdisplay > 1024)
>   			return MODE_VIRTUAL_Y;
> -		if (mga_vga_calculate_mode_bandwidth(mode, bpp) >
> -		    (31877 * 1024))
> -			return MODE_BANDWIDTH;
> -	} else if (mdev->type == G200_EV &&
> -		(mga_vga_calculate_mode_bandwidth(mode, bpp)
> -			> (32700 * 1024))) {
> -		return MODE_BANDWIDTH;
> -	} else if (mdev->type == G200_EH &&
> -		(mga_vga_calculate_mode_bandwidth(mode, bpp)
> -			> (37500 * 1024))) {
> -		return MODE_BANDWIDTH;
> -	} else if (mdev->type == G200_ER &&
> -		(mga_vga_calculate_mode_bandwidth(mode,
> -			bpp) > (55000 * 1024))) {
> -		return MODE_BANDWIDTH;
>   	}
>   
>   	if ((mode->hdisplay % 8) != 0 || (mode->hsync_start % 8) != 0 ||
> @@ -775,30 +739,6 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
>   	return MODE_OK;
>   }
>   
> -static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
> -	.get_modes  = mgag200_vga_connector_helper_get_modes,
> -	.mode_valid = mga_vga_mode_valid,
> -};
> -
> -static const struct drm_connector_funcs mga_vga_connector_funcs = {
> -	.reset                  = drm_atomic_helper_connector_reset,
> -	.fill_modes             = drm_helper_probe_single_connector_modes,
> -	.destroy                = drm_connector_cleanup,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -/*
> - * Simple Display Pipe
> - */
> -
> -static enum drm_mode_status
> -mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> -				       const struct drm_display_mode *mode)
> -{
> -	return MODE_OK;
> -}
> -
>   static void
>   mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
>   		      struct drm_rect *clip, const struct iosys_map *map)
> @@ -1009,6 +949,31 @@ static const uint64_t mgag200_simple_display_pipe_fmtmods[] = {
>    * Mode config
>    */
>   
> +/* Calculates a mode's required memory bandwidth (in KiB/sec). */
> +static uint32_t mgag200_calculate_mode_bandwidth(const struct drm_display_mode *mode,
> +						 unsigned int bits_per_pixel)
> +{
> +	uint32_t total_area, divisor;
> +	uint64_t active_area, pixels_per_second, bandwidth;
> +	uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
> +
> +	divisor = 1024;
> +
> +	if (!mode->htotal || !mode->vtotal || !mode->clock)
> +		return 0;
> +
> +	active_area = mode->hdisplay * mode->vdisplay;
> +	total_area = mode->htotal * mode->vtotal;
> +
> +	pixels_per_second = active_area * mode->clock * 1000;
> +	do_div(pixels_per_second, total_area);
> +
> +	bandwidth = pixels_per_second * bytes_per_pixel * 100;
> +	do_div(bandwidth, divisor);
> +
> +	return (uint32_t)bandwidth;
> +}
> +
>   static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *dev,
>   							   const struct drm_display_mode *mode)
>   {
> @@ -1024,6 +989,33 @@ static enum drm_mode_status mgag200_mode_config_mode_valid(struct drm_device *de
>   	if (fbpages > max_fbpages)
>   		return MODE_MEM;
>   
> +	if (IS_G200_SE(mdev)) {
> +		u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
> +
> +		if (unique_rev_id == 0x01) {
> +			if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (24400 * 1024))
> +				return MODE_BAD;
> +		} else if (unique_rev_id == 0x02) {
> +			if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (30100 * 1024))
> +				return MODE_BAD;
> +		} else {
> +			if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (55000 * 1024))
> +				return MODE_BAD;
> +		}
> +	} else if (mdev->type == G200_WB) {
> +		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (31877 * 1024))
> +			return MODE_BAD;
> +	} else if (mdev->type == G200_EV) {
> +		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (32700 * 1024))
> +			return MODE_BAD;
> +	} else if (mdev->type == G200_EH) {
> +		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (37500 * 1024))
> +			return MODE_BAD;
> +	} else if (mdev->type == G200_ER) {
> +		if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > (55000 * 1024))
> +			return MODE_BAD;
> +	}
> +
>   	return MODE_OK;
>   }

One suggestion to avoid too much repetition:

static int mgag200_get_bandwidth_kbps(mdev) {

	if (IS_G200_SE(mdev)) {
		u32 unique_rev_id = mdev->model.g200se.unique_rev_id;

		if (unique_rev_id == 0x01) {
			return 24400;
		} else if (unique_rev_id == 0x02) {
			return 30100;
	...

	} else if (mdev->type == G200_ER) {
		return 55000;
	}
	/* No bandwidth defined */
	return 0;
}

then in mgag200_mode_config_mode_valid()

int g200_bandwidth = mgag200_get_bandwidth_kbps(mdev);

if (g200_bandwidth && mgag200_calculate_mode_bandwidth(mode, max_bpp * 
8) > g200_bandwidth * 1024)
	return MODE_BAD;



I've also tested this patchset, and have seen no regression.

you can add

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>

for the whole series.

-- 

Jocelyn

Re: [PATCH 7/7] drm/mgag200: Split up connector's mode_valid helper

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 12380 bytes --]

Hi

Am 12.05.22 um 12:38 schrieb Jocelyn Falempe:
> On 09/05/2022 12:35, Thomas Zimmermann wrote:
>> Split up the connector's mode_valid helper into a simple-pipe and a
>> mode-config helper. The simple-pipe helper tests for display-size
>> limits while the mode-config helper tests for memory-bandwidth limits.
>>
>> Also add the mgag200_ prefix to mga_vga_calculate_mode_bandwidth() and
>> comment on the function's purpose.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 146 ++++++++++++-------------
>>   1 file changed, 69 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 92d3ae9489f0..a808827d7a2c 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -681,38 +681,27 @@ static int 
>> mgag200_vga_connector_helper_get_modes(struct drm_connector *connecto
>>       return ret;
>>   }
>> -static uint32_t mga_vga_calculate_mode_bandwidth(struct 
>> drm_display_mode *mode,
>> -                            int bits_per_pixel)
>> -{
>> -    uint32_t total_area, divisor;
>> -    uint64_t active_area, pixels_per_second, bandwidth;
>> -    uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
>> -
>> -    divisor = 1024;
>> -
>> -    if (!mode->htotal || !mode->vtotal || !mode->clock)
>> -        return 0;
>> -
>> -    active_area = mode->hdisplay * mode->vdisplay;
>> -    total_area = mode->htotal * mode->vtotal;
>> -
>> -    pixels_per_second = active_area * mode->clock * 1000;
>> -    do_div(pixels_per_second, total_area);
>> -
>> -    bandwidth = pixels_per_second * bytes_per_pixel * 100;
>> -    do_div(bandwidth, divisor);
>> +static const struct drm_connector_helper_funcs 
>> mga_vga_connector_helper_funcs = {
>> +    .get_modes  = mgag200_vga_connector_helper_get_modes,
>> +};
>> -    return (uint32_t)(bandwidth);
>> -}
>> +static const struct drm_connector_funcs mga_vga_connector_funcs = {
>> +    .reset                  = drm_atomic_helper_connector_reset,
>> +    .fill_modes             = drm_helper_probe_single_connector_modes,
>> +    .destroy                = drm_connector_cleanup,
>> +    .atomic_duplicate_state = 
>> drm_atomic_helper_connector_duplicate_state,
>> +    .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
>> +};
>> -#define MODE_BANDWIDTH    MODE_BAD
>> +/*
>> + * Simple Display Pipe
>> + */
>> -static enum drm_mode_status mga_vga_mode_valid(struct drm_connector 
>> *connector,
>> -                 struct drm_display_mode *mode)
>> +static enum drm_mode_status
>> +mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe 
>> *pipe,
>> +                       const struct drm_display_mode *mode)
>>   {
>> -    struct drm_device *dev = connector->dev;
>> -    struct mga_device *mdev = to_mga_device(dev);
>> -    int bpp = 32;
>> +    struct mga_device *mdev = to_mga_device(pipe->crtc.dev);
>>       if (IS_G200_SE(mdev)) {
>>           u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
>> @@ -722,42 +711,17 @@ static enum drm_mode_status 
>> mga_vga_mode_valid(struct drm_connector *connector,
>>                   return MODE_VIRTUAL_X;
>>               if (mode->vdisplay > 1200)
>>                   return MODE_VIRTUAL_Y;
>> -            if (mga_vga_calculate_mode_bandwidth(mode, bpp)
>> -                > (24400 * 1024))
>> -                return MODE_BANDWIDTH;
>>           } else if (unique_rev_id == 0x02) {
>>               if (mode->hdisplay > 1920)
>>                   return MODE_VIRTUAL_X;
>>               if (mode->vdisplay > 1200)
>>                   return MODE_VIRTUAL_Y;
>> -            if (mga_vga_calculate_mode_bandwidth(mode, bpp)
>> -                > (30100 * 1024))
>> -                return MODE_BANDWIDTH;
>> -        } else {
>> -            if (mga_vga_calculate_mode_bandwidth(mode, bpp)
>> -                > (55000 * 1024))
>> -                return MODE_BANDWIDTH;
>>           }
>>       } else if (mdev->type == G200_WB) {
>>           if (mode->hdisplay > 1280)
>>               return MODE_VIRTUAL_X;
>>           if (mode->vdisplay > 1024)
>>               return MODE_VIRTUAL_Y;
>> -        if (mga_vga_calculate_mode_bandwidth(mode, bpp) >
>> -            (31877 * 1024))
>> -            return MODE_BANDWIDTH;
>> -    } else if (mdev->type == G200_EV &&
>> -        (mga_vga_calculate_mode_bandwidth(mode, bpp)
>> -            > (32700 * 1024))) {
>> -        return MODE_BANDWIDTH;
>> -    } else if (mdev->type == G200_EH &&
>> -        (mga_vga_calculate_mode_bandwidth(mode, bpp)
>> -            > (37500 * 1024))) {
>> -        return MODE_BANDWIDTH;
>> -    } else if (mdev->type == G200_ER &&
>> -        (mga_vga_calculate_mode_bandwidth(mode,
>> -            bpp) > (55000 * 1024))) {
>> -        return MODE_BANDWIDTH;
>>       }
>>       if ((mode->hdisplay % 8) != 0 || (mode->hsync_start % 8) != 0 ||
>> @@ -775,30 +739,6 @@ static enum drm_mode_status 
>> mga_vga_mode_valid(struct drm_connector *connector,
>>       return MODE_OK;
>>   }
>> -static const struct drm_connector_helper_funcs 
>> mga_vga_connector_helper_funcs = {
>> -    .get_modes  = mgag200_vga_connector_helper_get_modes,
>> -    .mode_valid = mga_vga_mode_valid,
>> -};
>> -
>> -static const struct drm_connector_funcs mga_vga_connector_funcs = {
>> -    .reset                  = drm_atomic_helper_connector_reset,
>> -    .fill_modes             = drm_helper_probe_single_connector_modes,
>> -    .destroy                = drm_connector_cleanup,
>> -    .atomic_duplicate_state = 
>> drm_atomic_helper_connector_duplicate_state,
>> -    .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
>> -};
>> -
>> -/*
>> - * Simple Display Pipe
>> - */
>> -
>> -static enum drm_mode_status
>> -mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe 
>> *pipe,
>> -                       const struct drm_display_mode *mode)
>> -{
>> -    return MODE_OK;
>> -}
>> -
>>   static void
>>   mgag200_handle_damage(struct mga_device *mdev, struct 
>> drm_framebuffer *fb,
>>                 struct drm_rect *clip, const struct iosys_map *map)
>> @@ -1009,6 +949,31 @@ static const uint64_t 
>> mgag200_simple_display_pipe_fmtmods[] = {
>>    * Mode config
>>    */
>> +/* Calculates a mode's required memory bandwidth (in KiB/sec). */
>> +static uint32_t mgag200_calculate_mode_bandwidth(const struct 
>> drm_display_mode *mode,
>> +                         unsigned int bits_per_pixel)
>> +{
>> +    uint32_t total_area, divisor;
>> +    uint64_t active_area, pixels_per_second, bandwidth;
>> +    uint64_t bytes_per_pixel = (bits_per_pixel + 7) / 8;
>> +
>> +    divisor = 1024;
>> +
>> +    if (!mode->htotal || !mode->vtotal || !mode->clock)
>> +        return 0;
>> +
>> +    active_area = mode->hdisplay * mode->vdisplay;
>> +    total_area = mode->htotal * mode->vtotal;
>> +
>> +    pixels_per_second = active_area * mode->clock * 1000;
>> +    do_div(pixels_per_second, total_area);
>> +
>> +    bandwidth = pixels_per_second * bytes_per_pixel * 100;
>> +    do_div(bandwidth, divisor);
>> +
>> +    return (uint32_t)bandwidth;
>> +}
>> +
>>   static enum drm_mode_status mgag200_mode_config_mode_valid(struct 
>> drm_device *dev,
>>                                  const struct drm_display_mode *mode)
>>   {
>> @@ -1024,6 +989,33 @@ static enum drm_mode_status 
>> mgag200_mode_config_mode_valid(struct drm_device *de
>>       if (fbpages > max_fbpages)
>>           return MODE_MEM;
>> +    if (IS_G200_SE(mdev)) {
>> +        u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
>> +
>> +        if (unique_rev_id == 0x01) {
>> +            if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (24400 * 1024))
>> +                return MODE_BAD;
>> +        } else if (unique_rev_id == 0x02) {
>> +            if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (30100 * 1024))
>> +                return MODE_BAD;
>> +        } else {
>> +            if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (55000 * 1024))
>> +                return MODE_BAD;
>> +        }
>> +    } else if (mdev->type == G200_WB) {
>> +        if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (31877 * 1024))
>> +            return MODE_BAD;
>> +    } else if (mdev->type == G200_EV) {
>> +        if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (32700 * 1024))
>> +            return MODE_BAD;
>> +    } else if (mdev->type == G200_EH) {
>> +        if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (37500 * 1024))
>> +            return MODE_BAD;
>> +    } else if (mdev->type == G200_ER) {
>> +        if (mgag200_calculate_mode_bandwidth(mode, max_bpp * 8) > 
>> (55000 * 1024))
>> +            return MODE_BAD;
>> +    }
>> +
>>       return MODE_OK;
>>   }
> 
> One suggestion to avoid too much repetition:
> 
> static int mgag200_get_bandwidth_kbps(mdev) {
> 
>      if (IS_G200_SE(mdev)) {
>          u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
> 
>          if (unique_rev_id == 0x01) {
>              return 24400;
>          } else if (unique_rev_id == 0x02) {
>              return 30100;
>      ...
> 
>      } else if (mdev->type == G200_ER) {
>          return 55000;
>      }
>      /* No bandwidth defined */
>      return 0;
> }
> 
> then in mgag200_mode_config_mode_valid()
> 
> int g200_bandwidth = mgag200_get_bandwidth_kbps(mdev);
> 
> if (g200_bandwidth && mgag200_calculate_mode_bandwidth(mode, max_bpp * 
> 8) > g200_bandwidth * 1024)
>      return MODE_BAD;
> 
> 
> 
> I've also tested this patchset, and have seen no regression.
> 
> you can add
> 
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> for the whole series.
> 

Great! Thank you so much.

Since posting this patchset, I talked to Adam Jackson about why G200SE 
sometimes prefers 16-bit color depth. [1] He told me that some very 
early revisions of the G200SE had only 1.75 MiB. Reducing the bpp 
allowed them to run at 1024x768. He mentioned that this was required to 
pass some sort of conformance test.

As patch 6/7 now hardcodes a requirement for all modes to support 4 bpp, 
these old devices are stuck at 640x480. That should still be enough for 
a server. I don't expect that anyone really cares any longer, but if it 
ever comes up we can relax the requirement again. I'll mention that in 
the commit message before merging the patch.

I'll also wait for your patches to land. So you won't run into merge 
conflicts.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.17.6/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L1053

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

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

Re: [PATCH 7/7] drm/mgag200: Split up connector's mode_valid helper

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1422 bytes --]

Hi

Am 12.05.22 um 12:38 schrieb Jocelyn Falempe:
...
> 
> One suggestion to avoid too much repetition:
> 
> static int mgag200_get_bandwidth_kbps(mdev) {
> 
>      if (IS_G200_SE(mdev)) {
>          u32 unique_rev_id = mdev->model.g200se.unique_rev_id;
> 
>          if (unique_rev_id == 0x01) {
>              return 24400;
>          } else if (unique_rev_id == 0x02) {
>              return 30100;
>      ...
> 
>      } else if (mdev->type == G200_ER) {
>          return 55000;
>      }
>      /* No bandwidth defined */
>      return 0;
> }
> 
> then in mgag200_mode_config_mode_valid()
> 
> int g200_bandwidth = mgag200_get_bandwidth_kbps(mdev);
> 
> if (g200_bandwidth && mgag200_calculate_mode_bandwidth(mode, max_bpp * 
> 8) > g200_bandwidth * 1024)
>      return MODE_BAD;
> 

FYI that code will soon be parameterized via model-specific constants.

Best regards
Thomas

> 
> 
> I've also tested this patchset, and have seen no regression.
> 
> you can add
> 
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> for the whole series.
> 

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

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