[PATCH v2 0/3] drm/mgag200: Control VIDRST and BMC from CRTC

[PATCH v2 0/3] drm/mgag200: Control VIDRST and BMC from CRTC

[Thomas Zimmermann]

(was: drm/mgag200: Handle VIDRST from BMC helpers)

The VIDRST pin controls CRTC synchronization with an external clock
chip, such as a BMC or TV encoder. This patchset separates the CRTC
state from the BMC state and streamlines the driver code.

v2:
- run BMC and VIDRST logic from CRTC code (Jocelyn)

Thomas Zimmermann (3):
  drm/mgag200: Only set VIDRST bits in CRTC modesetting
  drm/mgag200: Remove vidrst callbacks from struct mgag200_device_funcs
  drm/mgag200: Rename BMC vidrst names

 drivers/gpu/drm/mgag200/mgag200_bmc.c     |  9 ++-----
 drivers/gpu/drm/mgag200/mgag200_drv.h     | 31 ++++++++---------------
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |  9 +++----
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  9 +++----
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 --
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |  9 +++----
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 --
 drivers/gpu/drm/mgag200/mgag200_mode.c    | 29 ++++++++++-----------
 8 files changed, 36 insertions(+), 64 deletions(-)

-- 
2.45.2

[PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting

[Thomas Zimmermann]

The VRSTEN and HRSTEN bits control whether a CRTC synchronizes its
display signal with an external source on the VIDRST pin. The G200WB
and G200EW3 models synchronize with a BMC chip, but different external
video encoders, such as the Matrox Maven, can also be attached to the
pin.

Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the
bits are independent from the BMC. Add the field set_vidrst to the CRTC
state for this purpose. Off by default, control the CRTC VIDRST setting
from the CRTC's atomic_check helper.

v2:
- keep logic entirely in CRTC (Jocelyn)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_bmc.c    |  5 -----
 drivers/gpu/drm/mgag200/mgag200_drv.h    |  5 ++++-
 drivers/gpu/drm/mgag200/mgag200_g200er.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200se.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 14 ++++++++++----
 6 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 23ef85aa7e37..1c7aa4f36787 100644
--- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
@@ -77,11 +77,6 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
 {
 	u8 tmp;
 
-	/* Ensure that the vrsten and hrsten are set */
-	WREG8(MGAREG_CRTCEXT_INDEX, 1);
-	tmp = RREG8(MGAREG_CRTCEXT_DATA);
-	WREG8(MGAREG_CRTCEXT_DATA, tmp | 0x88);
-
 	/* Assert rstlvl2 */
 	WREG8(DAC_INDEX, MGA1064_REMHEADCTL2);
 	tmp = RREG8(DAC_DATA);
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 7f7dfbd0f013..4b75613de882 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -179,6 +179,8 @@ struct mgag200_crtc_state {
 	const struct drm_format_info *format;
 
 	struct mgag200_pll_values pixpllc;
+
+	bool set_vidrst;
 };
 
 static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base)
@@ -430,7 +432,8 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st
 	.atomic_duplicate_state = mgag200_crtc_atomic_duplicate_state, \
 	.atomic_destroy_state = mgag200_crtc_atomic_destroy_state
 
-void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode);
+void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode,
+			   bool set_vidrst);
 void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format);
 void mgag200_enable_display(struct mga_device *mdev);
 void mgag200_init_registers(struct mga_device *mdev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index 4e8a1756138d..abfbed6ec390 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -195,7 +195,7 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 		funcs->disable_vidrst(mdev);
 
 	mgag200_set_format_regs(mdev, format);
-	mgag200_set_mode_regs(mdev, adjusted_mode);
+	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
 	if (funcs->pixpllc_atomic_update)
 		funcs->pixpllc_atomic_update(crtc, old_state);
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index d884f3cb0ec7..acc99999e2b5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -196,7 +196,7 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 		funcs->disable_vidrst(mdev);
 
 	mgag200_set_format_regs(mdev, format);
-	mgag200_set_mode_regs(mdev, adjusted_mode);
+	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
 	if (funcs->pixpllc_atomic_update)
 		funcs->pixpllc_atomic_update(crtc, old_state);
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index a824bb8ad579..be4e124102c6 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -327,7 +327,7 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 		funcs->disable_vidrst(mdev);
 
 	mgag200_set_format_regs(mdev, format);
-	mgag200_set_mode_regs(mdev, adjusted_mode);
+	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
 	if (funcs->pixpllc_atomic_update)
 		funcs->pixpllc_atomic_update(crtc, old_state);
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index d4550e4b3b01..e746f365fecf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -201,9 +201,9 @@ void mgag200_init_registers(struct mga_device *mdev)
 	WREG8(MGA_MISC_OUT, misc);
 }
 
