[PATCH 0/5] drm/mgag200: Handle BMC in dedicated VGA output

[PATCH 0/5] drm/mgag200: Handle BMC in dedicated VGA output

[Thomas Zimmermann]

Add a separate output for VGA connectors with BMC support. Convert
code for server models. Keep the original VGA for regular G200 models.

The VGA-BMC output either reports the connected VGA display or, if 
that is not available, the BMC. Makes the BMC connector obsolete, so 
remove it.

The benefit of the VGA-BMC connector is that is works with any
userspace. Most userspace compositors have problems configuring the
current BMC connector. The BMC connector is now gone and only a
VGA connector is present.
 
A different approach to removing the BMC connector has been tried in
[1]. The new series also picks up an idea from an older series, [2]
where BMC support is entirely transparent to the CRTC.
 
[1] https://patchwork.freedesktop.org/series/136084/
[2] https://patchwork.freedesktop.org/series/135931/

Thomas Zimmermann (5):
  drm/mgag200: Add VGA-BMC output
  drm/mgag200: vga-bmc: Transparently handle BMC
  drm/mgag200: vga-bmc: Control CRTC VIDRST flag from encoder
  drm/mgag200: vga-bmc: Control BMC scanout from encoder
  drm/mgag200: Remove BMC output

 drivers/gpu/drm/mgag200/Makefile          |   1 +
 drivers/gpu/drm/mgag200/mgag200_bmc.c     | 102 --------------
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  13 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  |   6 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c |   6 +-
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |   9 +-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |   9 +-
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |   6 +-
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |   9 +-
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |   6 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c    |   9 --
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 156 ++++++++++++++++++++++
 12 files changed, 167 insertions(+), 165 deletions(-)
 create mode 100644 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c

-- 
2.46.0

[PATCH 1/5] drm/mgag200: Add VGA-BMC output

[Thomas Zimmermann]

Duplicate VGA output to VGA-BMC output and update all code for Matrox
server chips. The new output represents a VGA output that has a BMC
attached to it. No functional changes so far.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/Makefile          |  1 +
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  3 +
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 72 +++++++++++++++++++++++
 10 files changed, 83 insertions(+), 7 deletions(-)
 create mode 100644 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
index d1b25f9f6586..5a02203fad12 100644
--- a/drivers/gpu/drm/mgag200/Makefile
+++ b/drivers/gpu/drm/mgag200/Makefile
@@ -12,6 +12,7 @@ mgag200-y := \
 	mgag200_g200se.o \
 	mgag200_g200wb.o \
 	mgag200_mode.o \
+	mgag200_vga_bmc.o \
 	mgag200_vga.o
 
 obj-$(CONFIG_DRM_MGAG200) += mgag200.o
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 8df3c84d2405..1301d3066a49 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -437,6 +437,9 @@ void mgag200_enable_display(struct mga_device *mdev);
 void mgag200_init_registers(struct mga_device *mdev);
 int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_available);
 
+/* mgag200_vga_bmc.c */
+int mgag200_vga_bmc_output_init(struct mga_device *mdev);
+
 /* mgag200_vga.c */
 int mgag200_vga_output_init(struct mga_device *mdev);
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index 72bd8e3421c2..1824c0e59c03 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -215,7 +215,7 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index 1bbb0745b84a..9cb2a165e460 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -119,7 +119,7 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index e65d59173939..e0f7816ad87a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -258,7 +258,7 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index a4890b496050..a2c683f82127 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -259,7 +259,7 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index fbaa97c7e0da..7a2806be2d2e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -128,7 +128,7 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index fcb97e4253c4..b83f3f31ed0e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -390,7 +390,7 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index 33ef35c95acb..cb971ca5e306 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -262,7 +262,7 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
 	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
 
