[PATCH 1/4] drm/mgag200: Only set VIDRST bits in CRTC modesetting
Thomas Zimmermann
tzimmermann at suse.de
Thu Jul 4 12:16:39 UTC 2024
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 <tzimmermann at suse.de>
>> ---
>> 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, &mgag200_bmc_encoder_helper_funcs);
>> +
>> encoder->possible_crtcs = drm_crtc_mask(crtc);
>> bmc_connector = &mdev->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);
>> + 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 bb6204002cb3..4f4612192e30 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) |
>> @@ -656,7 +658,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 +719,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;
>> }
>
> With the small nitpick.
>
> Reviewed-by: Jocelyn Falempe <jfalempe at redhat.com>
>
>
--
--
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)
More information about the dri-devel
mailing list