-void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode)
+void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode,
+			   bool set_vidrst)
 {
-	const struct mgag200_device_info *info = mdev->info;
 	unsigned int hdisplay, hsyncstart, hsyncend, htotal;
 	unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
 	u8 misc, crtcext1, crtcext2, crtcext5;
@@ -238,9 +238,11 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod
 		   ((hdisplay & 0x100) >> 7) |
 		   ((hsyncstart & 0x100) >> 6) |
 		    (htotal & 0x40);
-	if (info->has_vidrst)
+	if (set_vidrst)
 		crtcext1 |= MGAREG_CRTCEXT1_VRSTEN |
 			    MGAREG_CRTCEXT1_HRSTEN;
+	else
+		crtcext1 &= ~(MGAREG_CRTCEXT1_VRSTEN | MGAREG_CRTCEXT1_HRSTEN);
 
 	crtcext2 = ((vtotal & 0xc00) >> 10) |
 		   ((vdisplay & 0x400) >> 8) |
@@ -597,6 +599,7 @@ 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;
 
@@ -607,6 +610,8 @@ 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->has_vidrst;
+
 	if (new_crtc_state->mode_changed) {
 		if (funcs->pixpllc_atomic_check) {
 			ret = funcs->pixpllc_atomic_check(crtc, new_state);
@@ -656,7 +661,7 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
 		funcs->disable_vidrst(mdev);
 
 	mgag200_set_format_regs(mdev, format);
-	mgag200_set_mode_regs(mdev, adjusted_mode);
+	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
 	if (funcs->pixpllc_atomic_update)
 		funcs->pixpllc_atomic_update(crtc, old_state);
@@ -717,6 +722,7 @@ struct drm_crtc_state *mgag200_crtc_atomic_duplicate_state(struct drm_crtc *crtc
 	new_mgag200_crtc_state->format = mgag200_crtc_state->format;
 	memcpy(&new_mgag200_crtc_state->pixpllc, &mgag200_crtc_state->pixpllc,
 	       sizeof(new_mgag200_crtc_state->pixpllc));
+	new_mgag200_crtc_state->set_vidrst = mgag200_crtc_state->set_vidrst;
 
 	return &new_mgag200_crtc_state->base;
 }
-- 
2.45.2

[PATCH v2 2/3] drm/mgag200: Remove vidrst callbacks from struct mgag200_device_funcs

[Thomas Zimmermann]

The callbacks disable_vidrst and enable_vidrst are obsolete. Remove
the fields from struct mgag200_device_funcs. Instead call their
implementations directly of the field 'has_vidrst' has been set in
struct mgag200_device_info.

Also change the logic slightly. The BMC used to start and stop scanout
during the CRTC's atomic_enable and atomic_disable. Plane updates were
done while the BMC scanned out the display. Now only stop once in
atomic_disable at the beginning of a modeset and then restart the
scanout at the end of a modeset in atomic_enable. While the modeset
takes place, the BMC does not scanout at all.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h     | 12 ------------
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |  7 ++-----
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  7 ++-----
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 --
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |  7 ++-----
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 --
 drivers/gpu/drm/mgag200/mgag200_mode.c    | 15 ++++-----------
 7 files changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 4b75613de882..4a46c8c006c8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -247,18 +247,6 @@ struct mgag200_device_info {
 	}
 
 struct mgag200_device_funcs {
-	/*
-	 * Disables an external reset source (i.e., BMC) before programming
-	 * a new display mode.
-	 */
-	void (*disable_vidrst)(struct mga_device *mdev);
-
-	/*
-	 * Enables an external reset source (i.e., BMC) after programming
-	 * a new display mode.
-	 */
-	void (*enable_vidrst)(struct mga_device *mdev);
-
 	/*
 	 * Validate that the given state can be programmed into PIXPLLC. On
 	 * success, the calculated parameters should be stored in the CRTC's
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index abfbed6ec390..b3bb3e9fb0d1 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -191,9 +191,6 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	const struct drm_format_info *format = mgag200_crtc_state->format;
 
-	if (funcs->disable_vidrst)
-		funcs->disable_vidrst(mdev);
-
 	mgag200_set_format_regs(mdev, format);
 	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
@@ -209,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (funcs->enable_vidrst)
-		funcs->enable_vidrst(mdev);
+	if (mdev->info->has_vidrst)
+		mgag200_bmc_enable_vidrst(mdev);
 }
 
 static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index acc99999e2b5..3ac0a508e2c5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -192,9 +192,6 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	const struct drm_format_info *format = mgag200_crtc_state->format;
 
-	if (funcs->disable_vidrst)
-		funcs->disable_vidrst(mdev);
-
 	mgag200_set_format_regs(mdev, format);
 	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
@@ -210,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (funcs->enable_vidrst)
-		funcs->enable_vidrst(mdev);
+	if (mdev->info->has_vidrst)
+		mgag200_bmc_enable_vidrst(mdev);
 }
 
 static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index 839401e8b465..265f3e95830a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -146,8 +146,6 @@ static const struct mgag200_device_info mgag200_g200ew3_device_info =
 	MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false);
 
 static const struct mgag200_device_funcs mgag200_g200ew3_device_funcs = {
-	.disable_vidrst = mgag200_bmc_disable_vidrst,
-	.enable_vidrst = mgag200_bmc_enable_vidrst,
 	.pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, // same as G200WB
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index be4e124102c6..7a8099eb100c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -323,9 +323,6 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	const struct drm_format_info *format = mgag200_crtc_state->format;
 
-	if (funcs->disable_vidrst)
-		funcs->disable_vidrst(mdev);
-
 	mgag200_set_format_regs(mdev, format);
 	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
@@ -341,8 +338,8 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (funcs->enable_vidrst)
-		funcs->enable_vidrst(mdev);
+	if (mdev->info->has_vidrst)
+		mgag200_bmc_enable_vidrst(mdev);
 }
 
 static const struct drm_crtc_helper_funcs mgag200_g200se_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index 835df0f4fc13..e25477347c3e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -280,8 +280,6 @@ static const struct mgag200_device_info mgag200_g200wb_device_info =
 	MGAG200_DEVICE_INFO_INIT(1280, 1024, 31877, true, 0, 1, false);
 
 static const struct mgag200_device_funcs mgag200_g200wb_device_funcs = {
-	.disable_vidrst = mgag200_bmc_disable_vidrst,
-	.enable_vidrst = mgag200_bmc_enable_vidrst,
 	.pixpllc_atomic_check = mgag200_g200wb_pixpllc_atomic_check,
 	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update,
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e746f365fecf..fcc10723d385 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -657,9 +657,6 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
 	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
 	const struct drm_format_info *format = mgag200_crtc_state->format;
 
-	if (funcs->disable_vidrst)
-		funcs->disable_vidrst(mdev);
-
 	mgag200_set_format_regs(mdev, format);
 	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
 
@@ -673,22 +670,18 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
 
 	mgag200_enable_display(mdev);
 
-	if (funcs->enable_vidrst)
-		funcs->enable_vidrst(mdev);
+	if (mdev->info->has_vidrst)
+		mgag200_bmc_enable_vidrst(mdev);
 }
 
 void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *old_state)
 {
 	struct mga_device *mdev = to_mga_device(crtc->dev);
-	const struct mgag200_device_funcs *funcs = mdev->funcs;
 
-	if (funcs->disable_vidrst)
-		funcs->disable_vidrst(mdev);
+	if (mdev->info->has_vidrst)
+		mgag200_bmc_disable_vidrst(mdev);
 
 	mgag200_disable_display(mdev);
-
-	if (funcs->enable_vidrst)
-		funcs->enable_vidrst(mdev);
 }
 
 void mgag200_crtc_reset(struct drm_crtc *crtc)
-- 
2.45.2

[PATCH v2 3/3] drm/mgag200: Rename BMC vidrst names

[Thomas Zimmermann]

The BMC's scanout synchronization is only indirectly related to the
VIDRST functionality. Do some renaming.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_bmc.c    |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_drv.h    | 14 +++++++-------
 drivers/gpu/drm/mgag200/mgag200_g200er.c |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_g200ev.c |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_g200se.c |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 10 +++++-----
 6 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 1c7aa4f36787..45e35dffb3ea 100644
--- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
@@ -14,7 +14,7 @@ static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connect
 	return container_of(connector, struct mgag200_bmc_connector, base);
 }
 
-void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
+void mgag200_bmc_stop_scanout(struct mga_device *mdev)
 {
 	u8 tmp;
 	int iter_max;
@@ -73,7 +73,7 @@ void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
 	}
 }
 
-void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
+void mgag200_bmc_start_scanout(struct mga_device *mdev)
 {
 	u8 tmp;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 4a46c8c006c8..f97eaa49b089 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -216,8 +216,8 @@ struct mgag200_device_info {
 	 */
 	unsigned long max_mem_bandwidth;
 
-	/* HW has external source (e.g., BMC) to synchronize with */
-	bool has_vidrst:1;
+	/* Synchronize scanout with BMC */
+	bool sync_bmc:1;
 
 	struct {
 		unsigned data_bit:3;
@@ -232,13 +232,13 @@ struct mgag200_device_info {
 };
 
 #define MGAG200_DEVICE_INFO_INIT(_max_hdisplay, _max_vdisplay, _max_mem_bandwidth, \
-				 _has_vidrst, _i2c_data_bit, _i2c_clock_bit, \
+				 _sync_bmc, _i2c_data_bit, _i2c_clock_bit, \
 				 _bug_no_startadd) \
 	{ \
 		.max_hdisplay = (_max_hdisplay), \
 		.max_vdisplay = (_max_vdisplay), \
 		.max_mem_bandwidth = (_max_mem_bandwidth), \
-		.has_vidrst = (_has_vidrst), \
+		.sync_bmc = (_sync_bmc), \
 		.i2c = { \
 			.data_bit = (_i2c_data_bit), \
 			.clock_bit = (_i2c_clock_bit), \
@@ -430,9 +430,9 @@ int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_avail
 /* mgag200_vga.c */
 int mgag200_vga_output_init(struct mga_device *mdev);
 
-				/* mgag200_bmc.c */
-void mgag200_bmc_disable_vidrst(struct mga_device *mdev);
-void mgag200_bmc_enable_vidrst(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_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index b3bb3e9fb0d1..737a48aa9160 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -206,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->has_vidrst)
-		mgag200_bmc_enable_vidrst(mdev);
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_start_scanout(mdev);
 }
 
 static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index 3ac0a508e2c5..8d1ccc2ad94a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -207,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->has_vidrst)
-		mgag200_bmc_enable_vidrst(mdev);
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_start_scanout(mdev);
 }
 
 static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index 7a8099eb100c..cf7f6897838f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -338,8 +338,8 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->has_vidrst)
-		mgag200_bmc_enable_vidrst(mdev);
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_start_scanout(mdev);
 }
 
 static const struct drm_crtc_helper_funcs mgag200_g200se_crtc_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index fcc10723d385..735eb5906892 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -610,7 +610,7 @@ 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->has_vidrst;
+	new_mgag200_crtc_state->set_vidrst = mdev->info->sync_bmc;
 
 	if (new_crtc_state->mode_changed) {
 		if (funcs->pixpllc_atomic_check) {
@@ -670,16 +670,16 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
 
 	mgag200_enable_display(mdev);
 
-	if (mdev->info->has_vidrst)
-		mgag200_bmc_enable_vidrst(mdev);
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_start_scanout(mdev);
 }
 
 void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *old_state)
 {
 	struct mga_device *mdev = to_mga_device(crtc->dev);
 
-	if (mdev->info->has_vidrst)
-		mgag200_bmc_disable_vidrst(mdev);
+	if (mdev->info->sync_bmc)
+		mgag200_bmc_stop_scanout(mdev);
 
 	mgag200_disable_display(mdev);
 }
-- 
2.45.2

Re: [PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting

[Jocelyn Falempe]

On 10/07/2024 10:42, Thomas Zimmermann wrote:
> The VRSTEN and HRSTEN bits control whether a CRTC synchronizes its
> display signal with an external source on the VIDRST pin. The G200WB
> and G200EW3 models synchronize with a BMC chip, but different external
> video encoders, such as the Matrox Maven, can also be attached to the
> pin.
> 
> Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the
> bits are independent from the BMC. Add the field set_vidrst to the CRTC
> state for this purpose. Off by default, control the CRTC VIDRST setting
> from the CRTC's atomic_check helper.

Thanks, I think this has less chance for regression.

I've only a small nitpick below.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> v2:
> - keep logic entirely in CRTC (Jocelyn)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_bmc.c    |  5 -----
>   drivers/gpu/drm/mgag200/mgag200_drv.h    |  5 ++++-
>   drivers/gpu/drm/mgag200/mgag200_g200er.c |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_g200se.c |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_mode.c   | 14 ++++++++++----
>   6 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> index 23ef85aa7e37..1c7aa4f36787 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> @@ -77,11 +77,6 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
>   {
>   	u8 tmp;
>   
> -	/* Ensure that the vrsten and hrsten are set */
> -	WREG8(MGAREG_CRTCEXT_INDEX, 1);
> -	tmp = RREG8(MGAREG_CRTCEXT_DATA);
> -	WREG8(MGAREG_CRTCEXT_DATA, tmp | 0x88);
> -
>   	/* Assert rstlvl2 */
>   	WREG8(DAC_INDEX, MGA1064_REMHEADCTL2);
>   	tmp = RREG8(DAC_DATA);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 7f7dfbd0f013..4b75613de882 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -179,6 +179,8 @@ struct mgag200_crtc_state {
>   	const struct drm_format_info *format;
>   
>   	struct mgag200_pll_values pixpllc;
> +
> +	bool set_vidrst;
>   };
>   
>   static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_state *base)
> @@ -430,7 +432,8 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st
>   	.atomic_duplicate_state = mgag200_crtc_atomic_duplicate_state, \
>   	.atomic_destroy_state = mgag200_crtc_atomic_destroy_state
>   
> -void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode);
> +void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode,
> +			   bool set_vidrst);
>   void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format);
>   void mgag200_enable_display(struct mga_device *mdev);
>   void mgag200_init_registers(struct mga_device *mdev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index 4e8a1756138d..abfbed6ec390 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -195,7 +195,7 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   		funcs->disable_vidrst(mdev);
>   
>   	mgag200_set_format_regs(mdev, format);
> -	mgag200_set_mode_regs(mdev, adjusted_mode);
> +	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
>   	if (funcs->pixpllc_atomic_update)
>   		funcs->pixpllc_atomic_update(crtc, old_state);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index d884f3cb0ec7..acc99999e2b5 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -196,7 +196,7 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   		funcs->disable_vidrst(mdev);
>   
>   	mgag200_set_format_regs(mdev, format);
> -	mgag200_set_mode_regs(mdev, adjusted_mode);
> +	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
>   	if (funcs->pixpllc_atomic_update)
>   		funcs->pixpllc_atomic_update(crtc, old_state);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index a824bb8ad579..be4e124102c6 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -327,7 +327,7 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   		funcs->disable_vidrst(mdev);
>   
>   	mgag200_set_format_regs(mdev, format);
> -	mgag200_set_mode_regs(mdev, adjusted_mode);
> +	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
>   	if (funcs->pixpllc_atomic_update)
>   		funcs->pixpllc_atomic_update(crtc, old_state);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index d4550e4b3b01..e746f365fecf 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -201,9 +201,9 @@ void mgag200_init_registers(struct mga_device *mdev)
>   	WREG8(MGA_MISC_OUT, misc);
>   }
>   
> -void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode)
> +void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode,
> +			   bool set_vidrst)
>   {
> -	const struct mgag200_device_info *info = mdev->info;
>   	unsigned int hdisplay, hsyncstart, hsyncend, htotal;
>   	unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
>   	u8 misc, crtcext1, crtcext2, crtcext5;
> @@ -238,9 +238,11 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod
>   		   ((hdisplay & 0x100) >> 7) |
>   		   ((hsyncstart & 0x100) >> 6) |
>   		    (htotal & 0x40);
> -	if (info->has_vidrst)
> +	if (set_vidrst)
>   		crtcext1 |= MGAREG_CRTCEXT1_VRSTEN |
>   			    MGAREG_CRTCEXT1_HRSTEN;
> +	else
> +		crtcext1 &= ~(MGAREG_CRTCEXT1_VRSTEN | MGAREG_CRTCEXT1_HRSTEN);

The else case is useless, as crtcext1 has already this bits set to 0 
unconditionnaly.

>   
>   	crtcext2 = ((vtotal & 0xc00) >> 10) |
>   		   ((vdisplay & 0x400) >> 8) |
> @@ -597,6 +599,7 @@ 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;
>   
> @@ -607,6 +610,8 @@ 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->has_vidrst;
> +
>   	if (new_crtc_state->mode_changed) {
>   		if (funcs->pixpllc_atomic_check) {
>   			ret = funcs->pixpllc_atomic_check(crtc, new_state);
> @@ -656,7 +661,7 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
>   		funcs->disable_vidrst(mdev);
>   
>   	mgag200_set_format_regs(mdev, format);
> -	mgag200_set_mode_regs(mdev, adjusted_mode);
> +	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
>   	if (funcs->pixpllc_atomic_update)
>   		funcs->pixpllc_atomic_update(crtc, old_state);
> @@ -717,6 +722,7 @@ struct drm_crtc_state *mgag200_crtc_atomic_duplicate_state(struct drm_crtc *crtc
>   	new_mgag200_crtc_state->format = mgag200_crtc_state->format;
>   	memcpy(&new_mgag200_crtc_state->pixpllc, &mgag200_crtc_state->pixpllc,
>   	       sizeof(new_mgag200_crtc_state->pixpllc));
> +	new_mgag200_crtc_state->set_vidrst = mgag200_crtc_state->set_vidrst;
>   
>   	return &new_mgag200_crtc_state->base;
>   }

Re: [PATCH v2 2/3] drm/mgag200: Remove vidrst callbacks from struct mgag200_device_funcs

[Jocelyn Falempe]

On 10/07/2024 10:42, Thomas Zimmermann wrote:
> The callbacks disable_vidrst and enable_vidrst are obsolete. Remove
> the fields from struct mgag200_device_funcs. Instead call their
> implementations directly of the field 'has_vidrst' has been set in
> struct mgag200_device_info.
> 
> Also change the logic slightly. The BMC used to start and stop scanout
> during the CRTC's atomic_enable and atomic_disable. Plane updates were
> done while the BMC scanned out the display. Now only stop once in
> atomic_disable at the beginning of a modeset and then restart the
> scanout at the end of a modeset in atomic_enable. While the modeset
> takes place, the BMC does not scanout at all.


Thanks, it looks good to me.

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

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_drv.h     | 12 ------------
>   drivers/gpu/drm/mgag200/mgag200_g200er.c  |  7 ++-----
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  7 ++-----
>   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 --
>   drivers/gpu/drm/mgag200/mgag200_g200se.c  |  7 ++-----
>   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 --
>   drivers/gpu/drm/mgag200/mgag200_mode.c    | 15 ++++-----------
>   7 files changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4b75613de882..4a46c8c006c8 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -247,18 +247,6 @@ struct mgag200_device_info {
>   	}
>   
>   struct mgag200_device_funcs {
> -	/*
> -	 * Disables an external reset source (i.e., BMC) before programming
> -	 * a new display mode.
> -	 */
> -	void (*disable_vidrst)(struct mga_device *mdev);
> -
> -	/*
> -	 * Enables an external reset source (i.e., BMC) after programming
> -	 * a new display mode.
> -	 */
> -	void (*enable_vidrst)(struct mga_device *mdev);
> -
>   	/*
>   	 * Validate that the given state can be programmed into PIXPLLC. On
>   	 * success, the calculated parameters should be stored in the CRTC's
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index abfbed6ec390..b3bb3e9fb0d1 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -191,9 +191,6 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
>   	const struct drm_format_info *format = mgag200_crtc_state->format;
>   
> -	if (funcs->disable_vidrst)
> -		funcs->disable_vidrst(mdev);
> -
>   	mgag200_set_format_regs(mdev, format);
>   	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
> @@ -209,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (funcs->enable_vidrst)
> -		funcs->enable_vidrst(mdev);
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_enable_vidrst(mdev);
>   }
>   
>   static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index acc99999e2b5..3ac0a508e2c5 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -192,9 +192,6 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
>   	const struct drm_format_info *format = mgag200_crtc_state->format;
>   
> -	if (funcs->disable_vidrst)
> -		funcs->disable_vidrst(mdev);
> -
>   	mgag200_set_format_regs(mdev, format);
>   	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
> @@ -210,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (funcs->enable_vidrst)
> -		funcs->enable_vidrst(mdev);
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_enable_vidrst(mdev);
>   }
>   
>   static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> index 839401e8b465..265f3e95830a 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
> @@ -146,8 +146,6 @@ static const struct mgag200_device_info mgag200_g200ew3_device_info =
>   	MGAG200_DEVICE_INFO_INIT(2048, 2048, 0, true, 0, 1, false);
>   
>   static const struct mgag200_device_funcs mgag200_g200ew3_device_funcs = {
> -	.disable_vidrst = mgag200_bmc_disable_vidrst,
> -	.enable_vidrst = mgag200_bmc_enable_vidrst,
>   	.pixpllc_atomic_check = mgag200_g200ew3_pixpllc_atomic_check,
>   	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update, // same as G200WB
>   };
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index be4e124102c6..7a8099eb100c 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -323,9 +323,6 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
>   	const struct drm_format_info *format = mgag200_crtc_state->format;
>   
> -	if (funcs->disable_vidrst)
> -		funcs->disable_vidrst(mdev);
> -
>   	mgag200_set_format_regs(mdev, format);
>   	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
> @@ -341,8 +338,8 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (funcs->enable_vidrst)
> -		funcs->enable_vidrst(mdev);
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_enable_vidrst(mdev);
>   }
>   
>   static const struct drm_crtc_helper_funcs mgag200_g200se_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> index 835df0f4fc13..e25477347c3e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
> @@ -280,8 +280,6 @@ static const struct mgag200_device_info mgag200_g200wb_device_info =
>   	MGAG200_DEVICE_INFO_INIT(1280, 1024, 31877, true, 0, 1, false);
>   
>   static const struct mgag200_device_funcs mgag200_g200wb_device_funcs = {
> -	.disable_vidrst = mgag200_bmc_disable_vidrst,
> -	.enable_vidrst = mgag200_bmc_enable_vidrst,
>   	.pixpllc_atomic_check = mgag200_g200wb_pixpllc_atomic_check,
>   	.pixpllc_atomic_update = mgag200_g200wb_pixpllc_atomic_update,
>   };
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index e746f365fecf..fcc10723d385 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -657,9 +657,6 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
>   	struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
>   	const struct drm_format_info *format = mgag200_crtc_state->format;
>   
> -	if (funcs->disable_vidrst)
> -		funcs->disable_vidrst(mdev);
> -
>   	mgag200_set_format_regs(mdev, format);
>   	mgag200_set_mode_regs(mdev, adjusted_mode, mgag200_crtc_state->set_vidrst);
>   
> @@ -673,22 +670,18 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (funcs->enable_vidrst)
> -		funcs->enable_vidrst(mdev);
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_enable_vidrst(mdev);
>   }
>   
>   void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *old_state)
>   {
>   	struct mga_device *mdev = to_mga_device(crtc->dev);
> -	const struct mgag200_device_funcs *funcs = mdev->funcs;
>   
> -	if (funcs->disable_vidrst)
> -		funcs->disable_vidrst(mdev);
> +	if (mdev->info->has_vidrst)
> +		mgag200_bmc_disable_vidrst(mdev);
>   
>   	mgag200_disable_display(mdev);
> -
> -	if (funcs->enable_vidrst)
> -		funcs->enable_vidrst(mdev);
>   }
>   
>   void mgag200_crtc_reset(struct drm_crtc *crtc)

Re: [PATCH v2 3/3] drm/mgag200: Rename BMC vidrst names

[Jocelyn Falempe]

On 10/07/2024 10:42, Thomas Zimmermann wrote:
> The BMC's scanout synchronization is only indirectly related to the
> VIDRST functionality. Do some renaming.

Thanks, it looks good to me.

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

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/mgag200/mgag200_bmc.c    |  4 ++--
>   drivers/gpu/drm/mgag200/mgag200_drv.h    | 14 +++++++-------
>   drivers/gpu/drm/mgag200/mgag200_g200er.c |  4 ++--
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c |  4 ++--
>   drivers/gpu/drm/mgag200/mgag200_g200se.c |  4 ++--
>   drivers/gpu/drm/mgag200/mgag200_mode.c   | 10 +++++-----
>   6 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> index 1c7aa4f36787..45e35dffb3ea 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
> @@ -14,7 +14,7 @@ static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connect
>   	return container_of(connector, struct mgag200_bmc_connector, base);
>   }
>   
> -void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
> +void mgag200_bmc_stop_scanout(struct mga_device *mdev)
>   {
>   	u8 tmp;
>   	int iter_max;
> @@ -73,7 +73,7 @@ void mgag200_bmc_disable_vidrst(struct mga_device *mdev)
>   	}
>   }
>   
> -void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
> +void mgag200_bmc_start_scanout(struct mga_device *mdev)
>   {
>   	u8 tmp;
>   
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4a46c8c006c8..f97eaa49b089 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -216,8 +216,8 @@ struct mgag200_device_info {
>   	 */
>   	unsigned long max_mem_bandwidth;
>   
> -	/* HW has external source (e.g., BMC) to synchronize with */
> -	bool has_vidrst:1;
> +	/* Synchronize scanout with BMC */
> +	bool sync_bmc:1;
>   
>   	struct {
>   		unsigned data_bit:3;
> @@ -232,13 +232,13 @@ struct mgag200_device_info {
>   };
>   
>   #define MGAG200_DEVICE_INFO_INIT(_max_hdisplay, _max_vdisplay, _max_mem_bandwidth, \
> -				 _has_vidrst, _i2c_data_bit, _i2c_clock_bit, \
> +				 _sync_bmc, _i2c_data_bit, _i2c_clock_bit, \
>   				 _bug_no_startadd) \
>   	{ \
>   		.max_hdisplay = (_max_hdisplay), \
>   		.max_vdisplay = (_max_vdisplay), \
>   		.max_mem_bandwidth = (_max_mem_bandwidth), \
> -		.has_vidrst = (_has_vidrst), \
> +		.sync_bmc = (_sync_bmc), \
>   		.i2c = { \
>   			.data_bit = (_i2c_data_bit), \
>   			.clock_bit = (_i2c_clock_bit), \
> @@ -430,9 +430,9 @@ int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_avail
>   /* mgag200_vga.c */
>   int mgag200_vga_output_init(struct mga_device *mdev);
>   
> -				/* mgag200_bmc.c */
> -void mgag200_bmc_disable_vidrst(struct mga_device *mdev);
> -void mgag200_bmc_enable_vidrst(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_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> index b3bb3e9fb0d1..737a48aa9160 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
> @@ -206,8 +206,8 @@ static void mgag200_g200er_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->has_vidrst)
> -		mgag200_bmc_enable_vidrst(mdev);
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_start_scanout(mdev);
>   }
>   
>   static const struct drm_crtc_helper_funcs mgag200_g200er_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> index 3ac0a508e2c5..8d1ccc2ad94a 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
> @@ -207,8 +207,8 @@ static void mgag200_g200ev_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->has_vidrst)
> -		mgag200_bmc_enable_vidrst(mdev);
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_start_scanout(mdev);
>   }
>   
>   static const struct drm_crtc_helper_funcs mgag200_g200ev_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> index 7a8099eb100c..cf7f6897838f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
> @@ -338,8 +338,8 @@ static void mgag200_g200se_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->has_vidrst)
> -		mgag200_bmc_enable_vidrst(mdev);
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_start_scanout(mdev);
>   }
>   
>   static const struct drm_crtc_helper_funcs mgag200_g200se_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index fcc10723d385..735eb5906892 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -610,7 +610,7 @@ 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->has_vidrst;
> +	new_mgag200_crtc_state->set_vidrst = mdev->info->sync_bmc;
>   
>   	if (new_crtc_state->mode_changed) {
>   		if (funcs->pixpllc_atomic_check) {
> @@ -670,16 +670,16 @@ void mgag200_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_
>   
>   	mgag200_enable_display(mdev);
>   
> -	if (mdev->info->has_vidrst)
> -		mgag200_bmc_enable_vidrst(mdev);
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_start_scanout(mdev);
>   }
>   
>   void mgag200_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *old_state)
>   {
>   	struct mga_device *mdev = to_mga_device(crtc->dev);
>   
> -	if (mdev->info->has_vidrst)
> -		mgag200_bmc_disable_vidrst(mdev);
> +	if (mdev->info->sync_bmc)
> +		mgag200_bmc_stop_scanout(mdev);
>   
>   	mgag200_disable_display(mdev);
>   }

Re: [PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting

[Thomas Zimmermann]

Hi

Am 10.07.24 um 16:25 schrieb Jocelyn Falempe:
>
>
>> +    if (set_vidrst)
>>           crtcext1 |= MGAREG_CRTCEXT1_VRSTEN |
>>                   MGAREG_CRTCEXT1_HRSTEN;
>> +    else
>> +        crtcext1 &= ~(MGAREG_CRTCEXT1_VRSTEN | MGAREG_CRTCEXT1_HRSTEN);
>
> The else case is useless, as crtcext1 has already this bits set to 0 
> unconditionnaly.

Indeed. Will be fixed.

Best regards
Thomas

>
>>         crtcext2 = ((vtotal & 0xc00) >> 10) |
>>              ((vdisplay & 0x400) >> 8) |
>> @@ -597,6 +599,7 @@ 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;
>>   @@ -607,6 +610,8 @@ 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->has_vidrst;
>> +
>>       if (new_crtc_state->mode_changed) {
>>           if (funcs->pixpllc_atomic_check) {
>>               ret = funcs->pixpllc_atomic_check(crtc, new_state);
>> @@ -656,7 +661,7 @@ void mgag200_crtc_helper_atomic_enable(struct 
>> drm_crtc *crtc, struct drm_atomic_
>>           funcs->disable_vidrst(mdev);
>>         mgag200_set_format_regs(mdev, format);
>> -    mgag200_set_mode_regs(mdev, adjusted_mode);
>> +    mgag200_set_mode_regs(mdev, adjusted_mode, 
>> mgag200_crtc_state->set_vidrst);
>>         if (funcs->pixpllc_atomic_update)
>>           funcs->pixpllc_atomic_update(crtc, old_state);
>> @@ -717,6 +722,7 @@ struct drm_crtc_state 
>> *mgag200_crtc_atomic_duplicate_state(struct drm_crtc *crtc
>>       new_mgag200_crtc_state->format = mgag200_crtc_state->format;
>>       memcpy(&new_mgag200_crtc_state->pixpllc, 
>> &mgag200_crtc_state->pixpllc,
>>              sizeof(new_mgag200_crtc_state->pixpllc));
>> +    new_mgag200_crtc_state->set_vidrst = 
>> mgag200_crtc_state->set_vidrst;
>>         return &new_mgag200_crtc_state->base;
>>   }
>

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