[PATCH v5 00/13] drm/ast: Detect connector status for VGA and SIL164

[PATCH v5 00/13] drm/ast: Detect connector status for VGA and SIL164

[Thomas Zimmermann]

Detect the status of the VGA or SIL164 conenctor by polling the DDC
channel. Update the status at runtime and flip the BMC status as well.

Patches 1 adds a missing include statement that will be required to
make later patches compile.

Patches 2 and 3 simplify the VGA and SIL164 connectors, such that no
additional data structure is required. These patches have been reviewed
before as part of the series at [1].

Patches 4 to 10 improve the I2C code that is used to retrieve the
monitor's EDID data. It's now fully managed, it acquires the necessary
lock automatically and it is called DDC, which better represents its
purpose than I2C.

Patches 11 to 13 finally implement polling. Patch 11 updates ast's
EDID code to be up-to-date. The helper drm_connector_get_modes() reads
the EDID via DDC and updates the property. No driver code is required.
Patch 12 uses a similar pattern to detect the presence of the monitor
and sets the connector status accordingly. As polling also needs to be
cleaned up, patch 13 adds the necessary helpers to do so.

Tested on AST2500 hardware and BMC output. The BMC connector now also
flips its status correctly at runtime.

[1] https://patchwork.freedesktop.org/series/104547/

v5:
- share implementation in drm_connector_helper_detect_ctx() (Maxime)
- test for DDC presence with drm_probe_ddc() (Maxime, Jani)
- perform managed cleanup of poll thread

Thomas Zimmermann (13):
  drm/ast: Include <linux/of.h> where necessary
  drm/ast: Fail probing if DDC channel could not be initialized
  drm/ast: Remove struct ast_{vga,sil165}_connector
  drm/ast: Allocate instance of struct ast_i2c_chan with managed helpers
  drm/ast: Move DDC code to ast_ddc.{c,h}
  drm/ast: Rename struct ast_i2c_chan to struct ast_ddc
  drm/ast: Pass AST device to ast_ddc_create()
  drm/ast: Store AST device in struct ast_ddc
  drm/ast: Rename struct i2c_algo_bit_data callbacks and their
    parameters
  drm/ast: Acquire I/O-register lock in DDC code
  drm/ast: Use drm_connector_helper_get_modes()
  drm/ast: Implement polling for VGA and SIL164 connectors
  drm/ast: Automatically clean up poll helper

 drivers/gpu/drm/ast/Makefile                 |  10 +-
 drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} | 120 +++++++++------
 drivers/gpu/drm/ast/ast_ddc.h                |  20 +++
 drivers/gpu/drm/ast/ast_drv.c                |   1 +
 drivers/gpu/drm/ast/ast_drv.h                |  39 +----
 drivers/gpu/drm/ast/ast_main.c               |   1 +
 drivers/gpu/drm/ast/ast_mode.c               | 147 +++++--------------
 drivers/gpu/drm/drm_probe_helper.c           |  56 +++++++
 include/drm/drm_probe_helper.h               |   5 +
 9 files changed, 206 insertions(+), 193 deletions(-)
 rename drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} (54%)
 create mode 100644 drivers/gpu/drm/ast/ast_ddc.h

-- 
2.44.0

[PATCH v5 01/13] drm/ast: Include <linux/of.h> where necessary

[Thomas Zimmermann]

Include <linux/of.h> to get of_property_read_u32() in the source
files that need it. Avoids the proxy include via <linux/i2c.h>.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.c  | 1 +
 drivers/gpu/drm/ast/ast_main.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 90bcb1eb9cd94..f8c49ba68e789 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -27,6 +27,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pci.h>
 
 #include <drm/drm_aperture.h>
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 2f3ad5f949fcb..0637abb70361c 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -26,6 +26,7 @@
  * Authors: Dave Airlie <airlied@redhat.com>
  */
 
+#include <linux/of.h>
 #include <linux/pci.h>
 
 #include <drm/drm_atomic_helper.h>
-- 
2.44.0

Re: [v5,01/13] drm/ast: Include <linux/of.h> where necessary

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> Include <linux/of.h> to get of_property_read_u32() in the source
> files that need it. Avoids the proxy include via <linux/i2c.h>.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 02/13] drm/ast: Fail probing if DDC channel could not be initialized

[Thomas Zimmermann]

Expect the hardware to provide a DDC channel. Fail probing if its
initialization fails. Failing to initialize the DDC indicates a
larger problem, so there's no point in continuing.