-	ret = mgag200_vga_output_init(mdev);
+	ret = mgag200_vga_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
new file mode 100644
index 000000000000..b6b90632b3c6
--- /dev/null
+++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
+
+#include "mgag200_ddc.h"
+#include "mgag200_drv.h"
+
+static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
+	.destroy = drm_encoder_cleanup
+};
+
+static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
+	.get_modes = drm_connector_helper_get_modes,
+	.detect_ctx = drm_connector_helper_detect_from_ddc
+};
+
+static const struct drm_connector_funcs mgag200_vga_connector_funcs = {
+	.reset                  = drm_atomic_helper_connector_reset,
+	.fill_modes             = drm_helper_probe_single_connector_modes,
+	.destroy                = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state
+};
+
+int mgag200_vga_bmc_output_init(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->base;
+	struct drm_crtc *crtc = &mdev->crtc;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	struct i2c_adapter *ddc;
+	int ret;
+
+	encoder = &mdev->output.vga.encoder;
+	ret = drm_encoder_init(dev, encoder, &mgag200_dac_encoder_funcs,
+			       DRM_MODE_ENCODER_DAC, NULL);
+	if (ret) {
+		drm_err(dev, "drm_encoder_init() failed: %d\n", ret);
+		return ret;
+	}
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
+		drm_err(dev, "failed to add DDC bus: %d\n", ret);
+		return ret;
+	}
+
+	connector = &mdev->output.vga.connector;
+	ret = drm_connector_init_with_ddc(dev, connector,
+					  &mgag200_vga_connector_funcs,
+					  DRM_MODE_CONNECTOR_VGA, ddc);
+	if (ret) {
+		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
+		return ret;
+	}
+	drm_connector_helper_add(connector, &mgag200_vga_connector_helper_funcs);
+
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			    DRM_CONNECTOR_POLL_DISCONNECT;
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret) {
+		drm_err(dev, "drm_connector_attach_encoder() failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
-- 
2.46.0

Re: [PATCH 1/5] drm/mgag200: Add VGA-BMC output

[Jocelyn Falempe]

On 05/08/2024 15:05, Thomas Zimmermann wrote:
> Duplicate VGA output to VGA-BMC output and update all code for Matrox
> server chips. The new output represents a VGA output that has a BMC
> attached to it. No functional changes so far.

Thanks for this work.

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

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/Makefile          |  1 +
>   drivers/gpu/drm/mgag200/mgag200_drv.h     |  3 +
>   drivers/gpu/drm/mgag200/mgag200_g200eh.c  |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200eh3.c |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200er.c  |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200se.c  |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 72 +++++++++++++++++++++++
>   10 files changed, 83 insertions(+), 7 deletions(-)
>   create mode 100644 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> 
> diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
> index d1b25f9f6586..5a02203fad12 100644
> --- a/drivers/gpu/drm/mgag200/Makefile
> +++ b/drivers/gpu/drm/mgag200/Makefile
> @@ -12,6 +12,7 @@ mgag200-y := \
>   	mgag200_g200se.o \
>   	mgag200_g200wb.o \
>   	mgag200_mode.o \
> +	mgag200_vga_bmc.o \
>   	mgag200_vga.o
>   
>   obj-$(CONFIG_DRM_MGAG200) += mgag200.o
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 8df3c84d2405..1301d3066a49 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -437,6 +437,9 @@ void mgag200_enable_display(struct mga_device *mdev);
>   void mgag200_init_registers(struct mga_device *mdev);
>   int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_available);
>   
> +/* mgag200_vga_bmc.c */
> +int mgag200_vga_bmc_output_init(struct mga_device *mdev);
> +
>   /* mgag200_vga.c */
>   int mgag200_vga_output_init(struct mga_device *mdev);
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
> index 72bd8e3421c2..1824c0e59c03 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
> @@ -215,7 +215,7 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
> index 1bbb0745b84a..9cb2a165e460 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
> @@ -119,7 +119,7 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index e65d59173939..e0f7816ad87a 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -258,7 +258,7 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index a4890b496050..a2c683f82127 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -259,7 +259,7 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> index fbaa97c7e0da..7a2806be2d2e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> @@ -128,7 +128,7 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index fcb97e4253c4..b83f3f31ed0e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -390,7 +390,7 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> index 33ef35c95acb..cb971ca5e306 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> @@ -262,7 +262,7 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
>   	drm_mode_crtc_set_gamma_size(crtc, MGAG200_LUT_SIZE);
>   	drm_crtc_enable_color_mgmt(crtc, 0, false, MGAG200_LUT_SIZE);
>   
> -	ret = mgag200_vga_output_init(mdev);
> +	ret = mgag200_vga_bmc_output_init(mdev);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> new file mode 100644
> index 000000000000..b6b90632b3c6
> --- /dev/null
> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include "mgag200_ddc.h"
> +#include "mgag200_drv.h"
> +
> +static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup
> +};
> +
> +static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
> +	.get_modes = drm_connector_helper_get_modes,
> +	.detect_ctx = drm_connector_helper_detect_from_ddc
> +};
> +
> +static const struct drm_connector_funcs mgag200_vga_connector_funcs = {
> +	.reset                  = drm_atomic_helper_connector_reset,
> +	.fill_modes             = drm_helper_probe_single_connector_modes,
> +	.destroy                = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state   = drm_atomic_helper_connector_destroy_state
> +};
> +
> +int mgag200_vga_bmc_output_init(struct mga_device *mdev)
> +{
> +	struct drm_device *dev = &mdev->base;
> +	struct drm_crtc *crtc = &mdev->crtc;
> +	struct drm_encoder *encoder;
> +	struct drm_connector *connector;
> +	struct i2c_adapter *ddc;
> +	int ret;
> +
> +	encoder = &mdev->output.vga.encoder;
> +	ret = drm_encoder_init(dev, encoder, &mgag200_dac_encoder_funcs,
> +			       DRM_MODE_ENCODER_DAC, NULL);
> +	if (ret) {
> +		drm_err(dev, "drm_encoder_init() failed: %d\n", ret);
> +		return ret;
> +	}
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> +	ddc = mgag200_ddc_create(mdev);
> +	if (IS_ERR(ddc)) {
> +		ret = PTR_ERR(ddc);
> +		drm_err(dev, "failed to add DDC bus: %d\n", ret);
> +		return ret;
> +	}
> +
> +	connector = &mdev->output.vga.connector;
> +	ret = drm_connector_init_with_ddc(dev, connector,
> +					  &mgag200_vga_connector_funcs,
> +					  DRM_MODE_CONNECTOR_VGA, ddc);
> +	if (ret) {
> +		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
> +		return ret;
> +	}
> +	drm_connector_helper_add(connector, &mgag200_vga_connector_helper_funcs);
> +
> +	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +			    DRM_CONNECTOR_POLL_DISCONNECT;
> +
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +	if (ret) {
> +		drm_err(dev, "drm_connector_attach_encoder() failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[PATCH 2/5] drm/mgag200: vga-bmc: Transparently handle BMC

[Thomas Zimmermann]

The VGA-BMC connector selects the VGA output if a display has been
attached to the physical connector. Otherwise it selects the BMC
output. In any case, the connector status is set to 'detected', so
that the userspace compositor displays to it.

Depending on the setting, the connector's display modes either come
from the VGA monitor's EDID or from an internal list of BMC-compatible
modes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 50 ++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
index b6b90632b3c6..3a958c3587ac 100644
--- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_edid.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_probe_helper.h>
 
@@ -11,9 +12,54 @@ static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
 	.destroy = drm_encoder_cleanup
 };
 
+static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *connector)
+{
+	struct mga_device *mdev = to_mga_device(connector->dev);
+	const struct mgag200_device_info *minfo = mdev->info;
+	int count;
+
+	count = drm_connector_helper_get_modes(connector);
+
+	if (!count) {
+		/*
+		 * There's no EDID data without a connected monitor. Set BMC-
+		 * compatible modes in this case. The XGA default resolution
+		 * should work well for all BMCs.
+		 */
+		count = drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
+		if (count)
+			drm_set_preferred_mode(connector, 1024, 768);
+	}
+
+	return count;
+}
+
+/*
+ * There's no monitor connected if the DDC did not return an EDID. Still
+ * return 'connected' as there's always a BMC. Incrementing the connector's
+ * epoch counter triggers an update of the related properties.
+ */
+static int mgag200_vga_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
+						       struct drm_modeset_acquire_ctx *ctx,
+						       bool force)
+{
+	enum drm_connector_status old_status, status;
+
+	if (connector->edid_blob_ptr)
+		old_status = connector_status_connected;
+	else
+		old_status = connector_status_disconnected;
+
+	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
+
+	if (status != old_status)
+		++connector->epoch_counter;
+	return connector_status_connected;
+}
+
 static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
