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

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

[Thomas Zimmermann]

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

v3:
	* fix commit message in 1/7 to say 'drm/mgag200' (Jocelyn)
v2:
	* comment on memory constraints on early G200SE-A
	* fix connector DDC helper

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     |  36 ++++
 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, 169 insertions(+), 173 deletions(-)

-- 
2.36.1

[PATCH v3 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.

v3:
	* fix commit message to say 'drm/mgag200' (Jocelyn)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 931e3f3a0e99 ("drm/mgag200: Protect concurrent access to I/O registers with lock")
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
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 73d5a4a42b3a..762b9657ae8d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -691,16 +691,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.1

[PATCH v3 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>
Tested-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 762b9657ae8d..b227891d01ec 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -849,8 +849,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.1

[PATCH v3 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.

v2:
	* clear property if EDID is NULL in helper

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_probe_helper.c     | 36 ++++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++---------
 include/drm/drm_probe_helper.h         |  2 ++
 3 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 682359512996..d77f17867195 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -964,3 +964,39 @@ 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 count = 0;
+
+	if (!connector->ddc)
+		return 0;
+
+	edid = drm_get_edid(connector, connector->ddc);
+
+	// clears property if EDID is NULL
+	drm_connector_update_edid_property(connector, edid);
+
+	if (edid) {
+		count = drm_add_edid_modes(connector, edid);
+		kfree(edid);
+	}
+
+	return count;
+}
+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 b227891d01ec..4c0680dd1a78 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -689,26 +689,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;
@@ -828,7 +819,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.1

[PATCH v3 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>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 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 4c0680dd1a78..ae78950e270b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -811,13 +811,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,
@@ -826,7 +819,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,
 };
@@ -836,12 +829,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;
 	}
@@ -851,16 +843,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.1

[PATCH v3 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>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 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 ae78950e270b..0baaec117502 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -824,31 +824,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
  */
@@ -1109,7 +1084,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;
@@ -1132,14 +1108,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.1

[PATCH v3 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>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 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 0baaec117502..a718a20b3a20 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -796,18 +796,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;
 }
 
@@ -1067,9 +1055,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.1

[PATCH v3 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.

The memory-bandwidth tests assume that the display uses 4 bytes per
pixel. The first models of G200SE-A only had 1.75 MiB of VRAM, which
limits these devices to 640x480-32.

v2:
	* note the memory contraints on early G200SE-A

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 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 a718a20b3a20..028c6ba7124c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -705,38 +705,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;
@@ -746,42 +735,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 ||
@@ -799,30 +763,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)
@@ -1055,6 +995,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)
 {
@@ -1070,6 +1035,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.1

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

[Jani Nikula]

On Mon, 16 May 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 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.

When adding drm core code, please do *not* prefix with "drm/mgag200".

I, and I believe many people, totally ignore changes to a lot of
drivers, while trying to closely follow drm core changes.

Going forward, I'll be slightly changing what the .get_modes hook is
supposed to do, so this feels like a bit of a detour to me. Anyone
picking this up is up for another refactor later on. Oh well.


BR,
Jani.

PS. There's a // comment in there.

>
> v2:
> 	* clear property if EDID is NULL in helper
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> Tested-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_probe_helper.c     | 36 ++++++++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++---------
>  include/drm/drm_probe_helper.h         |  2 ++
>  3 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 682359512996..d77f17867195 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -964,3 +964,39 @@ 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 count = 0;
> +
> +	if (!connector->ddc)
> +		return 0;
> +
> +	edid = drm_get_edid(connector, connector->ddc);
> +
> +	// clears property if EDID is NULL
> +	drm_connector_update_edid_property(connector, edid);
> +
> +	if (edid) {
> +		count = drm_add_edid_modes(connector, edid);
> +		kfree(edid);
> +	}
> +
> +	return count;
> +}
> +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 b227891d01ec..4c0680dd1a78 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -689,26 +689,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;
> @@ -828,7 +819,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

-- 
Jani Nikula, Intel Open Source Graphics Center