v4:
	* give a rational in the commit message

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
---
 drivers/gpu/drm/ast/ast_drv.h  |  2 --
 drivers/gpu/drm/ast/ast_i2c.c  |  7 +++--
 drivers/gpu/drm/ast/ast_mode.c | 52 +++++++++++++---------------------
 3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 3be5ccf1f5f4d..cb0e4f332be80 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -160,7 +160,6 @@ struct ast_i2c_chan {
 
 struct ast_vga_connector {
 	struct drm_connector base;
-	struct ast_i2c_chan *i2c;
 };
 
 static inline struct ast_vga_connector *
@@ -171,7 +170,6 @@ to_ast_vga_connector(struct drm_connector *connector)
 
 struct ast_sil164_connector {
 	struct drm_connector base;
-	struct ast_i2c_chan *i2c;
 };
 
 static inline struct ast_sil164_connector *
diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_i2c.c
index e5d3f7121de42..c3046223a4919 100644
--- a/drivers/gpu/drm/ast/ast_i2c.c
+++ b/drivers/gpu/drm/ast/ast_i2c.c
@@ -117,7 +117,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 
 	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
 	if (!i2c)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	i2c->adapter.owner = THIS_MODULE;
 	i2c->adapter.dev.parent = dev->dev;
@@ -142,10 +142,11 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 
 	ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
 	if (ret)
-		return NULL;
+		return ERR_PTR(ret);
+
 	return i2c;
 
 out_kfree:
 	kfree(i2c);
-	return NULL;
+	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index a718646a66b8f..38c31a7283a69 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1345,22 +1345,18 @@ static int ast_crtc_init(struct drm_device *dev)
 
 static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_vga_connector *ast_vga_connector = to_ast_vga_connector(connector);
 	struct drm_device *dev = connector->dev;
 	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	if (!ast_vga_connector->i2c)
-		goto err_drm_connector_update_edid_property;
-
 	/*
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
 	mutex_lock(&ast->modeset_lock);
 
-	edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
+	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
 		goto err_mutex_unlock;
 
@@ -1373,7 +1369,6 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 
 err_mutex_unlock:
 	mutex_unlock(&ast->modeset_lock);
-err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1394,19 +1389,18 @@ static int ast_vga_connector_init(struct drm_device *dev,
 				  struct ast_vga_connector *ast_vga_connector)
 {
 	struct drm_connector *connector = &ast_vga_connector->base;
+	struct ast_i2c_chan *i2c;
 	int ret;
 
-	ast_vga_connector->i2c = ast_i2c_create(dev);
-	if (!ast_vga_connector->i2c)
-		drm_err(dev, "failed to add ddc bus for connector\n");
+	i2c = ast_i2c_create(dev);
+	if (IS_ERR(i2c)) {
+		ret = PTR_ERR(i2c);
+		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+		return ret;
+	}
 
-	if (ast_vga_connector->i2c)
-		ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
-						  DRM_MODE_CONNECTOR_VGA,
-						  &ast_vga_connector->i2c->adapter);
-	else
-		ret = drm_connector_init(dev, connector, &ast_vga_connector_funcs,
-					 DRM_MODE_CONNECTOR_VGA);
+	ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
+					  DRM_MODE_CONNECTOR_VGA, &i2c->adapter);
 	if (ret)
 		return ret;
 
@@ -1451,22 +1445,18 @@ static int ast_vga_output_init(struct ast_device *ast)
 
 static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_sil164_connector *ast_sil164_connector = to_ast_sil164_connector(connector);
 	struct drm_device *dev = connector->dev;
 	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	if (!ast_sil164_connector->i2c)
-		goto err_drm_connector_update_edid_property;
-
 	/*
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
 	mutex_lock(&ast->modeset_lock);
 
-	edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
+	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
 		goto err_mutex_unlock;
 
@@ -1479,7 +1469,6 @@ static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector
 
 err_mutex_unlock:
 	mutex_unlock(&ast->modeset_lock);
-err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1500,19 +1489,18 @@ static int ast_sil164_connector_init(struct drm_device *dev,
 				     struct ast_sil164_connector *ast_sil164_connector)
 {
 	struct drm_connector *connector = &ast_sil164_connector->base;
+	struct ast_i2c_chan *i2c;
 	int ret;
 
-	ast_sil164_connector->i2c = ast_i2c_create(dev);
-	if (!ast_sil164_connector->i2c)
-		drm_err(dev, "failed to add ddc bus for connector\n");
+	i2c = ast_i2c_create(dev);
+	if (IS_ERR(i2c)) {
+		ret = PTR_ERR(i2c);
+		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+		return ret;
+	}
 
-	if (ast_sil164_connector->i2c)
-		ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
-						  DRM_MODE_CONNECTOR_DVII,
-						  &ast_sil164_connector->i2c->adapter);
-	else
-		ret = drm_connector_init(dev, connector, &ast_sil164_connector_funcs,
-					 DRM_MODE_CONNECTOR_DVII);
+	ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
+					  DRM_MODE_CONNECTOR_DVII, &i2c->adapter);
 	if (ret)
 		return ret;
 
-- 
2.44.0

[PATCH v5 03/13] drm/ast: Remove struct ast_{vga,sil165}_connector

[Thomas Zimmermann]

Both, struct ast_vga_connector and struct ast_sil164_connector, are
now wrappers around struct drm_connector. Remove them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
---
 drivers/gpu/drm/ast/ast_drv.h  | 24 ++----------------------
 drivers/gpu/drm/ast/ast_mode.c | 22 ++++++++--------------
 2 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index cb0e4f332be80..12ad1c0fe151b 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -158,26 +158,6 @@ struct ast_i2c_chan {
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_vga_connector {
-	struct drm_connector base;
-};
-
-static inline struct ast_vga_connector *
-to_ast_vga_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct ast_vga_connector, base);
-}
-
-struct ast_sil164_connector {
-	struct drm_connector base;
-};
-
-static inline struct ast_sil164_connector *
-to_ast_sil164_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct ast_sil164_connector, base);
-}
-
 struct ast_bmc_connector {
 	struct drm_connector base;
 	struct drm_connector *physical_connector;
@@ -220,11 +200,11 @@ struct ast_device {
 	struct {
 		struct {
 			struct drm_encoder encoder;
-			struct ast_vga_connector vga_connector;
+			struct drm_connector connector;
 		} vga;
 		struct {
 			struct drm_encoder encoder;
-			struct ast_sil164_connector sil164_connector;
+			struct drm_connector connector;
 		} sil164;
 		struct {
 			struct drm_encoder encoder;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 38c31a7283a69..cbda04fca7107 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1385,10 +1385,8 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int ast_vga_connector_init(struct drm_device *dev,
-				  struct ast_vga_connector *ast_vga_connector)
+static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct drm_connector *connector = &ast_vga_connector->base;
 	struct ast_i2c_chan *i2c;
 	int ret;
 
@@ -1419,8 +1417,7 @@ static int ast_vga_output_init(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	struct drm_crtc *crtc = &ast->crtc;
 	struct drm_encoder *encoder = &ast->output.vga.encoder;
-	struct ast_vga_connector *ast_vga_connector = &ast->output.vga.vga_connector;
-	struct drm_connector *connector = &ast_vga_connector->base;
+	struct drm_connector *connector = &ast->output.vga.connector;
 	int ret;
 
 	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
@@ -1428,7 +1425,7 @@ static int ast_vga_output_init(struct ast_device *ast)
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	ret = ast_vga_connector_init(dev, ast_vga_connector);
+	ret = ast_vga_connector_init(dev, connector);
 	if (ret)
 		return ret;
 
@@ -1485,10 +1482,8 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int ast_sil164_connector_init(struct drm_device *dev,
-				     struct ast_sil164_connector *ast_sil164_connector)
+static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct drm_connector *connector = &ast_sil164_connector->base;
 	struct ast_i2c_chan *i2c;
 	int ret;
 
@@ -1519,8 +1514,7 @@ static int ast_sil164_output_init(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	struct drm_crtc *crtc = &ast->crtc;
 	struct drm_encoder *encoder = &ast->output.sil164.encoder;
-	struct ast_sil164_connector *ast_sil164_connector = &ast->output.sil164.sil164_connector;
-	struct drm_connector *connector = &ast_sil164_connector->base;
+	struct drm_connector *connector = &ast->output.sil164.connector;
 	int ret;
 
 	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
@@ -1528,7 +1522,7 @@ static int ast_sil164_output_init(struct ast_device *ast)
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	ret = ast_sil164_connector_init(dev, ast_sil164_connector);
+	ret = ast_sil164_connector_init(dev, connector);
 	if (ret)
 		return ret;
 
@@ -1940,13 +1934,13 @@ int ast_mode_config_init(struct ast_device *ast)
 		ret = ast_vga_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.vga.vga_connector.base;
+		physical_connector = &ast->output.vga.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_SIL164_BIT) {
 		ret = ast_sil164_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.sil164.sil164_connector.base;
+		physical_connector = &ast->output.sil164.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_DP501_BIT) {
 		ret = ast_dp501_output_init(ast);
-- 
2.44.0

[PATCH v5 04/13] drm/ast: Allocate instance of struct ast_i2c_chan with managed helpers

[Thomas Zimmermann]

Replace kzalloc() with drmm_kzalloc() and thereby put the release of
the I2C instance into a separate action. Avoids explicit error roll-
back in ast_i2c_chan_create(). No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_i2c.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_i2c.c
index c3046223a4919..dc28a5cbb1c2a 100644
--- a/drivers/gpu/drm/ast/ast_i2c.c
+++ b/drivers/gpu/drm/ast/ast_i2c.c
@@ -107,7 +107,6 @@ static void ast_i2c_release(struct drm_device *dev, void *res)
 	struct ast_i2c_chan *i2c = res;
 
 	i2c_del_adapter(&i2c->adapter);
-	kfree(i2c);
 }
 
 struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
@@ -115,7 +114,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	struct ast_i2c_chan *i2c;
 	int ret;
 
-	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
+	i2c = drmm_kzalloc(dev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return ERR_PTR(-ENOMEM);
 
@@ -137,7 +136,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	ret = i2c_bit_add_bus(&i2c->adapter);
 	if (ret) {
 		drm_err(dev, "Failed to register bit i2c\n");
-		goto out_kfree;
+		return ERR_PTR(ret);
 	}
 
 	ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
@@ -145,8 +144,4 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 		return ERR_PTR(ret);
 
 	return i2c;
-
-out_kfree:
-	kfree(i2c);
-	return ERR_PTR(ret);
 }
-- 
2.44.0

Re: [v5, 04/13] drm/ast: Allocate instance of struct ast_i2c_chan with managed helpers

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> Replace kzalloc() with drmm_kzalloc() and thereby put the release of
> the I2C instance into a separate action. Avoids explicit error roll-
> back in ast_i2c_chan_create(). No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 05/13] drm/ast: Move DDC code to ast_ddc.{c,h}

[Thomas Zimmermann]

Rename ast_i2c.c to ast_ddc.c and move its interface into the
new header ast_ddc.h. Update all include statements as necessary
and change the adapter name to 'AST DDC bus'.

This avoids including I2C headers in the driver's main header file,
which doesn't need them. Renaming files to _ddc indicates that the
code is about the DDC. I2C is really just the underlying bus here.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/Makefile                 | 10 +++++++++-
 drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} |  4 ++--
 drivers/gpu/drm/ast/ast_ddc.h                | 19 +++++++++++++++++++
 drivers/gpu/drm/ast/ast_drv.h                | 13 +------------
 drivers/gpu/drm/ast/ast_mode.c               |  1 +
 5 files changed, 32 insertions(+), 15 deletions(-)
 rename drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} (97%)
 create mode 100644 drivers/gpu/drm/ast/ast_ddc.h

diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
index 5a53ce51fb249..d794c076bc242 100644
--- a/drivers/gpu/drm/ast/Makefile
+++ b/drivers/gpu/drm/ast/Makefile
@@ -3,6 +3,14 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
-ast-y := ast_drv.o ast_i2c.o ast_main.o ast_mm.o ast_mode.o ast_post.o ast_dp501.o ast_dp.o
+ast-y := \
+	ast_ddc.o \
+	ast_dp501.o \
+	ast_dp.o \
+	ast_drv.o \
+	ast_main.o \
+	ast_mm.o \
+	ast_mode.o \
+	ast_post.o
 
 obj-$(CONFIG_DRM_AST) := ast.o
diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_ddc.c
similarity index 97%
rename from drivers/gpu/drm/ast/ast_i2c.c
rename to drivers/gpu/drm/ast/ast_ddc.c
index dc28a5cbb1c2a..df604b4e9673c 100644
--- a/drivers/gpu/drm/ast/ast_i2c.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -24,6 +24,7 @@
 #include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 
+#include "ast_ddc.h"
 #include "ast_drv.h"
 
 static void ast_i2c_setsda(void *i2c_priv, int data)
@@ -122,8 +123,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	i2c->adapter.dev.parent = dev->dev;
 	i2c->dev = dev;
 	i2c_set_adapdata(&i2c->adapter, i2c);
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
-		 "AST i2c bit bus");
+	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "AST DDC bus");
 	i2c->adapter.algo_data = &i2c->bit;
 
 	i2c->bit.udelay = 20;
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
new file mode 100644
index 0000000000000..244666fd6c530
--- /dev/null
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __AST_DDC_H__
+#define __AST_DDC_H__
+
+#include <linux/i2c.h>
+#include <linux/i2c-algo-bit.h>
+
+struct drm_device;
+
+struct ast_i2c_chan {
+	struct i2c_adapter adapter;
+	struct drm_device *dev;
+	struct i2c_algo_bit_data bit;
+};
+
+struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
+
+#endif
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 12ad1c0fe151b..ba3d86973995f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -28,8 +28,6 @@
 #ifndef __AST_DRV_H__
 #define __AST_DRV_H__
 
-#include <linux/i2c.h>
-#include <linux/i2c-algo-bit.h>
 #include <linux/io.h>
 #include <linux/types.h>
 
@@ -149,15 +147,9 @@ static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
 }
 
 /*
- * Connector with i2c channel
+ * BMC
  */
 
-struct ast_i2c_chan {
-	struct i2c_adapter adapter;
-	struct drm_device *dev;
-	struct i2c_algo_bit_data bit;
-};
-
 struct ast_bmc_connector {
 	struct drm_connector base;
 	struct drm_connector *physical_connector;
@@ -476,9 +468,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
 u8 ast_get_dp501_max_clk(struct drm_device *dev);
 void ast_init_3rdtx(struct drm_device *dev);
 
-/* ast_i2c.c */
-struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
-
 /* aspeed DP */
 bool ast_astdp_is_connected(struct ast_device *ast);
 int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cbda04fca7107..bdef2160726e6 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -46,6 +46,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
+#include "ast_ddc.h"
 #include "ast_drv.h"
 #include "ast_tables.h"
 
-- 
2.44.0

Re: [v5,05/13] drm/ast: Move DDC code to ast_ddc.{c,h}

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> Rename ast_i2c.c to ast_ddc.c and move its interface into the
> new header ast_ddc.h. Update all include statements as necessary
> and change the adapter name to 'AST DDC bus'.
>
> This avoids including I2C headers in the driver's main header file,
> which doesn't need them. Renaming files to _ddc indicates that the
> code is about the DDC. I2C is really just the underlying bus here.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 06/13] drm/ast: Rename struct ast_i2c_chan to struct ast_ddc

[Thomas Zimmermann]

The struct struct ast_i2c_chan represents the Display Data Channel
(DDC); I2C is the underlying bus. Rename the structure, the variables
and the helper ast_i2c_create() to ddc-like terms. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c  | 71 ++++++++++++++++++----------------
 drivers/gpu/drm/ast/ast_ddc.h  |  4 +-
 drivers/gpu/drm/ast/ast_mode.c | 24 ++++++------
 3 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index df604b4e9673c..c0e5d03c028d8 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -29,8 +29,8 @@
 
 static void ast_i2c_setsda(void *i2c_priv, int data)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -45,8 +45,8 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 
 static void ast_i2c_setscl(void *i2c_priv, int clock)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -61,8 +61,8 @@ static void ast_i2c_setscl(void *i2c_priv, int clock)
 
 static int ast_i2c_getsda(void *i2c_priv)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -83,8 +83,8 @@ static int ast_i2c_getsda(void *i2c_priv)
 
 static int ast_i2c_getscl(void *i2c_priv)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -103,45 +103,50 @@ static int ast_i2c_getscl(void *i2c_priv)
 	return val & 1 ? 1 : 0;
 }
 
-static void ast_i2c_release(struct drm_device *dev, void *res)
+static void ast_ddc_release(struct drm_device *dev, void *res)
 {
-	struct ast_i2c_chan *i2c = res;
+	struct ast_ddc *ddc = res;
 
-	i2c_del_adapter(&i2c->adapter);
+	i2c_del_adapter(&ddc->adapter);
 }
 
-struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
+struct ast_ddc *ast_ddc_create(struct drm_device *dev)
 {
-	struct ast_i2c_chan *i2c;
+	struct ast_ddc *ddc;
+	struct i2c_adapter *adapter;
+	struct i2c_algo_bit_data *bit;
 	int ret;
 
-	i2c = drmm_kzalloc(dev->dev, sizeof(*i2c), GFP_KERNEL);
-	if (!i2c)
+	ddc = drmm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
+	if (!ddc)
 		return ERR_PTR(-ENOMEM);
-
-	i2c->adapter.owner = THIS_MODULE;
-	i2c->adapter.dev.parent = dev->dev;
-	i2c->dev = dev;
-	i2c_set_adapdata(&i2c->adapter, i2c);
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "AST DDC bus");
-	i2c->adapter.algo_data = &i2c->bit;
-
-	i2c->bit.udelay = 20;
-	i2c->bit.timeout = 2;
-	i2c->bit.data = i2c;
-	i2c->bit.setsda = ast_i2c_setsda;
-	i2c->bit.setscl = ast_i2c_setscl;
-	i2c->bit.getsda = ast_i2c_getsda;
-	i2c->bit.getscl = ast_i2c_getscl;
-	ret = i2c_bit_add_bus(&i2c->adapter);
+	ddc->dev = dev;
+
+	adapter = &ddc->adapter;
+	adapter->owner = THIS_MODULE;
+	adapter->dev.parent = dev->dev;
+	i2c_set_adapdata(adapter, ddc);
+	snprintf(adapter->name, sizeof(adapter->name), "AST DDC bus");
+
+	bit = &ddc->bit;
+	bit->udelay = 20;
+	bit->timeout = 2;
+	bit->data = ddc;
+	bit->setsda = ast_i2c_setsda;
+	bit->setscl = ast_i2c_setscl;
+	bit->getsda = ast_i2c_getsda;
+	bit->getscl = ast_i2c_getscl;
+
+	adapter->algo_data = bit;
+	ret = i2c_bit_add_bus(adapter);
 	if (ret) {
 		drm_err(dev, "Failed to register bit i2c\n");
 		return ERR_PTR(ret);
 	}
 
-	ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
+	ret = drmm_add_action_or_reset(dev, ast_ddc_release, ddc);
 	if (ret)
 		return ERR_PTR(ret);
 
-	return i2c;
+	return ddc;
 }
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
index 244666fd6c530..071f9be3e27de 100644
--- a/drivers/gpu/drm/ast/ast_ddc.h
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -8,12 +8,12 @@
 
 struct drm_device;
 
-struct ast_i2c_chan {
+struct ast_ddc {
 	struct i2c_adapter adapter;
 	struct drm_device *dev;
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
+struct ast_ddc *ast_ddc_create(struct drm_device *dev);
 
 #endif
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index bdef2160726e6..40cb495acc908 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1388,18 +1388,18 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = {
 
 static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct ast_i2c_chan *i2c;
+	struct ast_ddc *ddc;
 	int ret;
 
-	i2c = ast_i2c_create(dev);
-	if (IS_ERR(i2c)) {
-		ret = PTR_ERR(i2c);
-		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+	ddc = ast_ddc_create(dev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
+		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA, &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, &ddc->adapter);
 	if (ret)
 		return ret;
 
@@ -1485,18 +1485,18 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = {
 
 static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct ast_i2c_chan *i2c;
+	struct ast_ddc *ddc;
 	int ret;
 
-	i2c = ast_i2c_create(dev);
-	if (IS_ERR(i2c)) {
-		ret = PTR_ERR(i2c);
-		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+	ddc = ast_ddc_create(dev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
+		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
-					  DRM_MODE_CONNECTOR_DVII, &i2c->adapter);
+					  DRM_MODE_CONNECTOR_DVII, &ddc->adapter);
 	if (ret)
 		return ret;
 
-- 
2.44.0

Re: [v5,06/13] drm/ast: Rename struct ast_i2c_chan to struct ast_ddc

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> The struct struct ast_i2c_chan represents the Display Data Channel
> (DDC); I2C is the underlying bus. Rename the structure, the variables
> and the helper ast_i2c_create() to ddc-like terms. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Personally, I think _i2c_ is better name than _ddc_. Because It seems that
the Display Data Channel is comes from VESA standard, while aspeed I2C can
be used to configure the external display bridges(encoder). So _i2c_ is a
*abstract* name, and can be utilized to do something else beyond the DDC
itself.


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

Re: [v5,06/13] drm/ast: Rename struct ast_i2c_chan to struct ast_ddc

[Thomas Zimmermann]

Hi

Am 20.03.24 um 18:32 schrieb Sui Jingfeng:
> Hi,
>
>
> On 2024/3/20 17:34, Thomas Zimmermann wrote:
>> The struct struct ast_i2c_chan represents the Display Data Channel
>> (DDC); I2C is the underlying bus. Rename the structure, the variables
>> and the helper ast_i2c_create() to ddc-like terms. No functional
>> changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>
>
> Personally, I think _i2c_ is better name than _ddc_. Because It seems 
> that
> the Display Data Channel is comes from VESA standard, while aspeed I2C 
> can
> be used to configure the external display bridges(encoder). So _i2c_ is a
> *abstract* name, and can be utilized to do something else beyond the DDC
> itself.

This specific instance of i2c is for DDC; even the AST manual refers to 
it as DDC. I don't think there's anything else we could do with those 
registers. If we have other uses for i2c, we can easily add them in 
separate code.

>
>
> Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Thanks for reviewing the series. I think the new helper in patch 12 
could be useful for loongson [1][2] as well.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/loongson/lsdc_output_7a1000.c#L80
[2] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/loongson/lsdc_output_7a2000.c#L177

-- 
--
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)

Re: [v5,06/13] drm/ast: Rename struct ast_i2c_chan to struct ast_ddc

[Sui Jingfeng]

Hi,


On 2024/3/21 16:13, Thomas Zimmermann wrote:
> Hi
>
> Am 20.03.24 um 18:32 schrieb Sui Jingfeng:
>> Hi,
>>
>>
>> On 2024/3/20 17:34, Thomas Zimmermann wrote:
>>> The struct struct ast_i2c_chan represents the Display Data Channel
>>> (DDC); I2C is the underlying bus. Rename the structure, the variables
>>> and the helper ast_i2c_create() to ddc-like terms. No functional
>>> changes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>>
>> Personally, I think _i2c_ is better name than _ddc_. Because It seems 
>> that
>> the Display Data Channel is comes from VESA standard, while aspeed 
>> I2C can
>> be used to configure the external display bridges(encoder). So _i2c_ 
>> is a
>> *abstract* name, and can be utilized to do something else beyond the DDC
>> itself.
>
> This specific instance of i2c is for DDC; even the AST manual refers 
> to it as DDC. I don't think there's anything else we could do with 
> those registers. If we have other uses for i2c, we can easily add them 
> in separate code.
>
>>
>>
>> Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>
> Thanks for reviewing the series. I think the new helper in patch 12 
> could be useful for loongson [1][2] as well.
>

Yes, after this series landed. I'm going to post a few patch to remove some boilerplate.
I want to follow as well, but need a period of time. I have tested the whole series with
ast 2600 dGPU, the /sys/class/.../status will reflect the physical hut-plug status.


> Best regards
> Thomas
>
> [1] 
> https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/loongson/lsdc_output_7a1000.c#L80
> [2] 
> https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/loongson/lsdc_output_7a2000.c#L177
>

-- 
Best regards,
Sui

[PATCH v5 07/13] drm/ast: Pass AST device to ast_ddc_create()

[Thomas Zimmermann]

The DDC code needs the AST device. Pass it to ast_ddc_create() and
avoid an internal upcast. Improves type safety within the DDC code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c  | 3 ++-
 drivers/gpu/drm/ast/ast_ddc.h  | 3 ++-
 drivers/gpu/drm/ast/ast_mode.c | 6 ++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index c0e5d03c028d8..24b7d589f0d4c 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -110,8 +110,9 @@ static void ast_ddc_release(struct drm_device *dev, void *res)
 	i2c_del_adapter(&ddc->adapter);
 }
 
-struct ast_ddc *ast_ddc_create(struct drm_device *dev)
+struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 {
+	struct drm_device *dev = &ast->base;
 	struct ast_ddc *ddc;
 	struct i2c_adapter *adapter;
 	struct i2c_algo_bit_data *bit;
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
index 071f9be3e27de..d423b144a3458 100644
--- a/drivers/gpu/drm/ast/ast_ddc.h
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -6,6 +6,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 
+struct ast_device;
 struct drm_device;
 
 struct ast_ddc {
@@ -14,6 +15,6 @@ struct ast_ddc {
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_ddc *ast_ddc_create(struct drm_device *dev);
+struct ast_ddc *ast_ddc_create(struct ast_device *ast);
 
 #endif
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 40cb495acc908..fc73d3b65b2a1 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1388,10 +1388,11 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = {
 
 static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
+	struct ast_device *ast = to_ast_device(dev);
 	struct ast_ddc *ddc;
 	int ret;
 
-	ddc = ast_ddc_create(dev);
+	ddc = ast_ddc_create(ast);
 	if (IS_ERR(ddc)) {
 		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
@@ -1485,10 +1486,11 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = {
 
 static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
+	struct ast_device *ast = to_ast_device(dev);
 	struct ast_ddc *ddc;
 	int ret;
 
-	ddc = ast_ddc_create(dev);
+	ddc = ast_ddc_create(ast);
 	if (IS_ERR(ddc)) {
 		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
-- 
2.44.0

Re: [v5,07/13] drm/ast: Pass AST device to ast_ddc_create()

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> The DDC code needs the AST device. Pass it to ast_ddc_create() and
> avoid an internal upcast. Improves type safety within the DDC code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 08/13] drm/ast: Store AST device in struct ast_ddc

[Thomas Zimmermann]

The DDC code needs the AST device. Store a pointer in struct ast_ddc
and avoid internal upcasts. Improves type safety within the DDC code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c | 10 +++++-----
 drivers/gpu/drm/ast/ast_ddc.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index 24b7d589f0d4c..47670285fd765 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -30,7 +30,7 @@
 static void ast_i2c_setsda(void *i2c_priv, int data)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -46,7 +46,7 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 static void ast_i2c_setscl(void *i2c_priv, int clock)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -62,7 +62,7 @@ static void ast_i2c_setscl(void *i2c_priv, int clock)
 static int ast_i2c_getsda(void *i2c_priv)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -84,7 +84,7 @@ static int ast_i2c_getsda(void *i2c_priv)
 static int ast_i2c_getscl(void *i2c_priv)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -121,7 +121,7 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 	ddc = drmm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
 	if (!ddc)
 		return ERR_PTR(-ENOMEM);
-	ddc->dev = dev;
+	ddc->ast = ast;
 
 	adapter = &ddc->adapter;
 	adapter->owner = THIS_MODULE;
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
index d423b144a3458..08f3994e09ccd 100644
--- a/drivers/gpu/drm/ast/ast_ddc.h
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -7,11 +7,11 @@
 #include <linux/i2c-algo-bit.h>
 
 struct ast_device;
-struct drm_device;
 
 struct ast_ddc {
+	struct ast_device *ast;
+
 	struct i2c_adapter adapter;
-	struct drm_device *dev;
 	struct i2c_algo_bit_data bit;
 };
 
-- 
2.44.0

Re: [v5,08/13] drm/ast: Store AST device in struct ast_ddc

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> The DDC code needs the AST device. Store a pointer in struct ast_ddc
> and avoid internal upcasts. Improves type safety within the DDC code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 09/13] drm/ast: Rename struct i2c_algo_bit_data callbacks and their parameters

[Thomas Zimmermann]

Align the names of the algo-bit helpers with ast's convention of
using an ast prefix plus the struct's name plus the callback's name
for such function symbols. Change the parameter names of these
helpers to 'data' and 'state', as used in the declaration of struct
i2c_algo_bit_data. No functional changes.

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

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index 47670285fd765..b84e656124f18 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -27,15 +27,15 @@
 #include "ast_ddc.h"
 #include "ast_drv.h"
 
-static void ast_i2c_setsda(void *i2c_priv, int data)
+static void ast_ddc_algo_bit_data_setsda(void *data, int state)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
 	for (i = 0; i < 0x10000; i++) {
-		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
+		ujcrb7 = ((state & 0x01) ? 0 : 1) << 2;
 		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0xf1, ujcrb7);
 		jtemp = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0x04);
 		if (ujcrb7 == jtemp)
@@ -43,15 +43,15 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 	}
 }
 
-static void ast_i2c_setscl(void *i2c_priv, int clock)
+static void ast_ddc_algo_bit_data_setscl(void *data, int state)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
 	for (i = 0; i < 0x10000; i++) {
-		ujcrb7 = ((clock & 0x01) ? 0 : 1);
+		ujcrb7 = ((state & 0x01) ? 0 : 1);
 		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0xf4, ujcrb7);
 		jtemp = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0x01);
 		if (ujcrb7 == jtemp)
@@ -59,9 +59,9 @@ static void ast_i2c_setscl(void *i2c_priv, int clock)
 	}
 }
 
-static int ast_i2c_getsda(void *i2c_priv)
+static int ast_ddc_algo_bit_data_getsda(void *data)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
@@ -81,9 +81,9 @@ static int ast_i2c_getsda(void *i2c_priv)
 	return val & 1 ? 1 : 0;
 }
 
-static int ast_i2c_getscl(void *i2c_priv)
+static int ast_ddc_algo_bit_data_getscl(void *data)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
@@ -133,10 +133,10 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 	bit->udelay = 20;
 	bit->timeout = 2;
 	bit->data = ddc;
-	bit->setsda = ast_i2c_setsda;
-	bit->setscl = ast_i2c_setscl;
-	bit->getsda = ast_i2c_getsda;
-	bit->getscl = ast_i2c_getscl;
+	bit->setsda = ast_ddc_algo_bit_data_setsda;
+	bit->setscl = ast_ddc_algo_bit_data_setscl;
+	bit->getsda = ast_ddc_algo_bit_data_getsda;
+	bit->getscl = ast_ddc_algo_bit_data_getscl;
 
 	adapter->algo_data = bit;
 	ret = i2c_bit_add_bus(adapter);
-- 
2.44.0

Re: [v5, 09/13] drm/ast: Rename struct i2c_algo_bit_data callbacks and their parameters

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> Align the names of the algo-bit helpers with ast's convention of
> using an ast prefix plus the struct's name plus the callback's name
> for such function symbols. Change the parameter names of these
> helpers to 'data' and 'state', as used in the declaration of struct
> i2c_algo_bit_data. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 10/13] drm/ast: Acquire I/O-register lock in DDC code

[Thomas Zimmermann]

The modeset lock protects the DDC code from concurrent modeset
operations, which use the same registers. Move that code from the
connector helpers into the DDC helpers .pre_xfer() and .post_xfer().

Both, .pre_xfer() and .post_xfer(), enclose the transfer of data blocks
over the I2C channel in the internal I2C function bit_xfer(). Both
calls are executed unconditionally if present. Invoking DDC transfers
from any where within the driver now takes the lock.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c  | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/ast/ast_mode.c | 30 ++++--------------------------
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index b84e656124f18..b7718084422f3 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -59,6 +59,28 @@ static void ast_ddc_algo_bit_data_setscl(void *data, int state)
 	}
 }
 
+static int ast_ddc_algo_bit_data_pre_xfer(struct i2c_adapter *adapter)
+{
+	struct ast_ddc *ddc = i2c_get_adapdata(adapter);
+	struct ast_device *ast = ddc->ast;
+
+	/*
+	 * Protect access to I/O registers from concurrent modesetting
+	 * by acquiring the I/O-register lock.
+	 */
+	mutex_lock(&ast->modeset_lock);
+
+	return 0;
+}
+
+static void ast_ddc_algo_bit_data_post_xfer(struct i2c_adapter *adapter)
+{
+	struct ast_ddc *ddc = i2c_get_adapdata(adapter);
+	struct ast_device *ast = ddc->ast;
+
+	mutex_unlock(&ast->modeset_lock);
+}
+
 static int ast_ddc_algo_bit_data_getsda(void *data)
 {
 	struct ast_ddc *ddc = data;
@@ -137,6 +159,8 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 	bit->setscl = ast_ddc_algo_bit_data_setscl;
 	bit->getsda = ast_ddc_algo_bit_data_getsda;
 	bit->getscl = ast_ddc_algo_bit_data_getscl;
+	bit->pre_xfer = ast_ddc_algo_bit_data_pre_xfer;
+	bit->post_xfer = ast_ddc_algo_bit_data_post_xfer;
 
 	adapter->algo_data = bit;
 	ret = i2c_bit_add_bus(adapter);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index fc73d3b65b2a1..8766a0f2eb3c7 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1346,30 +1346,19 @@ static int ast_crtc_init(struct drm_device *dev)
 
 static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	/*
-	 * Protect access to I/O registers from concurrent modesetting
-	 * by acquiring the I/O-register lock.
-	 */
-	mutex_lock(&ast->modeset_lock);
-
 	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
-		goto err_mutex_unlock;
-
-	mutex_unlock(&ast->modeset_lock);
+		goto err_drm_get_edid;
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
 	return count;
 
-err_mutex_unlock:
-	mutex_unlock(&ast->modeset_lock);
+err_drm_get_edid:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1444,30 +1433,19 @@ static int ast_vga_output_init(struct ast_device *ast)
 
 static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	/*
-	 * Protect access to I/O registers from concurrent modesetting
-	 * by acquiring the I/O-register lock.
-	 */
-	mutex_lock(&ast->modeset_lock);
-
 	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
-		goto err_mutex_unlock;
-
-	mutex_unlock(&ast->modeset_lock);
+		goto err_drm_get_edid;
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
 	return count;
 
-err_mutex_unlock:
-	mutex_unlock(&ast->modeset_lock);
+err_drm_get_edid:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
-- 
2.44.0

Re: [v5,10/13] drm/ast: Acquire I/O-register lock in DDC code

[Sui Jingfeng]

Hi,


Tested with ast2600 hardware, no obvious problem found yet.


dmesg  | grep ast

  ast 0000:09:00.0: VGA not enabled on entry, requesting chip POST
  ast 0000:09:00.0: Using default configuration
  ast 0000:09:00.0: AST 2600 detected
  ast 0000:09:00.0: [drm] Using analog VGA
  ast 0000:09:00.0: [drm] dram MCLK=396 Mhz type=1 bus_width=16
  [drm] Initialized ast 0.1.0 20120228 for 0000:09:00.0 on minor 0
  ast 0000:09:00.0: [drm] fb0: astdrmfb frame buffer device


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> The modeset lock protects the DDC code from concurrent modeset
> operations, which use the same registers. Move that code from the
> connector helpers into the DDC helpers .pre_xfer() and .post_xfer().
>
> Both, .pre_xfer() and .post_xfer(), enclose the transfer of data blocks
> over the I2C channel in the internal I2C function bit_xfer(). Both
> calls are executed unconditionally if present. Invoking DDC transfers
> from any where within the driver now takes the lock.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Tested-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 11/13] drm/ast: Use drm_connector_helper_get_modes()

[Thomas Zimmermann]

The .get_modes() code for VGA and SIL164 connectors does not depend
on either type of connector. Replace the driver code with the common
helper drm_connector_helper_get_modes(). It reads EDID data via
DDC and updates the connector's EDID property.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 42 ++--------------------------------
 1 file changed, 2 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 8766a0f2eb3c7..71cc681d6188f 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1344,27 +1344,8 @@ static int ast_crtc_init(struct drm_device *dev)
  * VGA Connector
  */
 
-static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
-{
-	struct edid *edid;
-	int count;
-
-	edid = drm_get_edid(connector, connector->ddc);
-	if (!edid)
-		goto err_drm_get_edid;
-
-	count = drm_add_edid_modes(connector, edid);
-	kfree(edid);
-
-	return count;
-
-err_drm_get_edid:
-	drm_connector_update_edid_property(connector, NULL);
-	return 0;
-}
-
 static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
-	.get_modes = ast_vga_connector_helper_get_modes,
+	.get_modes = drm_connector_helper_get_modes,
 };
 
 static const struct drm_connector_funcs ast_vga_connector_funcs = {
@@ -1431,27 +1412,8 @@ static int ast_vga_output_init(struct ast_device *ast)
  * SIL164 Connector
  */
 
-static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
-{
-	struct edid *edid;
-	int count;
-
-	edid = drm_get_edid(connector, connector->ddc);
-	if (!edid)
-		goto err_drm_get_edid;
-
-	count = drm_add_edid_modes(connector, edid);
-	kfree(edid);
-
-	return count;
-
-err_drm_get_edid:
-	drm_connector_update_edid_property(connector, NULL);
-	return 0;
-}
-
 static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
-	.get_modes = ast_sil164_connector_helper_get_modes,
+	.get_modes = drm_connector_helper_get_modes,
 };
 
 static const struct drm_connector_funcs ast_sil164_connector_funcs = {
-- 
2.44.0

Re: [v5,11/13] drm/ast: Use drm_connector_helper_get_modes()

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> The .get_modes() code for VGA and SIL164 connectors does not depend
> on either type of connector. Replace the driver code with the common
> helper drm_connector_helper_get_modes(). It reads EDID data via
> DDC and updates the connector's EDID property.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Sui Jingfeng <sui.jingfeng@linux.dev>

-- 
Best regards,
Sui

[PATCH v5 12/13] drm/ast: Implement polling for VGA and SIL164 connectors

[Thomas Zimmermann]

Implement polling for VGA and SIL164 connectors. Set the flag
DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
for each type of connector by testing for EDID data.

The helper drm_connector_helper_detect_ctx() implements .detect_ctx()
on top of the connector's DDC channel. The function can be used by
other drivers as companion to drm_connector_helper_get_modes().

v5:
- share implementation in drm_connector_helper_detect_ctx() (Maxime)
- test for DDC presence with drm_probe_ddc() (Maxime, Jani)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c     |  6 ++++--
 drivers/gpu/drm/drm_probe_helper.c | 29 +++++++++++++++++++++++++++++
 include/drm/drm_probe_helper.h     |  3 +++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 71cc681d6188f..a42a0956c51de 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1346,6 +1346,7 @@ static int ast_crtc_init(struct drm_device *dev)
 
 static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
 	.get_modes = drm_connector_helper_get_modes,
+	.detect_ctx = drm_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_vga_connector_funcs = {
@@ -1379,7 +1380,7 @@ static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return 0;
 }
@@ -1414,6 +1415,7 @@ static int ast_vga_output_init(struct ast_device *ast)
 
 static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
 	.get_modes = drm_connector_helper_get_modes,
+	.detect_ctx = drm_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_sil164_connector_funcs = {
@@ -1447,7 +1449,7 @@ static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connecto
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 4d60cc810b577..b06dcc6c614e8 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1272,3 +1272,32 @@ int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
 	return i;
 }
 EXPORT_SYMBOL(drm_connector_helper_tv_get_modes);
+
+/**
+ * drm_connector_helper_detect_ctx - Read EDID and detect connector status.
+ * @connector: The connector
+ * @ctx: Acquire context
+ * @force: Perform screen-destructive operations, if necessary
+ *
+ * Detects the connector status by reading the EDID using drm_probe_ddc(),
+ * which requires connector->ddc to be set. Returns connector_status_connected
+ * on success or connector_status_disconnected on failure.
+ *
+ * Returns:
+ * The connector status as defined by enum drm_connector_status.
+ */
+int drm_connector_helper_detect_ctx(struct drm_connector *connector,
+				    struct drm_modeset_acquire_ctx *ctx,
+				    bool force)
+{
+	struct i2c_adapter *ddc = connector->ddc;
+
+	if (!ddc)
+		return connector_status_unknown;
+
+	if (drm_probe_ddc(ddc))
+		return connector_status_connected;
+
+	return connector_status_disconnected;
+}
+EXPORT_SYMBOL(drm_connector_helper_detect_ctx);
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 62741a88796bb..031b044528c89 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -37,4 +37,7 @@ int drm_connector_helper_get_modes_fixed(struct drm_connector *connector,
 int drm_connector_helper_get_modes(struct drm_connector *connector);
 int drm_connector_helper_tv_get_modes(struct drm_connector *connector);
 
+int drm_connector_helper_detect_ctx(struct drm_connector *connector,
+				    struct drm_modeset_acquire_ctx *ctx,
+				    bool force);
 #endif
-- 
2.44.0

Re: [v5,12/13] drm/ast: Implement polling for VGA and SIL164 connectors

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> Implement polling for VGA and SIL164 connectors. Set the flag
> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
> for each type of connector by testing for EDID data.
>
> The helper drm_connector_helper_detect_ctx() implements .detect_ctx()
> on top of the connector's DDC channel. The function can be used by
> other drivers as companion to drm_connector_helper_get_modes().
>
> v5:
> - share implementation in drm_connector_helper_detect_ctx() (Maxime)
> - test for DDC presence with drm_probe_ddc() (Maxime, Jani)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Yeah, previously, drm/ast only poll for the connected status.
Once connected, it never disconnected. This is fine for single
display pipe. But for two display pipes case, hot-plug is a core
functionality and absolutely necessary requirements.

Now that drm/ast becomes better than before:

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>


> ---
>   drivers/gpu/drm/ast/ast_mode.c     |  6 ++++--
>   drivers/gpu/drm/drm_probe_helper.c | 29 +++++++++++++++++++++++++++++
>   include/drm/drm_probe_helper.h     |  3 +++
>   3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 71cc681d6188f..a42a0956c51de 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1346,6 +1346,7 @@ static int ast_crtc_init(struct drm_device *dev)
>   
>   static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
>   	.get_modes = drm_connector_helper_get_modes,
> +	.detect_ctx = drm_connector_helper_detect_ctx,
>   };
>   
>   static const struct drm_connector_funcs ast_vga_connector_funcs = {
> @@ -1379,7 +1380,7 @@ static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *
>   	connector->interlace_allowed = 0;
>   	connector->doublescan_allowed = 0;
>   
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> +	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>   
>   	return 0;
>   }
> @@ -1414,6 +1415,7 @@ static int ast_vga_output_init(struct ast_device *ast)
>   
>   static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
>   	.get_modes = drm_connector_helper_get_modes,
> +	.detect_ctx = drm_connector_helper_detect_ctx,
>   };
>   
>   static const struct drm_connector_funcs ast_sil164_connector_funcs = {
> @@ -1447,7 +1449,7 @@ static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connecto
>   	connector->interlace_allowed = 0;
>   	connector->doublescan_allowed = 0;
>   
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> +	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 4d60cc810b577..b06dcc6c614e8 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1272,3 +1272,32 @@ int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
>   	return i;
>   }
>   EXPORT_SYMBOL(drm_connector_helper_tv_get_modes);
> +
> +/**
> + * drm_connector_helper_detect_ctx - Read EDID and detect connector status.
> + * @connector: The connector
> + * @ctx: Acquire context
> + * @force: Perform screen-destructive operations, if necessary
> + *
> + * Detects the connector status by reading the EDID using drm_probe_ddc(),
> + * which requires connector->ddc to be set. Returns connector_status_connected
> + * on success or connector_status_disconnected on failure.
> + *
> + * Returns:
> + * The connector status as defined by enum drm_connector_status.
> + */
> +int drm_connector_helper_detect_ctx(struct drm_connector *connector,
> +				    struct drm_modeset_acquire_ctx *ctx,
> +				    bool force)
> +{
> +	struct i2c_adapter *ddc = connector->ddc;
> +
> +	if (!ddc)
> +		return connector_status_unknown;
> +
> +	if (drm_probe_ddc(ddc))
> +		return connector_status_connected;
> +
> +	return connector_status_disconnected;
> +}
> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx);
> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
> index 62741a88796bb..031b044528c89 100644
> --- a/include/drm/drm_probe_helper.h
> +++ b/include/drm/drm_probe_helper.h
> @@ -37,4 +37,7 @@ int drm_connector_helper_get_modes_fixed(struct drm_connector *connector,
>   int drm_connector_helper_get_modes(struct drm_connector *connector);
>   int drm_connector_helper_tv_get_modes(struct drm_connector *connector);
>   
> +int drm_connector_helper_detect_ctx(struct drm_connector *connector,
> +				    struct drm_modeset_acquire_ctx *ctx,
> +				    bool force);
>   #endif

-- 
Best regards,
Sui

Re: [PATCH v5 12/13] drm/ast: Implement polling for VGA and SIL164 connectors

[Maxime Ripard]

Hi,

On Wed, Mar 20, 2024 at 10:34:17AM +0100, Thomas Zimmermann wrote:
> +/**
> + * drm_connector_helper_detect_ctx - Read EDID and detect connector status.
> + * @connector: The connector
> + * @ctx: Acquire context
> + * @force: Perform screen-destructive operations, if necessary
> + *
> + * Detects the connector status by reading the EDID using drm_probe_ddc(),
> + * which requires connector->ddc to be set. Returns connector_status_connected
> + * on success or connector_status_disconnected on failure.
> + *
> + * Returns:
> + * The connector status as defined by enum drm_connector_status.
> + */
> +int drm_connector_helper_detect_ctx(struct drm_connector *connector,
> +				    struct drm_modeset_acquire_ctx *ctx,
> +				    bool force)
> +{
> +	struct i2c_adapter *ddc = connector->ddc;
> +
> +	if (!ddc)
> +		return connector_status_unknown;
> +
> +	if (drm_probe_ddc(ddc))
> +		return connector_status_connected;
> +
> +	return connector_status_disconnected;
> +}
> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx);

I think it would be better to make it more obvious that we rely on DDC
to detect and we shouldn't consider it a generic helper that would work
in all cases.

drm_connector_helper_detect_probe_ddc maybe?

Maxime

Re: [PATCH v5 12/13] drm/ast: Implement polling for VGA and SIL164 connectors

[Thomas Zimmermann]

Hi

Am 21.03.24 um 15:09 schrieb Maxime Ripard:
> Hi,
>
> On Wed, Mar 20, 2024 at 10:34:17AM +0100, Thomas Zimmermann wrote:
>> +/**
>> + * drm_connector_helper_detect_ctx - Read EDID and detect connector status.
>> + * @connector: The connector
>> + * @ctx: Acquire context
>> + * @force: Perform screen-destructive operations, if necessary
>> + *
>> + * Detects the connector status by reading the EDID using drm_probe_ddc(),
>> + * which requires connector->ddc to be set. Returns connector_status_connected
>> + * on success or connector_status_disconnected on failure.
>> + *
>> + * Returns:
>> + * The connector status as defined by enum drm_connector_status.
>> + */
>> +int drm_connector_helper_detect_ctx(struct drm_connector *connector,
>> +				    struct drm_modeset_acquire_ctx *ctx,
>> +				    bool force)
>> +{
>> +	struct i2c_adapter *ddc = connector->ddc;
>> +
>> +	if (!ddc)
>> +		return connector_status_unknown;
>> +
>> +	if (drm_probe_ddc(ddc))
>> +		return connector_status_connected;
>> +
>> +	return connector_status_disconnected;
>> +}
>> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx);
> I think it would be better to make it more obvious that we rely on DDC
> to detect and we shouldn't consider it a generic helper that would work
> in all cases.
>
> drm_connector_helper_detect_probe_ddc maybe?

No objection from me about mentioning DDC. What what about 
drm_connector_helper_get_modes()? It relies on DDC as well, so I thought 
that that's the default. Should we consider renaming it?

Best regards
Thomas

>
> Maxime

-- 
--
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)

Re: [PATCH v5 12/13] drm/ast: Implement polling for VGA and SIL164 connectors

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2354 bytes --]

On Fri, Mar 22, 2024 at 07:40:52AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 21.03.24 um 15:09 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Wed, Mar 20, 2024 at 10:34:17AM +0100, Thomas Zimmermann wrote:
> > > +/**
> > > + * drm_connector_helper_detect_ctx - Read EDID and detect connector status.
> > > + * @connector: The connector
> > > + * @ctx: Acquire context
> > > + * @force: Perform screen-destructive operations, if necessary
> > > + *
> > > + * Detects the connector status by reading the EDID using drm_probe_ddc(),
> > > + * which requires connector->ddc to be set. Returns connector_status_connected
> > > + * on success or connector_status_disconnected on failure.
> > > + *
> > > + * Returns:
> > > + * The connector status as defined by enum drm_connector_status.
> > > + */
> > > +int drm_connector_helper_detect_ctx(struct drm_connector *connector,
> > > +				    struct drm_modeset_acquire_ctx *ctx,
> > > +				    bool force)
> > > +{
> > > +	struct i2c_adapter *ddc = connector->ddc;
> > > +
> > > +	if (!ddc)
> > > +		return connector_status_unknown;
> > > +
> > > +	if (drm_probe_ddc(ddc))
> > > +		return connector_status_connected;
> > > +
> > > +	return connector_status_disconnected;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_helper_detect_ctx);
> > I think it would be better to make it more obvious that we rely on DDC
> > to detect and we shouldn't consider it a generic helper that would work
> > in all cases.
> > 
> > drm_connector_helper_detect_probe_ddc maybe?
> 
> No objection from me about mentioning DDC. What what about
> drm_connector_helper_get_modes()? It relies on DDC as well, so I thought
> that that's the default. Should we consider renaming it?