-	.get_modes = drm_connector_helper_get_modes,
-	.detect_ctx = drm_connector_helper_detect_from_ddc
+	.get_modes = mgag200_vga_bmc_connector_helper_get_modes,
+	.detect_ctx = mgag200_vga_bmc_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs mgag200_vga_connector_funcs = {
-- 
2.46.0

Re: [PATCH 2/5] drm/mgag200: vga-bmc: Transparently handle BMC

[Jocelyn Falempe]

On 05/08/2024 15:05, Thomas Zimmermann wrote:
> The VGA-BMC connector selects the VGA output if a display has been
> attached to the physical connector. Otherwise it selects the BMC
> output. In any case, the connector status is set to 'detected', so
> that the userspace compositor displays to it.
> 
> Depending on the setting, the connector's display modes either come
> from the VGA monitor's EDID or from an internal list of BMC-compatible
> modes.
> 

Thanks for this work.

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

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 50 ++++++++++++++++++++++-
>   1 file changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> index b6b90632b3c6..3a958c3587ac 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   
>   #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_edid.h>
>   #include <drm/drm_modeset_helper_vtables.h>
>   #include <drm/drm_probe_helper.h>
>   
> @@ -11,9 +12,54 @@ static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
>   	.destroy = drm_encoder_cleanup
>   };
>   
> +static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *connector)
> +{
> +	struct mga_device *mdev = to_mga_device(connector->dev);
> +	const struct mgag200_device_info *minfo = mdev->info;
> +	int count;
> +
> +	count = drm_connector_helper_get_modes(connector);
> +
> +	if (!count) {
> +		/*
> +		 * There's no EDID data without a connected monitor. Set BMC-
> +		 * compatible modes in this case. The XGA default resolution
> +		 * should work well for all BMCs.
> +		 */
> +		count = drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
> +		if (count)
> +			drm_set_preferred_mode(connector, 1024, 768);
> +	}
> +
> +	return count;
> +}
> +
> +/*
> + * There's no monitor connected if the DDC did not return an EDID. Still
> + * return 'connected' as there's always a BMC. Incrementing the connector's
> + * epoch counter triggers an update of the related properties.
> + */
> +static int mgag200_vga_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
> +						       struct drm_modeset_acquire_ctx *ctx,
> +						       bool force)
> +{
> +	enum drm_connector_status old_status, status;
> +
> +	if (connector->edid_blob_ptr)
> +		old_status = connector_status_connected;
> +	else
> +		old_status = connector_status_disconnected;
> +
> +	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
> +
> +	if (status != old_status)
> +		++connector->epoch_counter;
> +	return connector_status_connected;
> +}
> +
>   static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
> -	.get_modes = drm_connector_helper_get_modes,
> -	.detect_ctx = drm_connector_helper_detect_from_ddc
> +	.get_modes = mgag200_vga_bmc_connector_helper_get_modes,
> +	.detect_ctx = mgag200_vga_bmc_connector_helper_detect_ctx,
>   };
>   
>   static const struct drm_connector_funcs mgag200_vga_connector_funcs = {

Re: [PATCH 2/5] drm/mgag200: vga-bmc: Transparently handle BMC

[Jani Nikula]

On Mon, 05 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> The VGA-BMC connector selects the VGA output if a display has been
> attached to the physical connector. Otherwise it selects the BMC
> output. In any case, the connector status is set to 'detected', so
> that the userspace compositor displays to it.
>
> Depending on the setting, the connector's display modes either come
> from the VGA monitor's EDID or from an internal list of BMC-compatible
> modes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 50 ++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> index b6b90632b3c6..3a958c3587ac 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -11,9 +12,54 @@ static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup
>  };
>  
> +static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *connector)
> +{
> +	struct mga_device *mdev = to_mga_device(connector->dev);
> +	const struct mgag200_device_info *minfo = mdev->info;
> +	int count;
> +
> +	count = drm_connector_helper_get_modes(connector);
> +
> +	if (!count) {
> +		/*
> +		 * There's no EDID data without a connected monitor. Set BMC-
> +		 * compatible modes in this case. The XGA default resolution
> +		 * should work well for all BMCs.
> +		 */
> +		count = drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
> +		if (count)
> +			drm_set_preferred_mode(connector, 1024, 768);
> +	}
> +
> +	return count;
> +}
> +
> +/*
> + * There's no monitor connected if the DDC did not return an EDID. Still
> + * return 'connected' as there's always a BMC. Incrementing the connector's
> + * epoch counter triggers an update of the related properties.
> + */
> +static int mgag200_vga_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
> +						       struct drm_modeset_acquire_ctx *ctx,
> +						       bool force)
> +{
> +	enum drm_connector_status old_status, status;
> +
> +	if (connector->edid_blob_ptr)

This is now the only place outside of drm_edid.c that uses edid_blob_ptr
for anything.

Seems like you're using it as a proxy for "had a display connected".

I wish it could be kept private to the EDID code.


BR,
Jani.


> +		old_status = connector_status_connected;
> +	else
> +		old_status = connector_status_disconnected;
> +
> +	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
> +
> +	if (status != old_status)
> +		++connector->epoch_counter;
> +	return connector_status_connected;
> +}
> +
>  static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
> -	.get_modes = drm_connector_helper_get_modes,
> -	.detect_ctx = drm_connector_helper_detect_from_ddc
> +	.get_modes = mgag200_vga_bmc_connector_helper_get_modes,
> +	.detect_ctx = mgag200_vga_bmc_connector_helper_detect_ctx,
>  };
>  
>  static const struct drm_connector_funcs mgag200_vga_connector_funcs = {

-- 
Jani Nikula, Intel

Re: [PATCH 2/5] drm/mgag200: vga-bmc: Transparently handle BMC

[Thomas Zimmermann]

Hi

Am 27.09.24 um 16:08 schrieb Jani Nikula:
> On Mon, 05 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> The VGA-BMC connector selects the VGA output if a display has been
>> attached to the physical connector. Otherwise it selects the BMC
>> output. In any case, the connector status is set to 'detected', so
>> that the userspace compositor displays to it.
>>
>> Depending on the setting, the connector's display modes either come
>> from the VGA monitor's EDID or from an internal list of BMC-compatible
>> modes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 50 ++++++++++++++++++++++-
>>   1 file changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
>> index b6b90632b3c6..3a958c3587ac 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   
>>   #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_edid.h>
>>   #include <drm/drm_modeset_helper_vtables.h>
>>   #include <drm/drm_probe_helper.h>
>>   
>> @@ -11,9 +12,54 @@ static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
>>   	.destroy = drm_encoder_cleanup
>>   };
>>   
>> +static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *connector)
>> +{
>> +	struct mga_device *mdev = to_mga_device(connector->dev);
>> +	const struct mgag200_device_info *minfo = mdev->info;
>> +	int count;
>> +
>> +	count = drm_connector_helper_get_modes(connector);
>> +
>> +	if (!count) {
>> +		/*
>> +		 * There's no EDID data without a connected monitor. Set BMC-
>> +		 * compatible modes in this case. The XGA default resolution
>> +		 * should work well for all BMCs.
>> +		 */
>> +		count = drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
>> +		if (count)
>> +			drm_set_preferred_mode(connector, 1024, 768);
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +/*
>> + * There's no monitor connected if the DDC did not return an EDID. Still
>> + * return 'connected' as there's always a BMC. Incrementing the connector's
>> + * epoch counter triggers an update of the related properties.
>> + */
>> +static int mgag200_vga_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
>> +						       struct drm_modeset_acquire_ctx *ctx,
>> +						       bool force)
>> +{
>> +	enum drm_connector_status old_status, status;
>> +
>> +	if (connector->edid_blob_ptr)
> This is now the only place outside of drm_edid.c that uses edid_blob_ptr
> for anything.
>
> Seems like you're using it as a proxy for "had a display connected".
>
> I wish it could be kept private to the EDID code.

No problem. I'd also prefer to change this to work like in ast, [1] 
where that function tests for the connector's 'physical status'.

But I'd like to store the physical status in struct drm_connector and 
have an optional update helper that detects the actual (logical) status, 
as outlined at [2]. Can we talk about that?

Best regards
Thomas

[1] 
https://gitlab.freedesktop.org/drm/kernel/-/blob/ae2c6d8b3b88c176dff92028941a4023f1b4cb91/drivers/gpu/drm/ast/ast_vga.c#L29
[2] 
https://lore.kernel.org/dri-devel/1d16c1ae-2e27-4daa-b8a6-5eab179ef551@suse.de/

>
>
> BR,
> Jani.
>
>
>> +		old_status = connector_status_connected;
>> +	else
>> +		old_status = connector_status_disconnected;
>> +
>> +	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
>> +
>> +	if (status != old_status)
>> +		++connector->epoch_counter;
>> +	return connector_status_connected;
>> +}
>> +
>>   static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
>> -	.get_modes = drm_connector_helper_get_modes,
>> -	.detect_ctx = drm_connector_helper_detect_from_ddc
>> +	.get_modes = mgag200_vga_bmc_connector_helper_get_modes,
>> +	.detect_ctx = mgag200_vga_bmc_connector_helper_detect_ctx,
>>   };
>>   
>>   static const struct drm_connector_funcs mgag200_vga_connector_funcs = {

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[PATCH 3/5] drm/mgag200: vga-bmc: Control CRTC VIDRST flag from encoder

[Thomas Zimmermann]

Control the VIDRST pin from the VGA-BMC encoder's atomic_check and
remove the respective code from CRTC. Makes the VIDRST functionality
fully composable.

The VIDRST pin allows an external clock source to control the SYNC
signals of the Matrox chip. The functionality is part of the CRTC,
but depends on the presence of the clock source. This is the case for
some BMCs, so control the pin from the VGA-BMC output.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c    |  3 ---
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index dd125ef15559..8dc16821e1cd 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -611,7 +611,6 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st
 	struct mga_device *mdev = to_mga_device(dev);
 	const struct mgag200_device_funcs *funcs = mdev->funcs;
 	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
-	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	struct drm_property_blob *new_gamma_lut = new_crtc_state->gamma_lut;
 	int ret;
 
@@ -622,8 +621,6 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st
 	if (ret)
 		return ret;
 
-	new_mgag200_crtc_state->set_vidrst = mdev->info->sync_bmc;
-
 	if (new_crtc_state->mode_changed) {
 		if (funcs->pixpllc_atomic_check) {
 			ret = funcs->pixpllc_atomic_check(crtc, new_state);
diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
index 3a958c3587ac..77340f2dee17 100644
--- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
@@ -8,6 +8,22 @@
 #include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
+static int mgag200_vga_bmc_encoder_atomic_check(struct drm_encoder *encoder,
+						struct drm_crtc_state *new_crtc_state,
+						struct drm_connector_state *new_connector_state)
+{
+	struct mga_device *mdev = to_mga_device(encoder->dev);
+	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
+
+	new_mgag200_crtc_state->set_vidrst = mdev->info->sync_bmc;
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs mgag200_dac_encoder_helper_funcs = {
+	.atomic_check = mgag200_vga_bmc_encoder_atomic_check,
+};
+
 static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
 	.destroy = drm_encoder_cleanup
 };
@@ -86,6 +102,8 @@ int mgag200_vga_bmc_output_init(struct mga_device *mdev)
 		drm_err(dev, "drm_encoder_init() failed: %d\n", ret);
 		return ret;
 	}
+	drm_encoder_helper_add(encoder, &mgag200_dac_encoder_helper_funcs);
+
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	ddc = mgag200_ddc_create(mdev);
-- 
2.46.0

Re: [PATCH 3/5] drm/mgag200: vga-bmc: Control CRTC VIDRST flag from encoder

[Jocelyn Falempe]

On 05/08/2024 15:05, Thomas Zimmermann wrote:
> Control the VIDRST pin from the VGA-BMC encoder's atomic_check and
> remove the respective code from CRTC. Makes the VIDRST functionality
> fully composable.
> 
> The VIDRST pin allows an external clock source to control the SYNC
> signals of the Matrox chip. The functionality is part of the CRTC,
> but depends on the presence of the clock source. This is the case for
> some BMCs, so control the pin from the VGA-BMC output.


Thanks for this work.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c    |  3 ---
>   drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 18 ++++++++++++++++++
>   2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index dd125ef15559..8dc16821e1cd 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -611,7 +611,6 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st
>   	struct mga_device *mdev = to_mga_device(dev);
>   	const struct mgag200_device_funcs *funcs = mdev->funcs;
>   	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
>   	struct drm_property_blob *new_gamma_lut = new_crtc_state->gamma_lut;
>   	int ret;
>   
> @@ -622,8 +621,6 @@ int mgag200_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_st
>   	if (ret)
>   		return ret;
>   
> -	new_mgag200_crtc_state->set_vidrst = mdev->info->sync_bmc;
> -
>   	if (new_crtc_state->mode_changed) {
>   		if (funcs->pixpllc_atomic_check) {
>   			ret = funcs->pixpllc_atomic_check(crtc, new_state);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> index 3a958c3587ac..77340f2dee17 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> @@ -8,6 +8,22 @@
>   #include "mgag200_ddc.h"
>   #include "mgag200_drv.h"
>   
> +static int mgag200_vga_bmc_encoder_atomic_check(struct drm_encoder *encoder,
> +						struct drm_crtc_state *new_crtc_state,
> +						struct drm_connector_state *new_connector_state)
> +{
> +	struct mga_device *mdev = to_mga_device(encoder->dev);
> +	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
> +
> +	new_mgag200_crtc_state->set_vidrst = mdev->info->sync_bmc;
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs mgag200_dac_encoder_helper_funcs = {
> +	.atomic_check = mgag200_vga_bmc_encoder_atomic_check,
> +};
> +
>   static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
>   	.destroy = drm_encoder_cleanup
>   };
> @@ -86,6 +102,8 @@ int mgag200_vga_bmc_output_init(struct mga_device *mdev)
>   		drm_err(dev, "drm_encoder_init() failed: %d\n", ret);
>   		return ret;
>   	}
> +	drm_encoder_helper_add(encoder, &mgag200_dac_encoder_helper_funcs);
> +
>   	encoder->possible_crtcs = drm_crtc_mask(crtc);
>   
>   	ddc = mgag200_ddc_create(mdev);

[PATCH 4/5] drm/mgag200: vga-bmc: Control BMC scanout from encoder

[Thomas Zimmermann]

Move calls to stop and start BMC scanout from CRTC helpers to the
VGA-BMC encoder's atomic_disable and atomic_enable. Makes the BMC
scanout transparent to the CRTC.

DRM's atomic helpers call an encoder's atomic_disable and atomic_enable
helpers for all enabled encoders. The BMC stops scanning out the VGA
signal if modeset disables the VGA encoder, and starts scanning out
if the modeset enables the VGA encoder.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |  3 ---
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  3 ---
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |  3 ---
 drivers/gpu/drm/mgag200/mgag200_mode.c    |  6 ------
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 20 ++++++++++++++++++++
 5 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index e0f7816ad87a..b99b308a7e54 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -207,9 +207,6 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->sync_bmc)
-		mgag200_bmc_start_scanout(mdev);
-
 	drm_crtc_vblank_on(crtc);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index a2c683f82127..717e4357adcc 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -208,9 +208,6 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->sync_bmc)
-		mgag200_bmc_start_scanout(mdev);
-
 	drm_crtc_vblank_on(crtc);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index b83f3f31ed0e..0db7e14e3328 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -339,9 +339,6 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->sync_bmc)
-		mgag200_bmc_start_scanout(mdev);
-
 	drm_crtc_vblank_on(crtc);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 8dc16821e1cd..7159909aca1e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -693,9 +693,6 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->sync_bmc)
-		mgag200_bmc_start_scanout(mdev);
-
 	drm_crtc_vblank_on(crtc);
 }
 
@@ -705,9 +702,6 @@ void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic
 
 	drm_crtc_vblank_off(crtc);
 
-	if (mdev->info->sync_bmc)
-		mgag200_bmc_stop_scanout(mdev);
-
 	mgag200_disable_display(mdev);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
index 77340f2dee17..a5a3ac108bd5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
@@ -8,6 +8,24 @@
 #include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
+static void mgag200_vga_bmc_encoder_atomic_disable(struct drm_encoder *encoder,
+						   struct drm_atomic_state *state)
+{
+	struct mga_device *mdev = to_mga_device(encoder->dev);
+
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_stop_scanout(mdev);
+}
+
+static void mgag200_vga_bmc_encoder_atomic_enable(struct drm_encoder *encoder,
+						  struct drm_atomic_state *state)
+{
+	struct mga_device *mdev = to_mga_device(encoder->dev);
+
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_start_scanout(mdev);
+}
+
 static int mgag200_vga_bmc_encoder_atomic_check(struct drm_encoder *encoder,
 						struct drm_crtc_state *new_crtc_state,
 						struct drm_connector_state *new_connector_state)
