* [PATCH v2 1/5] drm/ast: Support multiple outputs
2022-05-31 11:14 [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
@ 2022-05-31 11:14 ` Thomas Zimmermann
2022-06-02 7:24 ` Patrik Jakobsson
2022-05-31 11:14 ` [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property Thomas Zimmermann
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2022-05-31 11:14 UTC (permalink / raw)
To: airlied, kuohsiang_chou, airlied, daniel, maarten.lankhorst, mripard
Cc: Thomas Zimmermann, dri-devel, Javier Martinez Canillas
Systems with AST graphics can have multiple output; typically VGA
plus some other port. Record detected output chips in a bitmask and
initialize each output on its own.
Assume a VGA output by default and use SIL164 and DP501 if available.
For ASTDP assume that it can run in parallel with VGA.
Tested on AST2100.
v2:
* make VGA/SIL164/DP501 mutually exclusive
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: a59b026419f3 ("drm/ast: Initialize encoder and connector for VGA in helper function")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/ast/ast_dp.c | 5 ++---
drivers/gpu/drm/ast/ast_dp501.c | 2 +-
drivers/gpu/drm/ast/ast_drv.h | 4 ++--
drivers/gpu/drm/ast/ast_main.c | 21 ++++++++----------
drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++++---------------
drivers/gpu/drm/ast/ast_post.c | 2 +-
6 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index 4551bc8a3ecf..f573d582407e 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -160,13 +160,12 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower)
}
if (bDPExecute)
- ast->tx_chip_type = AST_TX_ASTDP;
+ ast->tx_chip_types |= BIT(AST_TX_ASTDP);
ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE5,
(u8) ~ASTDP_HOST_EDID_READ_DONE_MASK,
ASTDP_HOST_EDID_READ_DONE);
- } else
- ast->tx_chip_type = AST_TX_NONE;
+ }
}
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 204c926a18ea..4f75a9efb610 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -450,7 +450,7 @@ void ast_init_3rdtx(struct drm_device *dev)
ast_init_dvo(dev);
break;
default:
- if (ast->tx_chip_type == AST_TX_SIL164)
+ if (ast->tx_chip_types & BIT(AST_TX_SIL164))
ast_init_dvo(dev);
else
ast_init_analog(dev);
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index afebe35f205e..3055b0be7b67 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -173,7 +173,7 @@ struct ast_private {
struct drm_plane primary_plane;
struct ast_cursor_plane cursor_plane;
struct drm_crtc crtc;
- union {
+ struct {
struct {
struct drm_encoder encoder;
struct ast_vga_connector vga_connector;
@@ -199,7 +199,7 @@ struct ast_private {
ast_use_defaults
} config_mode;
- enum ast_tx_chip tx_chip_type;
+ unsigned long tx_chip_types; /* bitfield of enum ast_chip_type */
u8 *dp501_fw_addr;
const struct firmware *dp501_fw; /* dp501 fw */
};
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index d770d5a23c1a..50b8d51382c7 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -216,7 +216,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
}
/* Check 3rd Tx option (digital output afaik) */
- ast->tx_chip_type = AST_TX_NONE;
+ ast->tx_chip_types |= BIT(AST_TX_NONE);
/*
* VGACRA3 Enhanced Color Mode Register, check if DVO is already
@@ -229,7 +229,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
if (!*need_post) {
jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xff);
if (jreg & 0x80)
- ast->tx_chip_type = AST_TX_SIL164;
+ ast->tx_chip_types = BIT(AST_TX_SIL164);
}
if ((ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST2500)) {
@@ -241,7 +241,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
switch (jreg) {
case 0x04:
- ast->tx_chip_type = AST_TX_SIL164;
+ ast->tx_chip_types = BIT(AST_TX_SIL164);
break;
case 0x08:
ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
@@ -254,22 +254,19 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
}
fallthrough;
case 0x0c:
- ast->tx_chip_type = AST_TX_DP501;
+ ast->tx_chip_types = BIT(AST_TX_DP501);
}
} else if (ast->chip == AST2600)
ast_dp_launch(&ast->base, 0);
/* Print stuff for diagnostic purposes */
- switch(ast->tx_chip_type) {
- case AST_TX_SIL164:
+ if (ast->tx_chip_types & BIT(AST_TX_NONE))
+ drm_info(dev, "Using analog VGA\n");
+ if (ast->tx_chip_types & BIT(AST_TX_SIL164))
drm_info(dev, "Using Sil164 TMDS transmitter\n");
- break;
- case AST_TX_DP501:
+ if (ast->tx_chip_types & BIT(AST_TX_DP501))
drm_info(dev, "Using DP501 DisplayPort transmitter\n");
- break;
- default:
- drm_info(dev, "Analog VGA only\n");
- }
+
return 0;
}
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 323af2746aa9..e19dd2f9b3ce 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -997,10 +997,10 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
case DRM_MODE_DPMS_ON:
ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x01, 0xdf, 0);
ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xfc, 0);
- if (ast->tx_chip_type == AST_TX_DP501)
+ if (ast->tx_chip_types & BIT(AST_TX_DP501))
ast_set_dp501_video_output(crtc->dev, 1);
- if (ast->tx_chip_type == AST_TX_ASTDP) {
+ if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
ast_dp_power_on_off(crtc->dev, AST_DP_POWER_ON);
ast_wait_for_vretrace(ast);
ast_dp_set_on_off(crtc->dev, 1);
@@ -1012,17 +1012,17 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
case DRM_MODE_DPMS_SUSPEND:
case DRM_MODE_DPMS_OFF:
ch = mode;
- if (ast->tx_chip_type == AST_TX_DP501)
+ if (ast->tx_chip_types & BIT(AST_TX_DP501))
ast_set_dp501_video_output(crtc->dev, 0);
- break;
- if (ast->tx_chip_type == AST_TX_ASTDP) {
+ if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
ast_dp_set_on_off(crtc->dev, 0);
ast_dp_power_on_off(crtc->dev, AST_DP_POWER_OFF);
}
ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x01, 0xdf, 0x20);
ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xfc, ch);
+ break;
}
}
@@ -1155,7 +1155,7 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
ast_crtc_load_lut(ast, crtc);
//Set Aspeed Display-Port
- if (ast->tx_chip_type == AST_TX_ASTDP)
+ if (ast->tx_chip_types & BIT(AST_TX_ASTDP))
ast_dp_set_mode(crtc, vbios_mode_info);
mutex_unlock(&ast->ioregs_lock);
@@ -1739,22 +1739,26 @@ int ast_mode_config_init(struct ast_private *ast)
ast_crtc_init(dev);
- switch (ast->tx_chip_type) {
- case AST_TX_NONE:
+ if (ast->tx_chip_types & BIT(AST_TX_NONE)) {
ret = ast_vga_output_init(ast);
- break;
- case AST_TX_SIL164:
+ if (ret)
+ return ret;
+ }
+ if (ast->tx_chip_types & BIT(AST_TX_SIL164)) {
ret = ast_sil164_output_init(ast);
- break;
- case AST_TX_DP501:
+ if (ret)
+ return ret;
+ }
+ if (ast->tx_chip_types & BIT(AST_TX_DP501)) {
ret = ast_dp501_output_init(ast);
- break;
- case AST_TX_ASTDP:
+ if (ret)
+ return ret;
+ }
+ if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
ret = ast_astdp_output_init(ast);
- break;
+ if (ret)
+ return ret;
}
- if (ret)
- return ret;
drm_mode_config_reset(dev);
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 0aa9cf0fb5c3..2e1c343b70a3 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -391,7 +391,7 @@ void ast_post_gpu(struct drm_device *dev)
ast_init_3rdtx(dev);
} else {
- if (ast->tx_chip_type != AST_TX_NONE)
+ if (ast->tx_chip_types & BIT(AST_TX_SIL164))
ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xcf, 0x80); /* Enable DVO */
}
}
--
2.36.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/5] drm/ast: Support multiple outputs
2022-05-31 11:14 ` [PATCH v2 1/5] drm/ast: Support multiple outputs Thomas Zimmermann
@ 2022-06-02 7:24 ` Patrik Jakobsson
2022-06-02 11:01 ` Thomas Zimmermann
0 siblings, 1 reply; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 7:24 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: David Airlie, Javier Martinez Canillas, dri-devel, Dave Airlie
On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Systems with AST graphics can have multiple output; typically VGA
> plus some other port. Record detected output chips in a bitmask and
> initialize each output on its own.
>
> Assume a VGA output by default and use SIL164 and DP501 if available.
> For ASTDP assume that it can run in parallel with VGA.
>
> Tested on AST2100.
>
> v2:
> * make VGA/SIL164/DP501 mutually exclusive
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: a59b026419f3 ("drm/ast: Initialize encoder and connector for VGA in helper function")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/ast/ast_dp.c | 5 ++---
> drivers/gpu/drm/ast/ast_dp501.c | 2 +-
> drivers/gpu/drm/ast/ast_drv.h | 4 ++--
> drivers/gpu/drm/ast/ast_main.c | 21 ++++++++----------
> drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++++---------------
> drivers/gpu/drm/ast/ast_post.c | 2 +-
> 6 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 4551bc8a3ecf..f573d582407e 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -160,13 +160,12 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower)
> }
>
> if (bDPExecute)
> - ast->tx_chip_type = AST_TX_ASTDP;
> + ast->tx_chip_types |= BIT(AST_TX_ASTDP);
Hi Thomas,
Just a matter of taste but an alternative to using the BIT macro
everywhere is to just define all the AST_TX_* as bits directly and get
rid of the enum ast_tx_chip.
Either way is fine with me.
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>
> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE5,
> (u8) ~ASTDP_HOST_EDID_READ_DONE_MASK,
> ASTDP_HOST_EDID_READ_DONE);
> - } else
> - ast->tx_chip_type = AST_TX_NONE;
> + }
> }
>
>
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index 204c926a18ea..4f75a9efb610 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -450,7 +450,7 @@ void ast_init_3rdtx(struct drm_device *dev)
> ast_init_dvo(dev);
> break;
> default:
> - if (ast->tx_chip_type == AST_TX_SIL164)
> + if (ast->tx_chip_types & BIT(AST_TX_SIL164))
> ast_init_dvo(dev);
> else
> ast_init_analog(dev);
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index afebe35f205e..3055b0be7b67 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -173,7 +173,7 @@ struct ast_private {
> struct drm_plane primary_plane;
> struct ast_cursor_plane cursor_plane;
> struct drm_crtc crtc;
> - union {
> + struct {
> struct {
> struct drm_encoder encoder;
> struct ast_vga_connector vga_connector;
> @@ -199,7 +199,7 @@ struct ast_private {
> ast_use_defaults
> } config_mode;
>
> - enum ast_tx_chip tx_chip_type;
> + unsigned long tx_chip_types; /* bitfield of enum ast_chip_type */
> u8 *dp501_fw_addr;
> const struct firmware *dp501_fw; /* dp501 fw */
> };
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index d770d5a23c1a..50b8d51382c7 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -216,7 +216,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
> }
>
> /* Check 3rd Tx option (digital output afaik) */
> - ast->tx_chip_type = AST_TX_NONE;
> + ast->tx_chip_types |= BIT(AST_TX_NONE);
>
> /*
> * VGACRA3 Enhanced Color Mode Register, check if DVO is already
> @@ -229,7 +229,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
> if (!*need_post) {
> jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xff);
> if (jreg & 0x80)
> - ast->tx_chip_type = AST_TX_SIL164;
> + ast->tx_chip_types = BIT(AST_TX_SIL164);
> }
>
> if ((ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST2500)) {
> @@ -241,7 +241,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
> jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
> switch (jreg) {
> case 0x04:
> - ast->tx_chip_type = AST_TX_SIL164;
> + ast->tx_chip_types = BIT(AST_TX_SIL164);
> break;
> case 0x08:
> ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
> @@ -254,22 +254,19 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
> }
> fallthrough;
> case 0x0c:
> - ast->tx_chip_type = AST_TX_DP501;
> + ast->tx_chip_types = BIT(AST_TX_DP501);
> }
> } else if (ast->chip == AST2600)
> ast_dp_launch(&ast->base, 0);
>
> /* Print stuff for diagnostic purposes */
> - switch(ast->tx_chip_type) {
> - case AST_TX_SIL164:
> + if (ast->tx_chip_types & BIT(AST_TX_NONE))
> + drm_info(dev, "Using analog VGA\n");
> + if (ast->tx_chip_types & BIT(AST_TX_SIL164))
> drm_info(dev, "Using Sil164 TMDS transmitter\n");
> - break;
> - case AST_TX_DP501:
> + if (ast->tx_chip_types & BIT(AST_TX_DP501))
> drm_info(dev, "Using DP501 DisplayPort transmitter\n");
> - break;
> - default:
> - drm_info(dev, "Analog VGA only\n");
> - }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 323af2746aa9..e19dd2f9b3ce 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -997,10 +997,10 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
> case DRM_MODE_DPMS_ON:
> ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x01, 0xdf, 0);
> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xfc, 0);
> - if (ast->tx_chip_type == AST_TX_DP501)
> + if (ast->tx_chip_types & BIT(AST_TX_DP501))
> ast_set_dp501_video_output(crtc->dev, 1);
>
> - if (ast->tx_chip_type == AST_TX_ASTDP) {
> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
> ast_dp_power_on_off(crtc->dev, AST_DP_POWER_ON);
> ast_wait_for_vretrace(ast);
> ast_dp_set_on_off(crtc->dev, 1);
> @@ -1012,17 +1012,17 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
> case DRM_MODE_DPMS_SUSPEND:
> case DRM_MODE_DPMS_OFF:
> ch = mode;
> - if (ast->tx_chip_type == AST_TX_DP501)
> + if (ast->tx_chip_types & BIT(AST_TX_DP501))
> ast_set_dp501_video_output(crtc->dev, 0);
> - break;
>
> - if (ast->tx_chip_type == AST_TX_ASTDP) {
> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
> ast_dp_set_on_off(crtc->dev, 0);
> ast_dp_power_on_off(crtc->dev, AST_DP_POWER_OFF);
> }
>
> ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x01, 0xdf, 0x20);
> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xfc, ch);
> + break;
> }
> }
>
> @@ -1155,7 +1155,7 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
> ast_crtc_load_lut(ast, crtc);
>
> //Set Aspeed Display-Port
> - if (ast->tx_chip_type == AST_TX_ASTDP)
> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP))
> ast_dp_set_mode(crtc, vbios_mode_info);
>
> mutex_unlock(&ast->ioregs_lock);
> @@ -1739,22 +1739,26 @@ int ast_mode_config_init(struct ast_private *ast)
>
> ast_crtc_init(dev);
>
> - switch (ast->tx_chip_type) {
> - case AST_TX_NONE:
> + if (ast->tx_chip_types & BIT(AST_TX_NONE)) {
> ret = ast_vga_output_init(ast);
> - break;
> - case AST_TX_SIL164:
> + if (ret)
> + return ret;
> + }
> + if (ast->tx_chip_types & BIT(AST_TX_SIL164)) {
> ret = ast_sil164_output_init(ast);
> - break;
> - case AST_TX_DP501:
> + if (ret)
> + return ret;
> + }
> + if (ast->tx_chip_types & BIT(AST_TX_DP501)) {
> ret = ast_dp501_output_init(ast);
> - break;
> - case AST_TX_ASTDP:
> + if (ret)
> + return ret;
> + }
> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
> ret = ast_astdp_output_init(ast);
> - break;
> + if (ret)
> + return ret;
> }
> - if (ret)
> - return ret;
>
> drm_mode_config_reset(dev);
>
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index 0aa9cf0fb5c3..2e1c343b70a3 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -391,7 +391,7 @@ void ast_post_gpu(struct drm_device *dev)
>
> ast_init_3rdtx(dev);
> } else {
> - if (ast->tx_chip_type != AST_TX_NONE)
> + if (ast->tx_chip_types & BIT(AST_TX_SIL164))
> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xcf, 0x80); /* Enable DVO */
> }
> }
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/5] drm/ast: Support multiple outputs
2022-06-02 7:24 ` Patrik Jakobsson
@ 2022-06-02 11:01 ` Thomas Zimmermann
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Zimmermann @ 2022-06-02 11:01 UTC (permalink / raw)
To: Patrik Jakobsson
Cc: David Airlie, Javier Martinez Canillas, dri-devel, Dave Airlie
[-- Attachment #1.1: Type: text/plain, Size: 11357 bytes --]
Hi
Am 02.06.22 um 09:24 schrieb Patrik Jakobsson:
> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Systems with AST graphics can have multiple output; typically VGA
>> plus some other port. Record detected output chips in a bitmask and
>> initialize each output on its own.
>>
>> Assume a VGA output by default and use SIL164 and DP501 if available.
>> For ASTDP assume that it can run in parallel with VGA.
>>
>> Tested on AST2100.
>>
>> v2:
>> * make VGA/SIL164/DP501 mutually exclusive
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: a59b026419f3 ("drm/ast: Initialize encoder and connector for VGA in helper function")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>> drivers/gpu/drm/ast/ast_dp.c | 5 ++---
>> drivers/gpu/drm/ast/ast_dp501.c | 2 +-
>> drivers/gpu/drm/ast/ast_drv.h | 4 ++--
>> drivers/gpu/drm/ast/ast_main.c | 21 ++++++++----------
>> drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++++---------------
>> drivers/gpu/drm/ast/ast_post.c | 2 +-
>> 6 files changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>> index 4551bc8a3ecf..f573d582407e 100644
>> --- a/drivers/gpu/drm/ast/ast_dp.c
>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>> @@ -160,13 +160,12 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower)
>> }
>>
>> if (bDPExecute)
>> - ast->tx_chip_type = AST_TX_ASTDP;
>> + ast->tx_chip_types |= BIT(AST_TX_ASTDP);
>
> Hi Thomas,
> Just a matter of taste but an alternative to using the BIT macro
> everywhere is to just define all the AST_TX_* as bits directly and get
> rid of the enum ast_tx_chip.
That makes sense.
>
> Either way is fine with me.
>
> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Thanks for reviewing my patchset.
Best regards
Thomas
>
>
>>
>> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE5,
>> (u8) ~ASTDP_HOST_EDID_READ_DONE_MASK,
>> ASTDP_HOST_EDID_READ_DONE);
>> - } else
>> - ast->tx_chip_type = AST_TX_NONE;
>> + }
>> }
>>
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
>> index 204c926a18ea..4f75a9efb610 100644
>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>> @@ -450,7 +450,7 @@ void ast_init_3rdtx(struct drm_device *dev)
>> ast_init_dvo(dev);
>> break;
>> default:
>> - if (ast->tx_chip_type == AST_TX_SIL164)
>> + if (ast->tx_chip_types & BIT(AST_TX_SIL164))
>> ast_init_dvo(dev);
>> else
>> ast_init_analog(dev);
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
>> index afebe35f205e..3055b0be7b67 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -173,7 +173,7 @@ struct ast_private {
>> struct drm_plane primary_plane;
>> struct ast_cursor_plane cursor_plane;
>> struct drm_crtc crtc;
>> - union {
>> + struct {
>> struct {
>> struct drm_encoder encoder;
>> struct ast_vga_connector vga_connector;
>> @@ -199,7 +199,7 @@ struct ast_private {
>> ast_use_defaults
>> } config_mode;
>>
>> - enum ast_tx_chip tx_chip_type;
>> + unsigned long tx_chip_types; /* bitfield of enum ast_chip_type */
>> u8 *dp501_fw_addr;
>> const struct firmware *dp501_fw; /* dp501 fw */
>> };
>> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
>> index d770d5a23c1a..50b8d51382c7 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -216,7 +216,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>> }
>>
>> /* Check 3rd Tx option (digital output afaik) */
>> - ast->tx_chip_type = AST_TX_NONE;
>> + ast->tx_chip_types |= BIT(AST_TX_NONE);
>>
>> /*
>> * VGACRA3 Enhanced Color Mode Register, check if DVO is already
>> @@ -229,7 +229,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>> if (!*need_post) {
>> jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xff);
>> if (jreg & 0x80)
>> - ast->tx_chip_type = AST_TX_SIL164;
>> + ast->tx_chip_types = BIT(AST_TX_SIL164);
>> }
>>
>> if ((ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST2500)) {
>> @@ -241,7 +241,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>> jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
>> switch (jreg) {
>> case 0x04:
>> - ast->tx_chip_type = AST_TX_SIL164;
>> + ast->tx_chip_types = BIT(AST_TX_SIL164);
>> break;
>> case 0x08:
>> ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
>> @@ -254,22 +254,19 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>> }
>> fallthrough;
>> case 0x0c:
>> - ast->tx_chip_type = AST_TX_DP501;
>> + ast->tx_chip_types = BIT(AST_TX_DP501);
>> }
>> } else if (ast->chip == AST2600)
>> ast_dp_launch(&ast->base, 0);
>>
>> /* Print stuff for diagnostic purposes */
>> - switch(ast->tx_chip_type) {
>> - case AST_TX_SIL164:
>> + if (ast->tx_chip_types & BIT(AST_TX_NONE))
>> + drm_info(dev, "Using analog VGA\n");
>> + if (ast->tx_chip_types & BIT(AST_TX_SIL164))
>> drm_info(dev, "Using Sil164 TMDS transmitter\n");
>> - break;
>> - case AST_TX_DP501:
>> + if (ast->tx_chip_types & BIT(AST_TX_DP501))
>> drm_info(dev, "Using DP501 DisplayPort transmitter\n");
>> - break;
>> - default:
>> - drm_info(dev, "Analog VGA only\n");
>> - }
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 323af2746aa9..e19dd2f9b3ce 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -997,10 +997,10 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
>> case DRM_MODE_DPMS_ON:
>> ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x01, 0xdf, 0);
>> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xfc, 0);
>> - if (ast->tx_chip_type == AST_TX_DP501)
>> + if (ast->tx_chip_types & BIT(AST_TX_DP501))
>> ast_set_dp501_video_output(crtc->dev, 1);
>>
>> - if (ast->tx_chip_type == AST_TX_ASTDP) {
>> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
>> ast_dp_power_on_off(crtc->dev, AST_DP_POWER_ON);
>> ast_wait_for_vretrace(ast);
>> ast_dp_set_on_off(crtc->dev, 1);
>> @@ -1012,17 +1012,17 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
>> case DRM_MODE_DPMS_SUSPEND:
>> case DRM_MODE_DPMS_OFF:
>> ch = mode;
>> - if (ast->tx_chip_type == AST_TX_DP501)
>> + if (ast->tx_chip_types & BIT(AST_TX_DP501))
>> ast_set_dp501_video_output(crtc->dev, 0);
>> - break;
>>
>> - if (ast->tx_chip_type == AST_TX_ASTDP) {
>> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
>> ast_dp_set_on_off(crtc->dev, 0);
>> ast_dp_power_on_off(crtc->dev, AST_DP_POWER_OFF);
>> }
>>
>> ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x01, 0xdf, 0x20);
>> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xfc, ch);
>> + break;
>> }
>> }
>>
>> @@ -1155,7 +1155,7 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>> ast_crtc_load_lut(ast, crtc);
>>
>> //Set Aspeed Display-Port
>> - if (ast->tx_chip_type == AST_TX_ASTDP)
>> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP))
>> ast_dp_set_mode(crtc, vbios_mode_info);
>>
>> mutex_unlock(&ast->ioregs_lock);
>> @@ -1739,22 +1739,26 @@ int ast_mode_config_init(struct ast_private *ast)
>>
>> ast_crtc_init(dev);
>>
>> - switch (ast->tx_chip_type) {
>> - case AST_TX_NONE:
>> + if (ast->tx_chip_types & BIT(AST_TX_NONE)) {
>> ret = ast_vga_output_init(ast);
>> - break;
>> - case AST_TX_SIL164:
>> + if (ret)
>> + return ret;
>> + }
>> + if (ast->tx_chip_types & BIT(AST_TX_SIL164)) {
>> ret = ast_sil164_output_init(ast);
>> - break;
>> - case AST_TX_DP501:
>> + if (ret)
>> + return ret;
>> + }
>> + if (ast->tx_chip_types & BIT(AST_TX_DP501)) {
>> ret = ast_dp501_output_init(ast);
>> - break;
>> - case AST_TX_ASTDP:
>> + if (ret)
>> + return ret;
>> + }
>> + if (ast->tx_chip_types & BIT(AST_TX_ASTDP)) {
>> ret = ast_astdp_output_init(ast);
>> - break;
>> + if (ret)
>> + return ret;
>> }
>> - if (ret)
>> - return ret;
>>
>> drm_mode_config_reset(dev);
>>
>> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
>> index 0aa9cf0fb5c3..2e1c343b70a3 100644
>> --- a/drivers/gpu/drm/ast/ast_post.c
>> +++ b/drivers/gpu/drm/ast/ast_post.c
>> @@ -391,7 +391,7 @@ void ast_post_gpu(struct drm_device *dev)
>>
>> ast_init_3rdtx(dev);
>> } else {
>> - if (ast->tx_chip_type != AST_TX_NONE)
>> + if (ast->tx_chip_types & BIT(AST_TX_SIL164))
>> ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xcf, 0x80); /* Enable DVO */
>> }
>> }
>> --
>> 2.36.1
>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property
2022-05-31 11:14 [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
2022-05-31 11:14 ` [PATCH v2 1/5] drm/ast: Support multiple outputs Thomas Zimmermann
@ 2022-05-31 11:14 ` Thomas Zimmermann
2022-06-02 7:24 ` Patrik Jakobsson
2022-05-31 11:15 ` [PATCH v2 3/5] drm/ast: Support output polling Thomas Zimmermann
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2022-05-31 11:14 UTC (permalink / raw)
To: airlied, kuohsiang_chou, airlied, daniel, maarten.lankhorst, mripard
Cc: Thomas Zimmermann, dri-devel
Read the display modes from the connectors DDC helper, which also
updates the connector's EDID property. The code for the connector's
.get_modes helper is now shared between VGA and SIL164.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_mode.c | 57 ++--------------------------------
1 file changed, 2 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index e19dd2f9b3ce..4ff8ec1c8931 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1302,37 +1302,19 @@ 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_private *ast = to_ast_private(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->ioregs_lock);
-
- edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
- if (!edid)
- goto err_mutex_unlock;
-
+ count = drm_connector_helper_get_modes_from_ddc(connector);
mutex_unlock(&ast->ioregs_lock);
- count = drm_add_edid_modes(connector, edid);
- kfree(edid);
-
return count;
-
-err_mutex_unlock:
- mutex_unlock(&ast->ioregs_lock);
-err_drm_connector_update_edid_property:
- drm_connector_update_edid_property(connector, NULL);
- return 0;
}
static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
@@ -1406,43 +1388,8 @@ static int ast_vga_output_init(struct ast_private *ast)
* SIL164 Connector
*/
-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_private *ast = to_ast_private(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->ioregs_lock);
-
- edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
- if (!edid)
- goto err_mutex_unlock;
-
- mutex_unlock(&ast->ioregs_lock);
-
- count = drm_add_edid_modes(connector, edid);
- kfree(edid);
-
- return count;
-
-err_mutex_unlock:
- mutex_unlock(&ast->ioregs_lock);
-err_drm_connector_update_edid_property:
- 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 = ast_vga_connector_helper_get_modes, // same as VGA connector
};
static const struct drm_connector_funcs ast_sil164_connector_funcs = {
--
2.36.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property
2022-05-31 11:14 ` [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property Thomas Zimmermann
@ 2022-06-02 7:24 ` Patrik Jakobsson
2022-06-07 8:13 ` Thomas Zimmermann
0 siblings, 1 reply; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 7:24 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Read the display modes from the connectors DDC helper, which also
> updates the connector's EDID property. The code for the connector's
> .get_modes helper is now shared between VGA and SIL164.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/ast/ast_mode.c | 57 ++--------------------------------
> 1 file changed, 2 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index e19dd2f9b3ce..4ff8ec1c8931 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1302,37 +1302,19 @@ static int ast_crtc_init(struct drm_device *dev)
>
> static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
Since this function is now generic you could consider removing _vga
from the name.
Either way:
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> {
> - struct ast_vga_connector *ast_vga_connector = to_ast_vga_connector(connector);
> struct drm_device *dev = connector->dev;
> struct ast_private *ast = to_ast_private(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->ioregs_lock);
> -
> - edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
> - if (!edid)
> - goto err_mutex_unlock;
> -
> + count = drm_connector_helper_get_modes_from_ddc(connector);
> mutex_unlock(&ast->ioregs_lock);
>
> - count = drm_add_edid_modes(connector, edid);
> - kfree(edid);
> -
> return count;
> -
> -err_mutex_unlock:
> - mutex_unlock(&ast->ioregs_lock);
> -err_drm_connector_update_edid_property:
> - drm_connector_update_edid_property(connector, NULL);
> - return 0;
> }
>
> static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
> @@ -1406,43 +1388,8 @@ static int ast_vga_output_init(struct ast_private *ast)
> * SIL164 Connector
> */
>
> -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_private *ast = to_ast_private(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->ioregs_lock);
> -
> - edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
> - if (!edid)
> - goto err_mutex_unlock;
> -
> - mutex_unlock(&ast->ioregs_lock);
> -
> - count = drm_add_edid_modes(connector, edid);
> - kfree(edid);
> -
> - return count;
> -
> -err_mutex_unlock:
> - mutex_unlock(&ast->ioregs_lock);
> -err_drm_connector_update_edid_property:
> - 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 = ast_vga_connector_helper_get_modes, // same as VGA connector
> };
>
> static const struct drm_connector_funcs ast_sil164_connector_funcs = {
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property
2022-06-02 7:24 ` Patrik Jakobsson
@ 2022-06-07 8:13 ` Thomas Zimmermann
2022-06-07 9:08 ` Patrik Jakobsson
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2022-06-07 8:13 UTC (permalink / raw)
To: Patrik Jakobsson; +Cc: David Airlie, dri-devel, Dave Airlie
[-- Attachment #1.1: Type: text/plain, Size: 4605 bytes --]
Hi
Am 02.06.22 um 09:24 schrieb Patrik Jakobsson:
> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Read the display modes from the connectors DDC helper, which also
>> updates the connector's EDID property. The code for the connector's
>> .get_modes helper is now shared between VGA and SIL164.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/ast/ast_mode.c | 57 ++--------------------------------
>> 1 file changed, 2 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index e19dd2f9b3ce..4ff8ec1c8931 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1302,37 +1302,19 @@ static int ast_crtc_init(struct drm_device *dev)
>>
>> static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>
> Since this function is now generic you could consider removing _vga
> from the name.
It's not fully generic. There are DisplayPort functions that do their
own thing.
From how I understand the driver's source code, there's a hardware
setting to switch between either analog VGA or SIL164-based DVI. Both
use the same registers. So the get_modes function can be shared.
Best regards
Thomas
>
> Either way:
> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>
>
>> {
>> - struct ast_vga_connector *ast_vga_connector = to_ast_vga_connector(connector);
>> struct drm_device *dev = connector->dev;
>> struct ast_private *ast = to_ast_private(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->ioregs_lock);
>> -
>> - edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
>> - if (!edid)
>> - goto err_mutex_unlock;
>> -
>> + count = drm_connector_helper_get_modes_from_ddc(connector);
>> mutex_unlock(&ast->ioregs_lock);
>>
>> - count = drm_add_edid_modes(connector, edid);
>> - kfree(edid);
>> -
>> return count;
>> -
>> -err_mutex_unlock:
>> - mutex_unlock(&ast->ioregs_lock);
>> -err_drm_connector_update_edid_property:
>> - drm_connector_update_edid_property(connector, NULL);
>> - return 0;
>> }
>>
>> static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
>> @@ -1406,43 +1388,8 @@ static int ast_vga_output_init(struct ast_private *ast)
>> * SIL164 Connector
>> */
>>
>> -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_private *ast = to_ast_private(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->ioregs_lock);
>> -
>> - edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
>> - if (!edid)
>> - goto err_mutex_unlock;
>> -
>> - mutex_unlock(&ast->ioregs_lock);
>> -
>> - count = drm_add_edid_modes(connector, edid);
>> - kfree(edid);
>> -
>> - return count;
>> -
>> -err_mutex_unlock:
>> - mutex_unlock(&ast->ioregs_lock);
>> -err_drm_connector_update_edid_property:
>> - 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 = ast_vga_connector_helper_get_modes, // same as VGA connector
>> };
>>
>> static const struct drm_connector_funcs ast_sil164_connector_funcs = {
>> --
>> 2.36.1
>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property
2022-06-07 8:13 ` Thomas Zimmermann
@ 2022-06-07 9:08 ` Patrik Jakobsson
0 siblings, 0 replies; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-07 9:08 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Tue, Jun 7, 2022 at 10:13 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 02.06.22 um 09:24 schrieb Patrik Jakobsson:
> > On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Read the display modes from the connectors DDC helper, which also
> >> updates the connector's EDID property. The code for the connector's
> >> .get_modes helper is now shared between VGA and SIL164.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >> drivers/gpu/drm/ast/ast_mode.c | 57 ++--------------------------------
> >> 1 file changed, 2 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> >> index e19dd2f9b3ce..4ff8ec1c8931 100644
> >> --- a/drivers/gpu/drm/ast/ast_mode.c
> >> +++ b/drivers/gpu/drm/ast/ast_mode.c
> >> @@ -1302,37 +1302,19 @@ static int ast_crtc_init(struct drm_device *dev)
> >>
> >> static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
> >
> > Since this function is now generic you could consider removing _vga
> > from the name.
>
> It's not fully generic. There are DisplayPort functions that do their
> own thing.
>
> From how I understand the driver's source code, there's a hardware
> setting to switch between either analog VGA or SIL164-based DVI. Both
> use the same registers. So the get_modes function can be shared.
What I meant is that there's no VGA specific code left so you could
just name it ast_connector_helper_get_modes()
But feel free to name it as you like.
>
> Best regards
> Thomas
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/5] drm/ast: Support output polling
2022-05-31 11:14 [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
2022-05-31 11:14 ` [PATCH v2 1/5] drm/ast: Support multiple outputs Thomas Zimmermann
2022-05-31 11:14 ` [PATCH v2 2/5] drm/ast: Fix updating the connector's EDID property Thomas Zimmermann
@ 2022-05-31 11:15 ` Thomas Zimmermann
2022-06-02 7:25 ` Patrik Jakobsson
2022-06-07 10:03 ` Jani Nikula
2022-05-31 11:15 ` [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized Thomas Zimmermann
` (2 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Thomas Zimmermann @ 2022-05-31 11:15 UTC (permalink / raw)
To: airlied, kuohsiang_chou, airlied, daniel, maarten.lankhorst, mripard
Cc: Thomas Zimmermann, dri-devel
Enable output polling for all connectors. VGA always uses EDID for this. As
there's currently no interrupt handling for the ast devices, we have to use
that trick for the various DP and DVI ports as well.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_mode.c | 14 ++++++++----
drivers/gpu/drm/drm_probe_helper.c | 35 ++++++++++++++++++++++++++++++
include/drm/drm_probe_helper.h | 3 +++
3 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 4ff8ec1c8931..bbc566c4c768 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1319,6 +1319,7 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
.get_modes = ast_vga_connector_helper_get_modes,
+ .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
};
static const struct drm_connector_funcs ast_vga_connector_funcs = {
@@ -1354,7 +1355,7 @@ static int ast_vga_connector_init(struct drm_device *dev,
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;
}
@@ -1390,6 +1391,7 @@ static int ast_vga_output_init(struct ast_private *ast)
static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
.get_modes = ast_vga_connector_helper_get_modes, // same as VGA connector
+ .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
};
static const struct drm_connector_funcs ast_sil164_connector_funcs = {
@@ -1425,7 +1427,7 @@ static int ast_sil164_connector_init(struct drm_device *dev,
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;
}
@@ -1488,6 +1490,7 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
.get_modes = ast_dp501_connector_helper_get_modes,
+ .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
};
static const struct drm_connector_funcs ast_dp501_connector_funcs = {
@@ -1512,7 +1515,7 @@ static int ast_dp501_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;
}
@@ -1575,6 +1578,7 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
.get_modes = ast_astdp_connector_helper_get_modes,
+ .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
};
static const struct drm_connector_funcs ast_astdp_connector_funcs = {
@@ -1599,7 +1603,7 @@ static int ast_astdp_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;
}
@@ -1709,5 +1713,7 @@ int ast_mode_config_init(struct ast_private *ast)
drm_mode_config_reset(dev);
+ drm_kms_helper_poll_init(dev);
+
return 0;
}
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 425f56280d51..4440a7b6b240 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1031,3 +1031,38 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
return count;
}
EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
+
+/**
+ * drm_connector_helper_detect_ctx_from_edid -
+ * Updates the connector's status by reading EDID data
+ * @connector: The connector.
+ * @ctx: The lock-acquisition context.
+ * @force: True if the operation was requested by userspace, false otherwise.
+ *
+ * Returns:
+ * The connector's status as enum drm_connector_status.
+ *
+ * Updates the connector's EDID property by reading the display modes
+ * and returns the connector's status. If the EDID property is set, the
+ * connector is assumed to be connected; and disconnected otherwise.
+ * If the get_modes helper is missing, the default status is 'unknown'.
+ *
+ * See struct drm_connector_helper_funcs.detect_ctx.
+ */
+int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx,
+ bool force)
+{
+ const struct drm_connector_helper_funcs *helper_funcs = connector->helper_private;
+
+ if (!helper_funcs || !helper_funcs->get_modes)
+ return connector_status_unknown;
+
+ helper_funcs->get_modes(connector);
+
+ if (!connector->edid_blob_ptr)
+ return connector_status_disconnected;
+
+ return connector_status_connected;
+}
+EXPORT_SYMBOL(drm_connector_helper_detect_ctx_from_edid);
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index c80cab7a53b7..7408cf010794 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -27,5 +27,8 @@ 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);
+int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx,
+ bool force);
#endif
--
2.36.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] drm/ast: Support output polling
2022-05-31 11:15 ` [PATCH v2 3/5] drm/ast: Support output polling Thomas Zimmermann
@ 2022-06-02 7:25 ` Patrik Jakobsson
2022-06-02 11:00 ` Thomas Zimmermann
2022-06-07 10:03 ` Jani Nikula
1 sibling, 1 reply; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 7:25 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Enable output polling for all connectors. VGA always uses EDID for this. As
> there's currently no interrupt handling for the ast devices, we have to use
> that trick for the various DP and DVI ports as well.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/ast/ast_mode.c | 14 ++++++++----
> drivers/gpu/drm/drm_probe_helper.c | 35 ++++++++++++++++++++++++++++++
> include/drm/drm_probe_helper.h | 3 +++
> 3 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 4ff8ec1c8931..bbc566c4c768 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1319,6 +1319,7 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>
> static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
> .get_modes = ast_vga_connector_helper_get_modes,
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_vga_connector_funcs = {
> @@ -1354,7 +1355,7 @@ static int ast_vga_connector_init(struct drm_device *dev,
> 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;
> }
> @@ -1390,6 +1391,7 @@ static int ast_vga_output_init(struct ast_private *ast)
>
> static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
> .get_modes = ast_vga_connector_helper_get_modes, // same as VGA connector
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_sil164_connector_funcs = {
> @@ -1425,7 +1427,7 @@ static int ast_sil164_connector_init(struct drm_device *dev,
> 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;
> }
> @@ -1488,6 +1490,7 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>
> static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
> .get_modes = ast_dp501_connector_helper_get_modes,
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_dp501_connector_funcs = {
> @@ -1512,7 +1515,7 @@ static int ast_dp501_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;
> }
> @@ -1575,6 +1578,7 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>
> static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
> .get_modes = ast_astdp_connector_helper_get_modes,
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_astdp_connector_funcs = {
> @@ -1599,7 +1603,7 @@ static int ast_astdp_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;
> }
> @@ -1709,5 +1713,7 @@ int ast_mode_config_init(struct ast_private *ast)
>
> drm_mode_config_reset(dev);
>
> + drm_kms_helper_poll_init(dev);
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 425f56280d51..4440a7b6b240 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1031,3 +1031,38 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
> return count;
> }
> EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
> +
> +/**
> + * drm_connector_helper_detect_ctx_from_edid -
> + * Updates the connector's status by reading EDID data
> + * @connector: The connector.
> + * @ctx: The lock-acquisition context.
> + * @force: True if the operation was requested by userspace, false otherwise.
> + *
> + * Returns:
> + * The connector's status as enum drm_connector_status.
> + *
> + * Updates the connector's EDID property by reading the display modes
> + * and returns the connector's status. If the EDID property is set, the
> + * connector is assumed to be connected; and disconnected otherwise.
> + * If the get_modes helper is missing, the default status is 'unknown'.
> + *
> + * See struct drm_connector_helper_funcs.detect_ctx.
> + */
> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *ctx,
> + bool force)
> +{
> + const struct drm_connector_helper_funcs *helper_funcs = connector->helper_private;
> +
> + if (!helper_funcs || !helper_funcs->get_modes)
> + return connector_status_unknown;
> +
> + helper_funcs->get_modes(connector);
> +
> + if (!connector->edid_blob_ptr)
> + return connector_status_disconnected;
This depends on EDID not being cached or other trickery happening in
helper_funcs->get_modes(). Perhaps the docs should mention this?
> +
> + return connector_status_connected;
> +}
> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx_from_edid);
> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
> index c80cab7a53b7..7408cf010794 100644
> --- a/include/drm/drm_probe_helper.h
> +++ b/include/drm/drm_probe_helper.h
> @@ -27,5 +27,8 @@ 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);
> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *ctx,
> + bool force);
>
> #endif
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] drm/ast: Support output polling
2022-06-02 7:25 ` Patrik Jakobsson
@ 2022-06-02 11:00 ` Thomas Zimmermann
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Zimmermann @ 2022-06-02 11:00 UTC (permalink / raw)
To: Patrik Jakobsson; +Cc: David Airlie, dri-devel, Dave Airlie
[-- Attachment #1.1: Type: text/plain, Size: 7321 bytes --]
Hi
Am 02.06.22 um 09:25 schrieb Patrik Jakobsson:
> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Enable output polling for all connectors. VGA always uses EDID for this. As
>> there's currently no interrupt handling for the ast devices, we have to use
>> that trick for the various DP and DVI ports as well.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/ast/ast_mode.c | 14 ++++++++----
>> drivers/gpu/drm/drm_probe_helper.c | 35 ++++++++++++++++++++++++++++++
>> include/drm/drm_probe_helper.h | 3 +++
>> 3 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 4ff8ec1c8931..bbc566c4c768 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1319,6 +1319,7 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>>
>> static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
>> .get_modes = ast_vga_connector_helper_get_modes,
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_vga_connector_funcs = {
>> @@ -1354,7 +1355,7 @@ static int ast_vga_connector_init(struct drm_device *dev,
>> 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;
>> }
>> @@ -1390,6 +1391,7 @@ static int ast_vga_output_init(struct ast_private *ast)
>>
>> static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
>> .get_modes = ast_vga_connector_helper_get_modes, // same as VGA connector
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_sil164_connector_funcs = {
>> @@ -1425,7 +1427,7 @@ static int ast_sil164_connector_init(struct drm_device *dev,
>> 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;
>> }
>> @@ -1488,6 +1490,7 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>>
>> static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
>> .get_modes = ast_dp501_connector_helper_get_modes,
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_dp501_connector_funcs = {
>> @@ -1512,7 +1515,7 @@ static int ast_dp501_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;
>> }
>> @@ -1575,6 +1578,7 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>>
>> static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
>> .get_modes = ast_astdp_connector_helper_get_modes,
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_astdp_connector_funcs = {
>> @@ -1599,7 +1603,7 @@ static int ast_astdp_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;
>> }
>> @@ -1709,5 +1713,7 @@ int ast_mode_config_init(struct ast_private *ast)
>>
>> drm_mode_config_reset(dev);
>>
>> + drm_kms_helper_poll_init(dev);
>> +
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 425f56280d51..4440a7b6b240 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1031,3 +1031,38 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
>> return count;
>> }
>> EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
>> +
>> +/**
>> + * drm_connector_helper_detect_ctx_from_edid -
>> + * Updates the connector's status by reading EDID data
>> + * @connector: The connector.
>> + * @ctx: The lock-acquisition context.
>> + * @force: True if the operation was requested by userspace, false otherwise.
>> + *
>> + * Returns:
>> + * The connector's status as enum drm_connector_status.
>> + *
>> + * Updates the connector's EDID property by reading the display modes
>> + * and returns the connector's status. If the EDID property is set, the
>> + * connector is assumed to be connected; and disconnected otherwise.
>> + * If the get_modes helper is missing, the default status is 'unknown'.
>> + *
>> + * See struct drm_connector_helper_funcs.detect_ctx.
>> + */
>> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + bool force)
>> +{
>> + const struct drm_connector_helper_funcs *helper_funcs = connector->helper_private;
>> +
>> + if (!helper_funcs || !helper_funcs->get_modes)
>> + return connector_status_unknown;
>> +
>> + helper_funcs->get_modes(connector);
>> +
>> + if (!connector->edid_blob_ptr)
>> + return connector_status_disconnected;
>
> This depends on EDID not being cached or other trickery happening in
> helper_funcs->get_modes(). Perhaps the docs should mention this?
Sure, we can mention that.
Best regards
Thomas
>
>
>
>> +
>> + return connector_status_connected;
>> +}
>> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx_from_edid);
>> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
>> index c80cab7a53b7..7408cf010794 100644
>> --- a/include/drm/drm_probe_helper.h
>> +++ b/include/drm/drm_probe_helper.h
>> @@ -27,5 +27,8 @@ 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);
>> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + bool force);
>>
>> #endif
>> --
>> 2.36.1
>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/5] drm/ast: Support output polling
2022-05-31 11:15 ` [PATCH v2 3/5] drm/ast: Support output polling Thomas Zimmermann
2022-06-02 7:25 ` Patrik Jakobsson
@ 2022-06-07 10:03 ` Jani Nikula
2022-06-07 10:50 ` Thomas Zimmermann
1 sibling, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2022-06-07 10:03 UTC (permalink / raw)
To: Thomas Zimmermann, airlied, kuohsiang_chou, airlied, daniel,
maarten.lankhorst, mripard
Cc: dri-devel, Thomas Zimmermann
On Tue, 31 May 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Enable output polling for all connectors. VGA always uses EDID for this. As
> there's currently no interrupt handling for the ast devices, we have to use
> that trick for the various DP and DVI ports as well.
In general, please don't add new helper functions under "drm/ast"
subject prefix. These will go under the radar. I only stumbled on this
by accident.
Please don't add new helper functions around get_modes and EDID
reading. I've been putting a lot of effort into changing how EDID will
be handled going forward, and this will just make it harder. See
e.g. [1], though there are pending review comments.
Please don't use connector->edid_blob_ptr for basically anything in the
drivers, or for logic regarding detection. It's way too overloaded
already, and difficult to untangle.
BR,
Jani.
[1] https://patchwork.freedesktop.org/series/104309/
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/ast/ast_mode.c | 14 ++++++++----
> drivers/gpu/drm/drm_probe_helper.c | 35 ++++++++++++++++++++++++++++++
> include/drm/drm_probe_helper.h | 3 +++
> 3 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 4ff8ec1c8931..bbc566c4c768 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1319,6 +1319,7 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>
> static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
> .get_modes = ast_vga_connector_helper_get_modes,
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_vga_connector_funcs = {
> @@ -1354,7 +1355,7 @@ static int ast_vga_connector_init(struct drm_device *dev,
> 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;
> }
> @@ -1390,6 +1391,7 @@ static int ast_vga_output_init(struct ast_private *ast)
>
> static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
> .get_modes = ast_vga_connector_helper_get_modes, // same as VGA connector
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_sil164_connector_funcs = {
> @@ -1425,7 +1427,7 @@ static int ast_sil164_connector_init(struct drm_device *dev,
> 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;
> }
> @@ -1488,6 +1490,7 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>
> static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
> .get_modes = ast_dp501_connector_helper_get_modes,
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_dp501_connector_funcs = {
> @@ -1512,7 +1515,7 @@ static int ast_dp501_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;
> }
> @@ -1575,6 +1578,7 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>
> static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
> .get_modes = ast_astdp_connector_helper_get_modes,
> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
> };
>
> static const struct drm_connector_funcs ast_astdp_connector_funcs = {
> @@ -1599,7 +1603,7 @@ static int ast_astdp_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;
> }
> @@ -1709,5 +1713,7 @@ int ast_mode_config_init(struct ast_private *ast)
>
> drm_mode_config_reset(dev);
>
> + drm_kms_helper_poll_init(dev);
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 425f56280d51..4440a7b6b240 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1031,3 +1031,38 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
> return count;
> }
> EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
> +
> +/**
> + * drm_connector_helper_detect_ctx_from_edid -
> + * Updates the connector's status by reading EDID data
> + * @connector: The connector.
> + * @ctx: The lock-acquisition context.
> + * @force: True if the operation was requested by userspace, false otherwise.
> + *
> + * Returns:
> + * The connector's status as enum drm_connector_status.
> + *
> + * Updates the connector's EDID property by reading the display modes
> + * and returns the connector's status. If the EDID property is set, the
> + * connector is assumed to be connected; and disconnected otherwise.
> + * If the get_modes helper is missing, the default status is 'unknown'.
> + *
> + * See struct drm_connector_helper_funcs.detect_ctx.
> + */
> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *ctx,
> + bool force)
> +{
> + const struct drm_connector_helper_funcs *helper_funcs = connector->helper_private;
> +
> + if (!helper_funcs || !helper_funcs->get_modes)
> + return connector_status_unknown;
> +
> + helper_funcs->get_modes(connector);
> +
> + if (!connector->edid_blob_ptr)
> + return connector_status_disconnected;
> +
> + return connector_status_connected;
> +}
> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx_from_edid);
> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
> index c80cab7a53b7..7408cf010794 100644
> --- a/include/drm/drm_probe_helper.h
> +++ b/include/drm/drm_probe_helper.h
> @@ -27,5 +27,8 @@ 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);
> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *ctx,
> + bool force);
>
> #endif
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] drm/ast: Support output polling
2022-06-07 10:03 ` Jani Nikula
@ 2022-06-07 10:50 ` Thomas Zimmermann
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Zimmermann @ 2022-06-07 10:50 UTC (permalink / raw)
To: Jani Nikula, airlied, kuohsiang_chou, airlied, daniel,
maarten.lankhorst, mripard
Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 7606 bytes --]
Hi
Am 07.06.22 um 12:03 schrieb Jani Nikula:
> On Tue, 31 May 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Enable output polling for all connectors. VGA always uses EDID for this. As
>> there's currently no interrupt handling for the ast devices, we have to use
>> that trick for the various DP and DVI ports as well.
>
> In general, please don't add new helper functions under "drm/ast"
> subject prefix. These will go under the radar. I only stumbled on this
> by accident.
>
> Please don't add new helper functions around get_modes and EDID
> reading. I've been putting a lot of effort into changing how EDID will
> be handled going forward, and this will just make it harder. See
> e.g. [1], though there are pending review comments.
>
> Please don't use connector->edid_blob_ptr for basically anything in the
> drivers, or for logic regarding detection. It's way too overloaded
> already, and difficult to untangle.
Thanks for the that info. I'll merge patch 1, which is unrelated and
later recreate my patchset on top of your changes.
Best regards
Thomas
>
>
> BR,
> Jani.
>
>
> [1] https://patchwork.freedesktop.org/series/104309/
>
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/ast/ast_mode.c | 14 ++++++++----
>> drivers/gpu/drm/drm_probe_helper.c | 35 ++++++++++++++++++++++++++++++
>> include/drm/drm_probe_helper.h | 3 +++
>> 3 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 4ff8ec1c8931..bbc566c4c768 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1319,6 +1319,7 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>>
>> static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
>> .get_modes = ast_vga_connector_helper_get_modes,
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_vga_connector_funcs = {
>> @@ -1354,7 +1355,7 @@ static int ast_vga_connector_init(struct drm_device *dev,
>> 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;
>> }
>> @@ -1390,6 +1391,7 @@ static int ast_vga_output_init(struct ast_private *ast)
>>
>> static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
>> .get_modes = ast_vga_connector_helper_get_modes, // same as VGA connector
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_sil164_connector_funcs = {
>> @@ -1425,7 +1427,7 @@ static int ast_sil164_connector_init(struct drm_device *dev,
>> 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;
>> }
>> @@ -1488,6 +1490,7 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>>
>> static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
>> .get_modes = ast_dp501_connector_helper_get_modes,
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_dp501_connector_funcs = {
>> @@ -1512,7 +1515,7 @@ static int ast_dp501_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;
>> }
>> @@ -1575,6 +1578,7 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>>
>> static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
>> .get_modes = ast_astdp_connector_helper_get_modes,
>> + .detect_ctx = drm_connector_helper_detect_ctx_from_edid,
>> };
>>
>> static const struct drm_connector_funcs ast_astdp_connector_funcs = {
>> @@ -1599,7 +1603,7 @@ static int ast_astdp_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;
>> }
>> @@ -1709,5 +1713,7 @@ int ast_mode_config_init(struct ast_private *ast)
>>
>> drm_mode_config_reset(dev);
>>
>> + drm_kms_helper_poll_init(dev);
>> +
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 425f56280d51..4440a7b6b240 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1031,3 +1031,38 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
>> return count;
>> }
>> EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
>> +
>> +/**
>> + * drm_connector_helper_detect_ctx_from_edid -
>> + * Updates the connector's status by reading EDID data
>> + * @connector: The connector.
>> + * @ctx: The lock-acquisition context.
>> + * @force: True if the operation was requested by userspace, false otherwise.
>> + *
>> + * Returns:
>> + * The connector's status as enum drm_connector_status.
>> + *
>> + * Updates the connector's EDID property by reading the display modes
>> + * and returns the connector's status. If the EDID property is set, the
>> + * connector is assumed to be connected; and disconnected otherwise.
>> + * If the get_modes helper is missing, the default status is 'unknown'.
>> + *
>> + * See struct drm_connector_helper_funcs.detect_ctx.
>> + */
>> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + bool force)
>> +{
>> + const struct drm_connector_helper_funcs *helper_funcs = connector->helper_private;
>> +
>> + if (!helper_funcs || !helper_funcs->get_modes)
>> + return connector_status_unknown;
>> +
>> + helper_funcs->get_modes(connector);
>> +
>> + if (!connector->edid_blob_ptr)
>> + return connector_status_disconnected;
>> +
>> + return connector_status_connected;
>> +}
>> +EXPORT_SYMBOL(drm_connector_helper_detect_ctx_from_edid);
>> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
>> index c80cab7a53b7..7408cf010794 100644
>> --- a/include/drm/drm_probe_helper.h
>> +++ b/include/drm/drm_probe_helper.h
>> @@ -27,5 +27,8 @@ 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);
>> +int drm_connector_helper_detect_ctx_from_edid(struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx,
>> + bool force);
>>
>> #endif
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized
2022-05-31 11:14 [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
` (2 preceding siblings ...)
2022-05-31 11:15 ` [PATCH v2 3/5] drm/ast: Support output polling Thomas Zimmermann
@ 2022-05-31 11:15 ` Thomas Zimmermann
2022-06-02 7:25 ` Patrik Jakobsson
2022-05-31 11:15 ` [PATCH v2 5/5] drm/ast: Remove struct ast_{vga,sil164}_connector Thomas Zimmermann
2022-05-31 11:16 ` [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
5 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2022-05-31 11:15 UTC (permalink / raw)
To: airlied, kuohsiang_chou, airlied, daniel, maarten.lankhorst, mripard
Cc: Thomas Zimmermann, dri-devel
Expect the hardware to provide a DDC channel. Fail probing if its
initialization fails.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_drv.h | 2 --
drivers/gpu/drm/ast/ast_i2c.c | 7 ++++---
drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++------------------
3 files changed, 22 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 3055b0be7b67..2a55fc7303b9 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -132,7 +132,6 @@ struct ast_i2c_chan {
struct ast_vga_connector {
struct drm_connector base;
- struct ast_i2c_chan *i2c;
};
static inline struct ast_vga_connector *
@@ -143,7 +142,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 93e91c36d649..1d039ff1396e 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.class = I2C_CLASS_DDC;
@@ -143,10 +143,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 bbc566c4c768..5f273b5dd769 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1334,19 +1334,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;
@@ -1406,19 +1405,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.36.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized
2022-05-31 11:15 ` [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized Thomas Zimmermann
@ 2022-06-02 7:25 ` Patrik Jakobsson
2022-06-02 7:42 ` Patrik Jakobsson
0 siblings, 1 reply; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 7:25 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Expect the hardware to provide a DDC channel. Fail probing if its
> initialization fails.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
It's funny how I just did the same thing to gma500. Great minds think alike ;)
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 | 38 ++++++++++++++++------------------
> 3 files changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 3055b0be7b67..2a55fc7303b9 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -132,7 +132,6 @@ struct ast_i2c_chan {
>
> struct ast_vga_connector {
> struct drm_connector base;
> - struct ast_i2c_chan *i2c;
> };
>
> static inline struct ast_vga_connector *
> @@ -143,7 +142,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 93e91c36d649..1d039ff1396e 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.class = I2C_CLASS_DDC;
> @@ -143,10 +143,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 bbc566c4c768..5f273b5dd769 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1334,19 +1334,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;
>
> @@ -1406,19 +1405,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.36.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized
2022-06-02 7:25 ` Patrik Jakobsson
@ 2022-06-02 7:42 ` Patrik Jakobsson
2022-06-02 9:32 ` Thomas Zimmermann
0 siblings, 1 reply; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 7:42 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Thu, Jun 2, 2022 at 9:25 AM Patrik Jakobsson
<patrik.r.jakobsson@gmail.com> wrote:
>
> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Expect the hardware to provide a DDC channel. Fail probing if its
> > initialization fails.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> It's funny how I just did the same thing to gma500. Great minds think alike ;)
>
> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
I just realized that the ast_i2c_chan is never freed. Could be fixed
in a follow-up patch?
>
>
> > ---
> > drivers/gpu/drm/ast/ast_drv.h | 2 --
> > drivers/gpu/drm/ast/ast_i2c.c | 7 ++++---
> > drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++------------------
> > 3 files changed, 22 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> > index 3055b0be7b67..2a55fc7303b9 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.h
> > +++ b/drivers/gpu/drm/ast/ast_drv.h
> > @@ -132,7 +132,6 @@ struct ast_i2c_chan {
> >
> > struct ast_vga_connector {
> > struct drm_connector base;
> > - struct ast_i2c_chan *i2c;
> > };
> >
> > static inline struct ast_vga_connector *
> > @@ -143,7 +142,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 93e91c36d649..1d039ff1396e 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.class = I2C_CLASS_DDC;
> > @@ -143,10 +143,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 bbc566c4c768..5f273b5dd769 100644
> > --- a/drivers/gpu/drm/ast/ast_mode.c
> > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > @@ -1334,19 +1334,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;
> >
> > @@ -1406,19 +1405,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.36.1
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized
2022-06-02 7:42 ` Patrik Jakobsson
@ 2022-06-02 9:32 ` Thomas Zimmermann
2022-06-02 10:34 ` Patrik Jakobsson
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2022-06-02 9:32 UTC (permalink / raw)
To: Patrik Jakobsson; +Cc: David Airlie, dri-devel, Dave Airlie
[-- Attachment #1.1: Type: text/plain, Size: 6242 bytes --]
Hi Patrik
Am 02.06.22 um 09:42 schrieb Patrik Jakobsson:
> On Thu, Jun 2, 2022 at 9:25 AM Patrik Jakobsson
> <patrik.r.jakobsson@gmail.com> wrote:
>>
>> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>
>>> Expect the hardware to provide a DDC channel. Fail probing if its
>>> initialization fails.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> It's funny how I just did the same thing to gma500. Great minds think alike ;)
>>
>> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>
> I just realized that the ast_i2c_chan is never freed. Could be fixed
> in a follow-up patch?
In ast_i2c_create(), there's a call to drmm_add_action_or_reset(), which
sets up ast_i2c_release(), which in turn kfree's the memory. Unless I'm
totally missing something here.
Best regards
Thomas
>
>>
>>
>>> ---
>>> drivers/gpu/drm/ast/ast_drv.h | 2 --
>>> drivers/gpu/drm/ast/ast_i2c.c | 7 ++++---
>>> drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++------------------
>>> 3 files changed, 22 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
>>> index 3055b0be7b67..2a55fc7303b9 100644
>>> --- a/drivers/gpu/drm/ast/ast_drv.h
>>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>>> @@ -132,7 +132,6 @@ struct ast_i2c_chan {
>>>
>>> struct ast_vga_connector {
>>> struct drm_connector base;
>>> - struct ast_i2c_chan *i2c;
>>> };
>>>
>>> static inline struct ast_vga_connector *
>>> @@ -143,7 +142,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 93e91c36d649..1d039ff1396e 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.class = I2C_CLASS_DDC;
>>> @@ -143,10 +143,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 bbc566c4c768..5f273b5dd769 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -1334,19 +1334,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;
>>>
>>> @@ -1406,19 +1405,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.36.1
>>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized
2022-06-02 9:32 ` Thomas Zimmermann
@ 2022-06-02 10:34 ` Patrik Jakobsson
0 siblings, 0 replies; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 10:34 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Thu, Jun 2, 2022 at 11:32 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi Patrik
>
> Am 02.06.22 um 09:42 schrieb Patrik Jakobsson:
> > On Thu, Jun 2, 2022 at 9:25 AM Patrik Jakobsson
> > <patrik.r.jakobsson@gmail.com> wrote:
> >>
> >> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>
> >>> Expect the hardware to provide a DDC channel. Fail probing if its
> >>> initialization fails.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>
> >> It's funny how I just did the same thing to gma500. Great minds think alike ;)
> >>
> >> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> >
> > I just realized that the ast_i2c_chan is never freed. Could be fixed
> > in a follow-up patch?
>
> In ast_i2c_create(), there's a call to drmm_add_action_or_reset(), which
> sets up ast_i2c_release(), which in turn kfree's the memory. Unless I'm
> totally missing something here.
Oh, I missed that. Then never mind :)
>
> Best regards
> Thomas
>
> >
> >>
> >>
> >>> ---
> >>> drivers/gpu/drm/ast/ast_drv.h | 2 --
> >>> drivers/gpu/drm/ast/ast_i2c.c | 7 ++++---
> >>> drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++------------------
> >>> 3 files changed, 22 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> >>> index 3055b0be7b67..2a55fc7303b9 100644
> >>> --- a/drivers/gpu/drm/ast/ast_drv.h
> >>> +++ b/drivers/gpu/drm/ast/ast_drv.h
> >>> @@ -132,7 +132,6 @@ struct ast_i2c_chan {
> >>>
> >>> struct ast_vga_connector {
> >>> struct drm_connector base;
> >>> - struct ast_i2c_chan *i2c;
> >>> };
> >>>
> >>> static inline struct ast_vga_connector *
> >>> @@ -143,7 +142,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 93e91c36d649..1d039ff1396e 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.class = I2C_CLASS_DDC;
> >>> @@ -143,10 +143,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 bbc566c4c768..5f273b5dd769 100644
> >>> --- a/drivers/gpu/drm/ast/ast_mode.c
> >>> +++ b/drivers/gpu/drm/ast/ast_mode.c
> >>> @@ -1334,19 +1334,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;
> >>>
> >>> @@ -1406,19 +1405,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.36.1
> >>>
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/5] drm/ast: Remove struct ast_{vga,sil164}_connector
2022-05-31 11:14 [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
` (3 preceding siblings ...)
2022-05-31 11:15 ` [PATCH v2 4/5] drm/ast: Fail probing if DCC channel could not be initialized Thomas Zimmermann
@ 2022-05-31 11:15 ` Thomas Zimmermann
2022-06-02 7:25 ` Patrik Jakobsson
2022-05-31 11:16 ` [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
5 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2022-05-31 11:15 UTC (permalink / raw)
To: airlied, kuohsiang_chou, airlied, daniel, maarten.lankhorst, mripard
Cc: Thomas Zimmermann, dri-devel
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>
---
drivers/gpu/drm/ast/ast_drv.h | 24 ++----------------------
drivers/gpu/drm/ast/ast_mode.c | 18 ++++++------------
2 files changed, 8 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 2a55fc7303b9..d456f6bc4b2c 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -130,26 +130,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);
-}
-
/*
* Device
*/
@@ -174,11 +154,11 @@ struct ast_private {
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 5f273b5dd769..326f29dae844 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1330,10 +1330,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;
@@ -1364,8 +1362,7 @@ static int ast_vga_output_init(struct ast_private *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);
@@ -1373,7 +1370,7 @@ static int ast_vga_output_init(struct ast_private *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;
@@ -1401,10 +1398,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;
@@ -1435,8 +1430,7 @@ static int ast_sil164_output_init(struct ast_private *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);
@@ -1444,7 +1438,7 @@ static int ast_sil164_output_init(struct ast_private *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;
--
2.36.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/5] drm/ast: Remove struct ast_{vga,sil164}_connector
2022-05-31 11:15 ` [PATCH v2 5/5] drm/ast: Remove struct ast_{vga,sil164}_connector Thomas Zimmermann
@ 2022-06-02 7:25 ` Patrik Jakobsson
0 siblings, 0 replies; 21+ messages in thread
From: Patrik Jakobsson @ 2022-06-02 7:25 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: David Airlie, dri-devel, Dave Airlie
On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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 | 18 ++++++------------
> 2 files changed, 8 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 2a55fc7303b9..d456f6bc4b2c 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -130,26 +130,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);
> -}
> -
> /*
> * Device
> */
> @@ -174,11 +154,11 @@ struct ast_private {
> 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 5f273b5dd769..326f29dae844 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1330,10 +1330,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;
>
> @@ -1364,8 +1362,7 @@ static int ast_vga_output_init(struct ast_private *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);
> @@ -1373,7 +1370,7 @@ static int ast_vga_output_init(struct ast_private *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;
>
> @@ -1401,10 +1398,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;
>
> @@ -1435,8 +1430,7 @@ static int ast_sil164_output_init(struct ast_private *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);
> @@ -1444,7 +1438,7 @@ static int ast_sil164_output_init(struct ast_private *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;
>
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/5] drm/ast: Connector cleanups and polling
2022-05-31 11:14 [PATCH v2 0/5] drm/ast: Connector cleanups and polling Thomas Zimmermann
` (4 preceding siblings ...)
2022-05-31 11:15 ` [PATCH v2 5/5] drm/ast: Remove struct ast_{vga,sil164}_connector Thomas Zimmermann
@ 2022-05-31 11:16 ` Thomas Zimmermann
5 siblings, 0 replies; 21+ messages in thread
From: Thomas Zimmermann @ 2022-05-31 11:16 UTC (permalink / raw)
To: airlied, kuohsiang_chou, airlied, daniel, maarten.lankhorst, mripard
Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1638 bytes --]
Am 31.05.22 um 13:14 schrieb Thomas Zimmermann:
> Various cleanups to ast's conenctor code. Simplifies the code, adds
> support for using VGA and ASTDP connectors at the same time, and
> initializes polling of the connector status.
>
> Patch 1 was first posted at [1], so this patchset starts at version
[1]
https://lore.kernel.org/dri-devel/20220510105010.20712-1-tzimmermann@suse.de/
> 2. The implementation of detect_ctx in patch 3 has been added to DRM
> helpers as it will be useful for other drivers, such as mgag200 and
> udl.
>
> Thomas Zimmermann (5):
> drm/ast: Support multiple outputs
> drm/ast: Fix updating the connector's EDID property
> drm/ast: Support output polling
> drm/ast: Fail probing if DCC channel could not be initialized
> drm/ast: Remove struct ast_{vga,sil164}_connector
>
> drivers/gpu/drm/ast/ast_dp.c | 5 +-
> drivers/gpu/drm/ast/ast_dp501.c | 2 +-
> drivers/gpu/drm/ast/ast_drv.h | 30 +-----
> drivers/gpu/drm/ast/ast_i2c.c | 7 +-
> drivers/gpu/drm/ast/ast_main.c | 21 ++--
> drivers/gpu/drm/ast/ast_mode.c | 165 ++++++++++-------------------
> drivers/gpu/drm/ast/ast_post.c | 2 +-
> drivers/gpu/drm/drm_probe_helper.c | 35 ++++++
> include/drm/drm_probe_helper.h | 3 +
> 9 files changed, 116 insertions(+), 154 deletions(-)
>
>
> base-commit: 2c8cc5cd20e28afe6b63acb28890e5f57d9bf055
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread