Re: [PATCH 1/4] drm/mgag200: Only set VIDRST bits in CRTC modesetting
On 04/07/2024 14:16, Thomas Zimmermann wrote:
Hi
Am 04.07.24 um 14:03 schrieb Jocelyn Falempe:
On 03/07/2024 15:40, 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.
If I understand correctly, it's a kind of VSYNC with the BMC, to avoid
tearing when using the remote console ?
I closely looked through the code behind enable_vidrst and
disable_vidrst. The involved registers are mostly undocumented, but from
the comments I assume that the BMC has to stop scanning out the display
signal. It's likely that it only picks up mode changes after the scanout
has been re-enabled.
BTW we've seen various models with BMC attached, but only G200EW3 and
G200WB use this code for synchronization. Do you think we could enable
it for all models and BMCs?
From one side it makes sense because all those chips are made for BMC.
On the other hand, it may break, and we don't know what the other BMC
firmwares are doing, and we even don't know if those pins are connected.
So I prefer to stay on the safe side, and keep it like this.
Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the
driver maintains the bits independently 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 BMC's atomic_check helper. So if a
BMC (or another external clock) requires synchronization, it instructs
the CRTC to synchronize. >
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_bmc.c | 26 +++-
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 | 11 ++
6 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c
b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 23ef85aa7e37..cb5400333862 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);
@@ -108,6 +103,25 @@ void mgag200_bmc_enable_vidrst(struct mga_device
*mdev)
WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
}
+static int mgag200_bmc_encoder_helper_atomic_check(struct
drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct mga_device *mdev = to_mga_device(encoder->dev);
+ struct mgag200_crtc_state *mgag200_crtc_state =
to_mgag200_crtc_state(crtc_state);
+
+ if (mdev->info->has_vidrst)
+ mgag200_crtc_state->set_vidrst = true;
+ else
+ mgag200_crtc_state->set_vidrst = false;
+
I think you can simplify it with:
mgag200_crtc_state->set_vidrst = mdev->info->has_vidrst;
Ok.
Best regards
Thomas
+ return 0;
+}
+
+static const struct drm_encoder_helper_funcs
mgag200_bmc_encoder_helper_funcs = {
+ .atomic_check = mgag200_bmc_encoder_helper_atomic_check,
+};
+
static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
@@ -190,6 +204,8 @@ int mgag200_bmc_output_init(struct mga_device
*mdev, struct drm_connector *physi
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
+ drm_encoder_helper_add(encoder, _bmc_encoder_helper_funcs);
+
encoder->possible_crtcs = drm_crtc_mask(crtc);
bmc_connector = >output.bmc.bmc_connector;
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
Re: [PATCH 1/4] drm/mgag200: Only set VIDRST bits in CRTC modesetting
Hi
Am 04.07.24 um 14:03 schrieb Jocelyn Falempe:
On 03/07/2024 15:40, 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.
If I understand correctly, it's a kind of VSYNC with the BMC, to avoid
tearing when using the remote console ?
I closely looked through the code behind enable_vidrst and
disable_vidrst. The involved registers are mostly undocumented, but from
the comments I assume that the BMC has to stop scanning out the display
signal. It's likely that it only picks up mode changes after the scanout
has been re-enabled.
BTW we've seen various models with BMC attached, but only G200EW3 and
G200WB use this code for synchronization. Do you think we could enable
it for all models and BMCs?
Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the
driver maintains the bits independently 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 BMC's atomic_check helper. So if a
BMC (or another external clock) requires synchronization, it instructs
the CRTC to synchronize. >
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_bmc.c | 26 +++-
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 | 11 ++
6 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c
b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 23ef85aa7e37..cb5400333862 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);
@@ -108,6 +103,25 @@ void mgag200_bmc_enable_vidrst(struct mga_device
*mdev)
WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
}
+static int mgag200_bmc_encoder_helper_atomic_check(struct
drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct mga_device *mdev = to_mga_device(encoder->dev);
+ struct mgag200_crtc_state *mgag200_crtc_state =
to_mgag200_crtc_state(crtc_state);
+
+ if (mdev->info->has_vidrst)
+ mgag200_crtc_state->set_vidrst = true;
+ else
+ mgag200_crtc_state->set_vidrst = false;
+
I think you can simplify it with:
mgag200_crtc_state->set_vidrst = mdev->info->has_vidrst;
Ok.
Best regards
Thomas
+ return 0;
+}
+
+static const struct drm_encoder_helper_funcs
mgag200_bmc_encoder_helper_funcs = {
+ .atomic_check = mgag200_bmc_encoder_helper_atomic_check,
+};
+
static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
@@ -190,6 +204,8 @@ int mgag200_bmc_output_init(struct mga_device
*mdev, struct drm_connector *physi
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
+ drm_encoder_helper_add(encoder, _bmc_encoder_helper_funcs);
+
encoder->possible_crtcs = drm_crtc_mask(crtc);
bmc_connector = >output.bmc.bmc_connector;
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
Re: [PATCH 1/4] drm/mgag200: Only set VIDRST bits in CRTC modesetting
On 03/07/2024 15:40, 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.
If I understand correctly, it's a kind of VSYNC with the BMC, to avoid
tearing when using the remote console ?
Only set VRSTEN and HRSTEN bits in the CRTC mode-setting code, so the
driver maintains the bits independently 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 BMC's atomic_check helper. So if a
BMC (or another external clock) requires synchronization, it instructs
the CRTC to synchronize. >
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_bmc.c| 26 +++-
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 | 11 ++
6 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c
b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 23ef85aa7e37..cb5400333862 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);
@@ -108,6 +103,25 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev)
WREG_DAC(MGA1064_GEN_IO_DATA, tmp);
}
+static int mgag200_bmc_encoder_helper_atomic_check(struct drm_encoder *encoder,
+ struct drm_crtc_state
*crtc_state,
+ struct drm_connector_state
*conn_state)
+{
+ struct mga_device *mdev = to_mga_device(encoder->dev);
+ struct mgag200_crtc_state *mgag200_crtc_state =
to_mgag200_crtc_state(crtc_state);
+
+ if (mdev->info->has_vidrst)
+ mgag200_crtc_state->set_vidrst = true;
+ else
+ mgag200_crtc_state->set_vidrst = false;
+
I think you can simplify it with:
mgag200_crtc_state->set_vidrst = mdev->info->has_vidrst;
+ return 0;
+}
+
+static const struct drm_encoder_helper_funcs mgag200_bmc_encoder_helper_funcs
= {
+ .atomic_check = mgag200_bmc_encoder_helper_atomic_check,
+};
+
static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
@@ -190,6 +204,8 @@ int mgag200_bmc_output_init(struct mga_device *mdev, struct
drm_connector *physi
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
+ drm_encoder_helper_add(encoder, _bmc_encoder_helper_funcs);
+
encoder->possible_crtcs = drm_crtc_mask(crtc);
bmc_connector = >output.bmc.bmc_connector;
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);
+