I see your point, but I think it's not totally in the same situation.
Unless you have a fixed mode panel (and even then, some support EDID),
EDID is the de-facto standard to retrieve the modes from the attached
monitor.

You don't really have that standardization with monitor detection:
probing the DDC bus is one of the many valid methods to do so, along
with reading the HPD register, reading a GPIO state, etc.

So I would say it's much more critical for the detect helper than it is
for the get_modes one. But we could totally do it for get_modes too to
make it somewhat consistent.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 12/13] drm/ast: Implement polling for VGA and SIL164 connectors

[Sui Jingfeng]

Hi,


On 2024/3/21 22:09, Maxime Ripard wrote:
> Hi,
>
> On Wed, Mar 20, 2024 at 10:34:17AM +0100, Thomas Zimmermann wrote:
>> +/**
>> + * drm_connector_helper_detect_ctx - Read EDID and detect connector status.
>> + * @connector: The connector
>> + * @ctx: Acquire context
>> + * @force: Perform screen-destructive operations, if necessary
>> + *
>> + * Detects the connector status by reading the EDID using drm_probe_ddc(),
>> + * which requires connector->ddc to be set. Returns connector_status_connected
>> + * on success or connector_status_disconnected on failure.
>> + *
>> + * Returns:
>> + * The connector status as defined by enum drm_connector_status.
>> + */
>> +int drm_connector_helper_detect_ctx(struct drm_connector *connector,
>> +				    struct drm_modeset_acquire_ctx *ctx,
>> +				    bool force)
>> +{
>> +	struct i2c_adapter *ddc = connector->ddc;
>> +
>> +	if (!ddc)
>> +		return connector_status_unknown;
>> +
>> +	if (drm_probe_ddc(ddc))
>> +		return connector_status_connected;
>> +
>> +	return connector_status_disconnected;
>> +}
>> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx);
> I think it would be better to make it more obvious that we rely on DDC
> to detect and we shouldn't consider it a generic helper that would work
> in all cases.
>
> drm_connector_helper_detect_probe_ddc maybe?

Yeah, the function name(drm_connector_helper_detect_ctx) is a little bit of casual.
So a better name is preferred.


> Maxime

-- 
Best regards,
Sui

[PATCH v5 13/13] drm/ast: Automatically clean up poll helper

[Thomas Zimmermann]

Automatically clean up the conncetor-poll thread as part of the DRM
device release. The new helper drmm_kms_helper_poll_init() provides
a shared implementation for all drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c     |  4 +++-
 drivers/gpu/drm/drm_probe_helper.c | 27 +++++++++++++++++++++++++++
 include/drm/drm_probe_helper.h     |  2 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index a42a0956c51de..7e56a77bed635 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1905,7 +1905,9 @@ int ast_mode_config_init(struct ast_device *ast)
 
 	drm_mode_config_reset(dev);
 
-	drm_kms_helper_poll_init(dev);
+	ret = drmm_kms_helper_poll_init(dev);
+	if (ret)
+		return ret;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index b06dcc6c614e8..a39c98ceac68a 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -37,6 +37,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
@@ -944,6 +945,32 @@ void drm_kms_helper_poll_fini(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_fini);
 
