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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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>







[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux