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

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

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 <tzimmermann@xxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>