| Message ID | 20240710084609.354578-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mgag200: Control VIDRST and BMC from CRTC | expand |
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; > }
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; >> } >
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; }
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(-)