[PATCH v2 1/3] drm/mgag200: Only set VIDRST bits in CRTC modesetting
Jocelyn Falempe
jfalempe at redhat.com
Wed Jul 10 14:25:43 UTC 2024
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 at redhat.com>
>
> v2:
> - keep logic entirely in CRTC (Jocelyn)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at 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;
> }
More information about the dri-devel
mailing list