@@ -21,6 +39,8 @@ static int mgag200_vga_bmc_encoder_atomic_check(struct drm_encoder *encoder,
 }
 
 static const struct drm_encoder_helper_funcs mgag200_dac_encoder_helper_funcs = {
+	.atomic_disable = mgag200_vga_bmc_encoder_atomic_disable,
+	.atomic_enable = mgag200_vga_bmc_encoder_atomic_enable,
 	.atomic_check = mgag200_vga_bmc_encoder_atomic_check,
 };
 
-- 
2.46.0

Re: [PATCH 4/5] drm/mgag200: vga-bmc: Control BMC scanout from encoder

[Jocelyn Falempe]

On 05/08/2024 15:06, Thomas Zimmermann wrote:
> Move calls to stop and start BMC scanout from CRTC helpers to the
> VGA-BMC encoder's atomic_disable and atomic_enable. Makes the BMC
> scanout transparent to the CRTC.
> 
> DRM's atomic helpers call an encoder's atomic_disable and atomic_enable
> helpers for all enabled encoders. The BMC stops scanning out the VGA
> signal if modeset disables the VGA encoder, and starts scanning out
> if the modeset enables the VGA encoder.
> 

Thanks for this work.

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

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_g200er.c  |  3 ---
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  3 ---
>   drivers/gpu/drm/mgag200/mgag200_g200se.c  |  3 ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c    |  6 ------
>   drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 20 ++++++++++++++++++++
>   5 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index e0f7816ad87a..b99b308a7e54 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -207,9 +207,6 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->sync_bmc)
> -		mgag200_bmc_start_scanout(mdev);
> -
>   	drm_crtc_vblank_on(crtc);
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index a2c683f82127..717e4357adcc 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -208,9 +208,6 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->sync_bmc)
> -		mgag200_bmc_start_scanout(mdev);
> -
>   	drm_crtc_vblank_on(crtc);
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index b83f3f31ed0e..0db7e14e3328 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -339,9 +339,6 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->sync_bmc)
> -		mgag200_bmc_start_scanout(mdev);
> -
>   	drm_crtc_vblank_on(crtc);
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 8dc16821e1cd..7159909aca1e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -693,9 +693,6 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->sync_bmc)
> -		mgag200_bmc_start_scanout(mdev);
> -
>   	drm_crtc_vblank_on(crtc);
>   }
>   
> @@ -705,9 +702,6 @@ void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic
>   
>   	drm_crtc_vblank_off(crtc);
>   
> -	if (mdev->info->sync_bmc)
> -		mgag200_bmc_stop_scanout(mdev);
> -
>   	mgag200_disable_display(mdev);
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> index 77340f2dee17..a5a3ac108bd5 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> @@ -8,6 +8,24 @@
>   #include "mgag200_ddc.h"
>   #include "mgag200_drv.h"
>   
> +static void mgag200_vga_bmc_encoder_atomic_disable(struct drm_encoder *encoder,
> +						   struct drm_atomic_state *state)
> +{
> +	struct mga_device *mdev = to_mga_device(encoder->dev);
> +
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_stop_scanout(mdev);
> +}
> +
> +static void mgag200_vga_bmc_encoder_atomic_enable(struct drm_encoder *encoder,
> +						  struct drm_atomic_state *state)
> +{
> +	struct mga_device *mdev = to_mga_device(encoder->dev);
> +
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_start_scanout(mdev);
> +}
> +
>   static int mgag200_vga_bmc_encoder_atomic_check(struct drm_encoder *encoder,
>   						struct drm_crtc_state *new_crtc_state,
>   						struct drm_connector_state *new_connector_state)
> @@ -21,6 +39,8 @@ static int mgag200_vga_bmc_encoder_atomic_check(struct drm_encoder *encoder,
>   }
>   
>   static const struct drm_encoder_helper_funcs mgag200_dac_encoder_helper_funcs = {
> +	.atomic_disable = mgag200_vga_bmc_encoder_atomic_disable,
> +	.atomic_enable = mgag200_vga_bmc_encoder_atomic_enable,
>   	.atomic_check = mgag200_vga_bmc_encoder_atomic_check,
>   };
>   