+static void drm_kms_helper_poll_init_release(struct drm_device *dev, void *res)
+{
+	drm_kms_helper_poll_fini(dev);
+}
+
+/**
+ * devm_drm_kms_helper_poll_init - initialize and enable output polling
+ * @dev: drm_device
+ *
+ * This function initializes and then also enables output polling support for
+ * @dev similar to drm_kms_helper_poll_init(). Polling will automatically be
+ * cleaned up when the DRM device goes away.
+ *
+ * See drm_kms_helper_poll_init() for more information.
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ */
+int drmm_kms_helper_poll_init(struct drm_device *dev)
+{
+	drm_kms_helper_poll_init(dev);
+
+	return drmm_add_action_or_reset(dev, drm_kms_helper_poll_init_release, dev);
+}
+EXPORT_SYMBOL(drmm_kms_helper_poll_init);
+
 static bool check_connector_changed(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 031b044528c89..9925cff749296 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -16,6 +16,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector
 int drm_helper_probe_detect(struct drm_connector *connector,
 			    struct drm_modeset_acquire_ctx *ctx,
 			    bool force);
+
+int drmm_kms_helper_poll_init(struct drm_device *dev);
 void drm_kms_helper_poll_init(struct drm_device *dev);
 void drm_kms_helper_poll_fini(struct drm_device *dev);
 bool drm_helper_hpd_irq_event(struct drm_device *dev);
-- 
2.44.0

Re: [PATCH v5 13/13] drm/ast: Automatically clean up poll helper

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.8 next-20240320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-ast-Include-linux-of-h-where-necessary/20240320-174013
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240320093738.6341-14-tzimmermann%40suse.de
patch subject: [PATCH v5 13/13] drm/ast: Automatically clean up poll helper
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240321/202403211604.aM8tDovD-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/202403211604.aM8tDovD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403211604.aM8tDovD-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_probe_helper.c:965: warning: expecting prototype for devm_drm_kms_helper_poll_init(). Prototype was for drmm_kms_helper_poll_init() instead


vim +965 drivers/gpu/drm/drm_probe_helper.c

   950	
   951	/**
   952	 * devm_drm_kms_helper_poll_init - initialize and enable output polling
   953	 * @dev: drm_device
   954	 *
   955	 * This function initializes and then also enables output polling support for
   956	 * @dev similar to drm_kms_helper_poll_init(). Polling will automatically be
   957	 * cleaned up when the DRM device goes away.
   958	 *
   959	 * See drm_kms_helper_poll_init() for more information.
   960	 *
   961	 * Returns:
   962	 * 0 on success, or a negative errno code otherwise.
   963	 */
   964	int drmm_kms_helper_poll_init(struct drm_device *dev)
 > 965	{
   966		drm_kms_helper_poll_init(dev);
   967	
   968		return drmm_add_action_or_reset(dev, drm_kms_helper_poll_init_release, dev);
   969	}
   970	EXPORT_SYMBOL(drmm_kms_helper_poll_init);
   971	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [v5,13/13] drm/ast: Automatically clean up poll helper

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:
> Automatically clean up the conncetor-poll thread as part of the DRM
> device release. The new helper drmm_kms_helper_poll_init() provides
> a shared implementation for all drivers.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>


Nice feature!

It seems that drm/loongson forget to calldrm_kms_helper_poll_fini() on driver leave, Opps.


Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>



> ---
>   drivers/gpu/drm/ast/ast_mode.c     |  4 +++-
>   drivers/gpu/drm/drm_probe_helper.c | 27 +++++++++++++++++++++++++++
>   include/drm/drm_probe_helper.h     |  2 ++
>   3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index a42a0956c51de..7e56a77bed635 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1905,7 +1905,9 @@ int ast_mode_config_init(struct ast_device *ast)
>   
>   	drm_mode_config_reset(dev);
>   
> -	drm_kms_helper_poll_init(dev);
> +	ret = drmm_kms_helper_poll_init(dev);
> +	if (ret)
> +		return ret;
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index b06dcc6c614e8..a39c98ceac68a 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -37,6 +37,7 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_edid.h>
>   #include <drm/drm_fourcc.h>
> +#include <drm/drm_managed.h>
>   #include <drm/drm_modeset_helper_vtables.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> @@ -944,6 +945,32 @@ void drm_kms_helper_poll_fini(struct drm_device *dev)
>   }
>   EXPORT_SYMBOL(drm_kms_helper_poll_fini);
>   
> +static void drm_kms_helper_poll_init_release(struct drm_device *dev, void *res)
> +{
> +	drm_kms_helper_poll_fini(dev);
> +}
> +
> +/**
> + * devm_drm_kms_helper_poll_init - initialize and enable output polling
> + * @dev: drm_device
> + *
> + * This function initializes and then also enables output polling support for
> + * @dev similar to drm_kms_helper_poll_init(). Polling will automatically be
> + * cleaned up when the DRM device goes away.
> + *
> + * See drm_kms_helper_poll_init() for more information.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int drmm_kms_helper_poll_init(struct drm_device *dev)
> +{
> +	drm_kms_helper_poll_init(dev);
> +
> +	return drmm_add_action_or_reset(dev, drm_kms_helper_poll_init_release, dev);
> +}
> +EXPORT_SYMBOL(drmm_kms_helper_poll_init);
> +
>   static bool check_connector_changed(struct drm_connector *connector)
>   {
>   	struct drm_device *dev = connector->dev;
> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
> index 031b044528c89..9925cff749296 100644
> --- a/include/drm/drm_probe_helper.h
> +++ b/include/drm/drm_probe_helper.h
> @@ -16,6 +16,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector
>   int drm_helper_probe_detect(struct drm_connector *connector,
>   			    struct drm_modeset_acquire_ctx *ctx,
>   			    bool force);
> +
> +int drmm_kms_helper_poll_init(struct drm_device *dev);
>   void drm_kms_helper_poll_init(struct drm_device *dev);
>   void drm_kms_helper_poll_fini(struct drm_device *dev);
>   bool drm_helper_hpd_irq_event(struct drm_device *dev);

-- 
Best regards,
Sui

Re: [v5,13/13] drm/ast: Automatically clean up poll helper

[Thomas Zimmermann]

Hi

Am 21.03.24 um 10:00 schrieb Sui Jingfeng:
> Hi,
>
>
> On 2024/3/20 17:34, Thomas Zimmermann wrote:
>> Automatically clean up the conncetor-poll thread as part of the DRM
>> device release. The new helper drmm_kms_helper_poll_init() provides
>> a shared implementation for all drivers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>
>
> Nice feature!
>
> It seems that drm/loongson forget to calldrm_kms_helper_poll_fini() on 
> driver leave, Opps.

Indeed. I'm surprised that this never blew up with ast. Without _fini(), 
the polling thread would likely run on a stale DRM device. We only 
release the device during shutdown and I guess it doesn't make 
difference then.

Best regards
Thomas

>
>
> Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>
>
>
>> ---
>>   drivers/gpu/drm/ast/ast_mode.c     |  4 +++-
>>   drivers/gpu/drm/drm_probe_helper.c | 27 +++++++++++++++++++++++++++
>>   include/drm/drm_probe_helper.h     |  2 ++
>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index a42a0956c51de..7e56a77bed635 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1905,7 +1905,9 @@ int ast_mode_config_init(struct ast_device *ast)
>>         drm_mode_config_reset(dev);
>>   -    drm_kms_helper_poll_init(dev);
>> +    ret = drmm_kms_helper_poll_init(dev);
>> +    if (ret)
>> +        return ret;
>>         return 0;
>>   }
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c 
>> b/drivers/gpu/drm/drm_probe_helper.c
>> index b06dcc6c614e8..a39c98ceac68a 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_fourcc.h>
>> +#include <drm/drm_managed.h>
>>   #include <drm/drm_modeset_helper_vtables.h>
>>   #include <drm/drm_print.h>
>>   #include <drm/drm_probe_helper.h>
>> @@ -944,6 +945,32 @@ void drm_kms_helper_poll_fini(struct drm_device 
>> *dev)
>>   }
>>   EXPORT_SYMBOL(drm_kms_helper_poll_fini);
>>   +static void drm_kms_helper_poll_init_release(struct drm_device 
>> *dev, void *res)
>> +{
>> +    drm_kms_helper_poll_fini(dev);
>> +}
>> +
>> +/**
>> + * devm_drm_kms_helper_poll_init - initialize and enable output polling
>> + * @dev: drm_device
>> + *
>> + * This function initializes and then also enables output polling 
>> support for
>> + * @dev similar to drm_kms_helper_poll_init(). Polling will 
>> automatically be
>> + * cleaned up when the DRM device goes away.
>> + *
>> + * See drm_kms_helper_poll_init() for more information.
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise.
>> + */
>> +int drmm_kms_helper_poll_init(struct drm_device *dev)
>> +{
>> +    drm_kms_helper_poll_init(dev);
>> +
>> +    return drmm_add_action_or_reset(dev, 
>> drm_kms_helper_poll_init_release, dev);
>> +}
>> +EXPORT_SYMBOL(drmm_kms_helper_poll_init);
>> +
>>   static bool check_connector_changed(struct drm_connector *connector)
>>   {
>>       struct drm_device *dev = connector->dev;
>> diff --git a/include/drm/drm_probe_helper.h 
>> b/include/drm/drm_probe_helper.h
>> index 031b044528c89..9925cff749296 100644
>> --- a/include/drm/drm_probe_helper.h
>> +++ b/include/drm/drm_probe_helper.h
>> @@ -16,6 +16,8 @@ int drm_helper_probe_single_connector_modes(struct 
>> drm_connector
>>   int drm_helper_probe_detect(struct drm_connector *connector,
>>                   struct drm_modeset_acquire_ctx *ctx,
>>                   bool force);
>> +
>> +int drmm_kms_helper_poll_init(struct drm_device *dev);
>>   void drm_kms_helper_poll_init(struct drm_device *dev);
>>   void drm_kms_helper_poll_fini(struct drm_device *dev);
>>   bool drm_helper_hpd_irq_event(struct drm_device *dev);
>

-- 
--
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)

Re: [v5,13/13] drm/ast: Automatically clean up poll helper

[Sui Jingfeng]

Hi,


On 2024/3/20 17:34, Thomas Zimmermann wrote:



> +
> +/**
> + * devm_drm_kms_helper_poll_init - initialize and enable output polling

Should be drmm_kms_helper_poll_init() here.

> + * @dev: drm_device
> + *
> + * This function initializes and then also enables output polling support for
> + * @dev similar to drm_kms_helper_poll_init(). Polling will automatically be
> + * cleaned up when the DRM device goes away.
> + *
> + * See drm_kms_helper_poll_init() for more information.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int drmm_kms_helper_poll_init(struct drm_device *dev)
> +{
> +	drm_kms_helper_poll_init(dev);
> +
> +	return drmm_add_action_or_reset(dev, drm_kms_helper_poll_init_release, dev);
> +}
> +EXPORT_SYMBOL(drmm_kms_helper_poll_init);

-- 
Best regards,
Sui

Re: [PATCH v5 00/13] drm/ast: Detect connector status for VGA and SIL164

[Jocelyn Falempe]

Hi,

Thanks for those patches, that's really a good enhancement to the ast 
driver, and fits well with the BMC virtual connector.


I've reviewed the whole series, and it looks good to me.

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

-- 

Jocelyn

On 20/03/2024 10:34, Thomas Zimmermann wrote:
> Detect the status of the VGA or SIL164 conenctor by polling the DDC
> channel. Update the status at runtime and flip the BMC status as well.
> 
> Patches 1 adds a missing include statement that will be required to
> make later patches compile.
> 
> Patches 2 and 3 simplify the VGA and SIL164 connectors, such that no
> additional data structure is required. These patches have been reviewed
> before as part of the series at [1].
> 
> Patches 4 to 10 improve the I2C code that is used to retrieve the
> monitor's EDID data. It's now fully managed, it acquires the necessary
> lock automatically and it is called DDC, which better represents its
> purpose than I2C.
> 
> Patches 11 to 13 finally implement polling. Patch 11 updates ast's
> EDID code to be up-to-date. The helper drm_connector_get_modes() reads
> the EDID via DDC and updates the property. No driver code is required.
> Patch 12 uses a similar pattern to detect the presence of the monitor
> and sets the connector status accordingly. As polling also needs to be
> cleaned up, patch 13 adds the necessary helpers to do so.
> 
> Tested on AST2500 hardware and BMC output. The BMC connector now also
> flips its status correctly at runtime.
> 
> [1] https://patchwork.freedesktop.org/series/104547/
> 
> v5:
> - share implementation in drm_connector_helper_detect_ctx() (Maxime)
> - test for DDC presence with drm_probe_ddc() (Maxime, Jani)
> - perform managed cleanup of poll thread
> 
> Thomas Zimmermann (13):
>    drm/ast: Include <linux/of.h> where necessary
>    drm/ast: Fail probing if DDC channel could not be initialized
>    drm/ast: Remove struct ast_{vga,sil165}_connector
>    drm/ast: Allocate instance of struct ast_i2c_chan with managed helpers
>    drm/ast: Move DDC code to ast_ddc.{c,h}
>    drm/ast: Rename struct ast_i2c_chan to struct ast_ddc
>    drm/ast: Pass AST device to ast_ddc_create()
>    drm/ast: Store AST device in struct ast_ddc
>    drm/ast: Rename struct i2c_algo_bit_data callbacks and their
>      parameters
>    drm/ast: Acquire I/O-register lock in DDC code
>    drm/ast: Use drm_connector_helper_get_modes()
>    drm/ast: Implement polling for VGA and SIL164 connectors
>    drm/ast: Automatically clean up poll helper
> 
>   drivers/gpu/drm/ast/Makefile                 |  10 +-
>   drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} | 120 +++++++++------
>   drivers/gpu/drm/ast/ast_ddc.h                |  20 +++
>   drivers/gpu/drm/ast/ast_drv.c                |   1 +
>   drivers/gpu/drm/ast/ast_drv.h                |  39 +----
>   drivers/gpu/drm/ast/ast_main.c               |   1 +
>   drivers/gpu/drm/ast/ast_mode.c               | 147 +++++--------------
>   drivers/gpu/drm/drm_probe_helper.c           |  56 +++++++
>   include/drm/drm_probe_helper.h               |   5 +
>   9 files changed, 206 insertions(+), 193 deletions(-)
>   rename drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} (54%)
>   create mode 100644 drivers/gpu/drm/ast/ast_ddc.h
>