[PATCH 5/5] drm/mgag200: Remove BMC output

[Thomas Zimmermann]

Mgag200's BMC connector tracks the status of an underlying physical
connector and updates the BMC status accordingly. This functionality
works around GNOME's settings app, which cannot handle multiple
outputs on the same CRTC.

The workaround is now obsolete as the VGA-BMC connector handles BMC
support internally. Hence, remove the driver's code and the BMC output
entirely.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_bmc.c     | 102 ----------------------
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  10 ---
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  |   4 -
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c |   4 -
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |   4 -
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |   4 -
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |   4 -
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |   4 -
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |   4 -
 9 files changed, 140 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 45e35dffb3ea..a689c71ff165 100644
--- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
@@ -9,11 +9,6 @@
 
 #include "mgag200_drv.h"
 
-static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct mgag200_bmc_connector, base);
-}
-
 void mgag200_bmc_stop_scanout(struct mga_device *mdev)
 {
 	u8 tmp;
@@ -102,100 +97,3 @@ void mgag200_bmc_start_scanout(struct mga_device *mdev)
 	tmp &= ~0x10;
 	WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
 }
-
-static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = {
-	.destroy = drm_encoder_cleanup,
-};
-
-static int mgag200_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
-						   struct drm_modeset_acquire_ctx *ctx,
-						   bool force)
-{
-	struct mgag200_bmc_connector *bmc_connector = to_mgag200_bmc_connector(connector);
-	struct drm_connector *physical_connector = bmc_connector->physical_connector;
-
-	/*
-	 * Most user-space compositors cannot handle more than one connected
-	 * connector per CRTC. Hence, we only mark the BMC as connected if the
-	 * physical connector is disconnected. If the physical connector's status
-	 * is connected or unknown, the BMC remains disconnected. This has no
-	 * effect on the output of the BMC.
-	 *
-	 * FIXME: Remove this logic once user-space compositors can handle more
-	 *        than one connector per CRTC. The BMC should always be connected.
-	 */
-
-	if (physical_connector && physical_connector->status == connector_status_disconnected)
-		return connector_status_connected;
-
-	return connector_status_disconnected;
-}
-
-static int mgag200_bmc_connector_helper_get_modes(struct drm_connector *connector)
-{
-	struct drm_device *dev = connector->dev;
-	struct mga_device *mdev = to_mga_device(dev);
-	const struct mgag200_device_info *minfo = mdev->info;
-
-	return drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
-}
-
-static const struct drm_connector_helper_funcs mgag200_bmc_connector_helper_funcs = {
-	.get_modes = mgag200_bmc_connector_helper_get_modes,
-	.detect_ctx = mgag200_bmc_connector_helper_detect_ctx,
-};
-
-static const struct drm_connector_funcs mgag200_bmc_connector_funcs = {
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static int mgag200_bmc_connector_init(struct drm_device *dev,
-				      struct mgag200_bmc_connector *bmc_connector,
-				      struct drm_connector *physical_connector)
-{
-	struct drm_connector *connector = &bmc_connector->base;
-	int ret;
-
-	ret = drm_connector_init(dev, connector, &mgag200_bmc_connector_funcs,
-				 DRM_MODE_CONNECTOR_VIRTUAL);
-	if (ret)
-		return ret;
-	drm_connector_helper_add(connector, &mgag200_bmc_connector_helper_funcs);
-
-	bmc_connector->physical_connector = physical_connector;
-
-	return 0;
-}
-
-int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector)
-{
-	struct drm_device *dev = &mdev->base;
-	struct drm_crtc *crtc = &mdev->crtc;
-	struct drm_encoder *encoder;
-	struct mgag200_bmc_connector *bmc_connector;
-	struct drm_connector *connector;
-	int ret;
-
-	encoder = &mdev->output.bmc.encoder;
-	ret = drm_encoder_init(dev, encoder, &mgag200_bmc_encoder_funcs,
-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
-	if (ret)
-		return ret;
-	encoder->possible_crtcs = drm_crtc_mask(crtc);
-
-	bmc_connector = &mdev->output.bmc.bmc_connector;
-	ret = mgag200_bmc_connector_init(dev, bmc_connector, physical_connector);
-	if (ret)
-		return ret;
-	connector = &bmc_connector->base;
-
-	ret = drm_connector_attach_encoder(connector, encoder);
-	if (ret)
-		return ret;
-
-	return 0;
-}
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 1301d3066a49..4760ba92871b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -188,11 +188,6 @@ static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_s
 	return container_of(base, struct mgag200_crtc_state, base);
 }
 
-struct mgag200_bmc_connector {
-	struct drm_connector base;
-	struct drm_connector *physical_connector;
-};
-
 enum mga_type {
 	G200_PCI,
 	G200_AGP,
@@ -283,10 +278,6 @@ struct mga_device {
 			struct drm_encoder encoder;
 			struct drm_connector connector;
 		} vga;
-		struct {
-			struct drm_encoder encoder;
-			struct mgag200_bmc_connector bmc_connector;
-		} bmc;
 	} output;
 };
 
@@ -446,6 +437,5 @@ int mgag200_vga_output_init(struct mga_device *mdev);
 /* mgag200_bmc.c */
 void mgag200_bmc_stop_scanout(struct mga_device *mdev);
 void mgag200_bmc_start_scanout(struct mga_device *mdev);
-int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector);
 
 #endif				/* __MGAG200_DRV_H__ */
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index 1824c0e59c03..09ced65c1d2f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -219,10 +219,6 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index 9cb2a165e460..5daa469137bd 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -123,10 +123,6 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index b99b308a7e54..09cfffafe130 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -259,10 +259,6 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index 717e4357adcc..3d48baa91d8b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -260,10 +260,6 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index 7a2806be2d2e..dabc778e64e8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -132,10 +132,6 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index 0db7e14e3328..9dcbe8304271 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -391,10 +391,6 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index cb971ca5e306..83a24aedbf2f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -266,10 +266,6 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
-- 
2.46.0

Re: [PATCH 5/5] drm/mgag200: Remove BMC output

[Jocelyn Falempe]

On 05/08/2024 15:06, Thomas Zimmermann wrote:
> Mgag200's BMC connector tracks the status of an underlying physical
> connector and updates the BMC status accordingly. This functionality
> works around GNOME's settings app, which cannot handle multiple
> outputs on the same CRTC.
> 
> The workaround is now obsolete as the VGA-BMC connector handles BMC
> support internally. Hence, remove the driver's code and the BMC output
> entirely.


Thanks for this work.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_bmc.c     | 102 ----------------------
>   drivers/gpu/drm/mgag200/mgag200_drv.h     |  10 ---
>   drivers/gpu/drm/mgag200/mgag200_g200eh.c  |   4 -
>   drivers/gpu/drm/mgag200/mgag200_g200eh3.c |   4 -
>   drivers/gpu/drm/mgag200/mgag200_g200er.c  |   4 -
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c  |   4 -
>   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |   4 -
>   drivers/gpu/drm/mgag200/mgag200_g200se.c  |   4 -
>   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |   4 -
>   9 files changed, 140 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> index 45e35dffb3ea..a689c71ff165 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> @@ -9,11 +9,6 @@
>   
>   #include "mgag200_drv.h"
>   
> -static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct mgag200_bmc_connector, base);
> -}
> -
>   void mgag200_bmc_stop_scanout(struct mga_device *mdev)
>   {
>   	u8 tmp;
> @@ -102,100 +97,3 @@ void mgag200_bmc_start_scanout(struct mga_device *mdev)
>   	tmp &= ~0x10;
>   	WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
>   }
> -
> -static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = {
> -	.destroy = drm_encoder_cleanup,
> -};
> -
> -static int mgag200_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
> -						   struct drm_modeset_acquire_ctx *ctx,
> -						   bool force)
> -{
> -	struct mgag200_bmc_connector *bmc_connector = to_mgag200_bmc_connector(connector);
> -	struct drm_connector *physical_connector = bmc_connector->physical_connector;
> -
> -	/*
> -	 * Most user-space compositors cannot handle more than one connected
> -	 * connector per CRTC. Hence, we only mark the BMC as connected if the
> -	 * physical connector is disconnected. If the physical connector's status
> -	 * is connected or unknown, the BMC remains disconnected. This has no
> -	 * effect on the output of the BMC.
> -	 *
> -	 * FIXME: Remove this logic once user-space compositors can handle more
> -	 *        than one connector per CRTC. The BMC should always be connected.
> -	 */
> -
> -	if (physical_connector && physical_connector->status == connector_status_disconnected)
> -		return connector_status_connected;
> -
> -	return connector_status_disconnected;
> -}
> -
> -static int mgag200_bmc_connector_helper_get_modes(struct drm_connector *connector)
> -{
> -	struct drm_device *dev = connector->dev;
> -	struct mga_device *mdev = to_mga_device(dev);
> -	const struct mgag200_device_info *minfo = mdev->info;
> -
> -	return drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
> -}
> -
> -static const struct drm_connector_helper_funcs mgag200_bmc_connector_helper_funcs = {
> -	.get_modes = mgag200_bmc_connector_helper_get_modes,
> -	.detect_ctx = mgag200_bmc_connector_helper_detect_ctx,
> -};
> -
> -static const struct drm_connector_funcs mgag200_bmc_connector_funcs = {
> -	.reset = drm_atomic_helper_connector_reset,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static int mgag200_bmc_connector_init(struct drm_device *dev,
> -				      struct mgag200_bmc_connector *bmc_connector,
> -				      struct drm_connector *physical_connector)
> -{
> -	struct drm_connector *connector = &bmc_connector->base;
> -	int ret;
> -
> -	ret = drm_connector_init(dev, connector, &mgag200_bmc_connector_funcs,
> -				 DRM_MODE_CONNECTOR_VIRTUAL);
> -	if (ret)
> -		return ret;
> -	drm_connector_helper_add(connector, &mgag200_bmc_connector_helper_funcs);
> -
> -	bmc_connector->physical_connector = physical_connector;
> -
> -	return 0;
> -}
> -
> -int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector)
> -{
> -	struct drm_device *dev = &mdev->base;
> -	struct drm_crtc *crtc = &mdev->crtc;
> -	struct drm_encoder *encoder;
> -	struct mgag200_bmc_connector *bmc_connector;
> -	struct drm_connector *connector;
> -	int ret;
> -
> -	encoder = &mdev->output.bmc.encoder;
> -	ret = drm_encoder_init(dev, encoder, &mgag200_bmc_encoder_funcs,
> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> -	if (ret)
> -		return ret;
> -	encoder->possible_crtcs = drm_crtc_mask(crtc);
> -
> -	bmc_connector = &mdev->output.bmc.bmc_connector;
> -	ret = mgag200_bmc_connector_init(dev, bmc_connector, physical_connector);
> -	if (ret)
> -		return ret;
> -	connector = &bmc_connector->base;
> -
> -	ret = drm_connector_attach_encoder(connector, encoder);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> -}
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 1301d3066a49..4760ba92871b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -188,11 +188,6 @@ static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_s
>   	return container_of(base, struct mgag200_crtc_state, base);
>   }
>   
> -struct mgag200_bmc_connector {
> -	struct drm_connector base;
> -	struct drm_connector *physical_connector;
> -};
> -
>   enum mga_type {
>   	G200_PCI,
>   	G200_AGP,
> @@ -283,10 +278,6 @@ struct mga_device {
>   			struct drm_encoder encoder;
>   			struct drm_connector connector;
>   		} vga;
> -		struct {
> -			struct drm_encoder encoder;
> -			struct mgag200_bmc_connector bmc_connector;
> -		} bmc;
>   	} output;
>   };
>   
> @@ -446,6 +437,5 @@ int mgag200_vga_output_init(struct mga_device *mdev);
>   /* mgag200_bmc.c */
>   void mgag200_bmc_stop_scanout(struct mga_device *mdev);
>   void mgag200_bmc_start_scanout(struct mga_device *mdev);
> -int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector);
>   
>   #endif				/* __MGAG200_DRV_H__ */
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
> index 1824c0e59c03..09ced65c1d2f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
> @@ -219,10 +219,6 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
> index 9cb2a165e460..5daa469137bd 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
> @@ -123,10 +123,6 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index b99b308a7e54..09cfffafe130 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -259,10 +259,6 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index 717e4357adcc..3d48baa91d8b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -260,10 +260,6 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> index 7a2806be2d2e..dabc778e64e8 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> @@ -132,10 +132,6 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index 0db7e14e3328..9dcbe8304271 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -391,10 +391,6 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> index cb971ca5e306..83a24aedbf2f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> @@ -266,10 +266,6 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
> -	if (ret)
> -		return ret;
> -
>   	return 0;
>   }
>